Add TempTableDefinitions and TableVarMapping #278

davidtme · 2017-11-16T14:37:21Z

This allows you to add temp tables via the TempTableDefinitions parameter and table var types via the TableVarMapping parameter

smoothdeveloper · 2018-05-11T05:49:40Z

The features of this PR are interesting, I'd like to see those two changes:

trim the changes not related to the PR (project files, fsharp.core, config file changes)
add comments describing the implementation details
possibly, add few unit tests to cover the string parsing logic of the implementation details, will help if we encounter bugs / corner cases in the future

Does the TVP type generated for the command unifies with the ones exposed in SqlProgrammabilityProvider's user defined types?

vasily-kirichenko · 2018-05-12T13:27:14Z

@davidtme I agree with @smoothdeveloper, could you please remove all unrelated changes, after which we could merge it? Thanks!

smoothdeveloper · 2018-05-12T20:18:43Z

src/SqlClient/SqlCommandProvider.fs

@@ -104,13 +109,19 @@ type SqlCommandProvider(config : TypeProviderConfig) as this =
        conn.CheckVersion()
        conn.LoadDataTypesMap()

-        let parameters = DesignTime.ExtractParameters(conn, sqlStatement, allParametersOptional)
+        let designTimeSqlStatement, tempTableNames = 
+            let sql, tempTableNames = DesignTime.SubstituteTempTables(conn, sqlStatement, tempTableDefinitions)


@davidtme we need a conditional to not go in that codepath if tempTableDefinitions is null or empty, because without that conditional, we are at risk of not having a workaround if SubstituteTempTables alters the statement in unexpected ways (SQL statements can have # in them, you can have that frequently in strings and dynamical statement).

I'd like the new codepaths to only be active when the parameter is actually defined by the user.

davidtme · 2018-05-16T14:12:27Z

I'll let #291 get in first then I'll go about making the changes suggested.

vasily-kirichenko · 2018-05-16T15:18:16Z

@davidtme OK. However, I'm stuck with #291, so no ETA.

vasily-kirichenko · 2018-05-17T17:29:19Z

@davidtme I'm not sure I'll finish the netstandard branch anytime soon (if ever), I think we should merge this PR. So, let's make the requested changes and we'll merge it.

# Conflicts: # RELEASE_NOTES.md # src/SqlClient/DesignTime.fs

davidtme · 2018-05-18T14:13:26Z

@smoothdeveloper The type(s) create to load data in to the temp tables live inside the command so are access like:

use cmd = new TempTable(conn)
cmd.LoadTempTables([ TempTable.Temp(Id = 1, Name = Some "monkey") ])

As for TVP, no types are create by my code just the design time sql is changed:

SELECT myId, myName from @input

Becomes something like (off the top of my head):

DECLARE @SQLCOMMANDPROVIDER_input dbo.MyTableType = @input
SELECT myId, myName from @SQLCOMMANDPROVIDER_input

davidtme · 2018-05-18T14:25:55Z

I'm not sure how get travis-ci to build the project, maybe it's a mono/old f# core thing as it complaining about

The module/namespace 'System.Collections.Generic' from compilation unit 'mscorlib' did not contain the namespace, module or type 'IReadOnlyCollection`1'

but it builds on windows:

FSharp.Data.TVPTests.UsingTVPInQuery [SKIP]
   Fails at runtime :(

FSharp.Data.TVPTests.InputIsEnumeratedExactlyOnce [SKIP]
   Flucky

FSharp.Data.TypeProviderTest.ConcurrentReaders [SKIP]
   Thread safe execution is not supported yet

FSharp.Data.TypeProviderTest.CommandTimeout [SKIP]
   Don't execute for usual runs. Too slow.

144 total, 0 failed, 4 skipped, took 3.531 seconds
Finished Target: RunTests
Starting Target: All (==> RunTests)
Finished Target: All

---------------------------------------------------------------------
Build Time Report
---------------------------------------------------------------------
Target            Duration
------            --------
Clean             00:00:00.0390081
RestorePackages   00:00:03.1203044
AssemblyInfo      00:00:00.0088544
Build             00:00:12.4519448
DeployTestDB      00:00:00.2427771
BuildTests        00:00:09.0156047
RunTests          00:00:04.2975552
All               00:00:00.0000459
Total:            00:00:29.2397542
Status:           Ok
---------------------------------------------------------------------

vasily-kirichenko · 2018-05-18T14:38:55Z

I'm not sure how get travis-ci to build the project

Pin FAKE to version ~> 4.

vasily-kirichenko · 2018-05-18T14:50:15Z

OK, it does not help. The good news, I remembered the right way: reference FSharp.Core via NuGet. The version should be 4.0.0.1 I believe.

@smoothdeveloper what version should we reference?

vasily-kirichenko · 2018-05-18T14:58:37Z

@davidtme But we must test it in VS 2015 after such change.

davidtme · 2018-05-18T15:01:52Z

@vasily-kirichenko I did originally have 4.0.0.1 in there (a85d2b8) but I removed it as part of the "clean up" and was hoped merging from master would sort it out... never mind it's a fine line between fixing to much and creating clutter in a PR so it's all good. Sadly (not really) I no longer have VS2015 so I can't test that part.

davidtme · 2018-05-18T15:44:37Z

I've moved the build on a but

2) FS0039: /home/travis/build/fsprojects/FSharp.Data.SqlClient/src/SqlClient/ISqlCommand.fs(190,179): The type 'SqlDataRecord' is not defined in 'Microsoft.SqlServer.Server'.

Looks like this was in issue in the past #49 so maybe pinning to 4.0.0.1 has resurrected this some how?

vasily-kirichenko · 2018-05-18T16:53:44Z

@davidtme I've built it from master branch and looked at FSharp.Core.dll in bin folder:

Try 4.1.18

vasily-kirichenko · 2018-05-18T17:04:18Z

No, it does not work. It seems the only way to make it compilable on Mac is upgrading to .net 4.5. Do we want to do it right now?

I think it's ok to have green build on windows for now.

vasily-kirichenko · 2018-05-18T17:13:17Z

I've built nuget package and testing in VS 2017:

vasily-kirichenko · 2018-05-18T17:35:00Z

OK, I have the same error with package built from master as well o_O

smoothdeveloper · 2018-05-18T17:56:34Z

src/SqlClient/SqlCommandProvider.fs

-        let designTimeSqlStatement, tempTableTypes = 
-            let sql, types = DesignTime.SubstituteTempTables(conn, sqlStatement, tempTableDefinitions)
-            DesignTime.SubstituteTableVar(sql, tableVarMapping), types
+        let connectionId = Guid.NewGuid().ToString().Substring(0, 8)


Can we have explanation on why we need this? it seems to be used for the provided type name of the table, maybe better to pick something deterministic, or to remove it from here and internal only to the place it is used.

What would be the problem to just use the whole connection string instead?

or maybe the GetHashCode on the connection string, this way we can probably avoid passing a parameter and keep it as implementation detail of the substitution code.

In some cases parallel msbuild or msbuild and visual studio can both be building the type provider at the same time so you end up with both trying to create, use and drop the same temp table which creates build errors. Adding a Guid in there stops them from fighting.

davidtme · 2018-05-18T18:56:25Z

Build is green :D I'm sure there is a cleaner way but could be a quick work around.

smoothdeveloper

Thanks for adding nice tests, and the conditional to avoid the new code path if the extra parameters aren't given.

Some more extra changes should be removed before this go in, the only non related change we should keep:

fix for mono reflection when dealing with TVP
fix to build.sh

smoothdeveloper · 2018-05-18T19:17:05Z

src/SqlClient/DesignTime.fs

@@ -40,7 +41,52 @@ module internal SharedLogic =
            // add .Table
            returnType.Single |> cmdProvidedType.AddMember

-type DesignTime private() = 
+module Prefixes =


We might want to keep new members in this file internal.

smoothdeveloper · 2018-05-18T19:18:37Z

src/SqlClient/DesignTime.fs

+type TempTableLoader(fieldCount, items: obj seq) =
+    let enumerator = items.GetEnumerator()
+
+    interface IDataReader with


can we add a small note about why we have to implement our own datareader and why those few members are enough?

smoothdeveloper · 2018-05-18T19:21:30Z

src/SqlClient/SqlClient.fsproj

@@ -51,8 +51,8 @@
    <OtherFlags>--warnon:1182</OtherFlags>
  </PropertyGroup>
  <ItemGroup>
-    <Reference Include="FSharp.Core, Version=$(TargetFSharpCoreVersion), Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a">
-      <Private>True</Private>
+    <Reference Include="FSharp.Core">


for now we should revert changes to project files, you can keep them local and eventually we will make the solution work better outside of VS2015, but it is easier to know we don't break things without those changes.

smoothdeveloper · 2018-05-18T19:22:05Z

src/SqlClient/packages.config

@@ -1,5 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
+  <package id="FSharp.Core" version="4.0.0.1" targetFramework="net40" />


same here, let's put that aside for now, you can keep it in your local repository.

smoothdeveloper · 2018-05-18T19:22:31Z

src/SqlClient.Tests/app.config

@@ -7,7 +7,7 @@
    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
      <dependentAssembly>
        <assemblyIdentity name="FSharp.Core" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
-        <bindingRedirect oldVersion="0.0.0.0-4.4.0.0" newVersion="4.3.1.0" />
+        <bindingRedirect oldVersion="0.0.0.0-4.4.0.0" newVersion="4.4.0.0" />


we shouldn't touch this for now.

smoothdeveloper · 2018-05-18T19:27:16Z

@davidtme I think some of your changes for Mono are good but require more testing to verify we don't break existing runtime environments.

I'm working on changes to bring up paket in master, I hope to submit a working PR this weekend, this should make dealing with fsharp.core and binding redirects easier.

vasily-kirichenko · 2018-05-19T07:25:10Z

Closed-reopen it to kick AppVeyor.

vasily-kirichenko · 2018-05-20T11:50:17Z

I've merged master into your branch and opened new PR #302

davidoptima added 2 commits November 16, 2017 14:32

Add TempTableDefinitions and TableVarMapping

8a9bbff

try and get the build working

921ca59

davidtme changed the title ~~[WIP] Add TempTableDefinitions and TableVarMapping~~ Add TempTableDefinitions and TableVarMapping Nov 16, 2017

davidoptima added 6 commits November 16, 2017 16:05

Add config

a85d2b8

Add loadTempTablesMethod

635b365

Version bump

d344274

bool -> bit

69f6a8e

add binary

0f0112d

Add TempTableLoader

1962219

smoothdeveloper reviewed May 12, 2018

View reviewed changes

smoothdeveloper mentioned this pull request May 12, 2018

Temp Tables #277

Closed

smoothdeveloper mentioned this pull request May 16, 2018

FSharp.Data.SqlCommandProvider throws error if statement/proc uses temp table #55

Closed

davidoptima added 5 commits May 18, 2018 14:21

Clean up

d60ead7

Merge remote-tracking branch 'remotes/upstream/master' into temp-tables

a5b454f

# Conflicts: # RELEASE_NOTES.md # src/SqlClient/DesignTime.fs

Clean up whitespace changes

a749685

More white space

5ecc01a

Add missing file

0b90ac1

Fix more merge fall out

68b5fe0

pin fake in build.sh

9f6a4d5

davidoptima added 2 commits May 18, 2018 16:10

FSharp.Core.4.0.0.1

622834e

update bindingRedirect

73ea1fa

smoothdeveloper reviewed May 18, 2018

View reviewed changes

try get the mono build working

85e1839

smoothdeveloper requested changes May 18, 2018

View reviewed changes

smoothdeveloper referenced this pull request May 18, 2018

revert reflection

abbf31d

vasily-kirichenko closed this May 19, 2018

vasily-kirichenko reopened this May 19, 2018

vasily-kirichenko closed this May 20, 2018

smoothdeveloper mentioned this pull request Jun 20, 2018

ExecuteNonQuery requires an open and available connection #306

Open

This was referenced Dec 15, 2020

Add temp table bulk loading Zaid-Ajaj/DustyTables#17

Open

[WIP] Add temp table bulk loading cmeeren/Facil#2

Closed

Add TempTableDefinitions and TableVarMapping #278

Add TempTableDefinitions and TableVarMapping #278

Uh oh!

Conversation

davidtme commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smoothdeveloper commented May 11, 2018

Uh oh!

vasily-kirichenko commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtme commented May 16, 2018

Uh oh!

vasily-kirichenko commented May 16, 2018

Uh oh!

vasily-kirichenko commented May 17, 2018

Uh oh!

davidtme commented May 18, 2018

Uh oh!

davidtme commented May 18, 2018

Uh oh!

vasily-kirichenko commented May 18, 2018

Uh oh!

vasily-kirichenko commented May 18, 2018

Uh oh!

vasily-kirichenko commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidtme commented May 18, 2018

Uh oh!

davidtme commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasily-kirichenko commented May 18, 2018

Uh oh!

vasily-kirichenko commented May 18, 2018

Uh oh!

vasily-kirichenko commented May 18, 2018

Uh oh!

vasily-kirichenko commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtme commented May 18, 2018

Uh oh!

smoothdeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smoothdeveloper commented May 18, 2018

Uh oh!

vasily-kirichenko commented May 19, 2018

Uh oh!

vasily-kirichenko commented May 20, 2018

Uh oh!

Uh oh!

davidtme commented Nov 16, 2017 •

edited

Loading

vasily-kirichenko commented May 18, 2018 •

edited

Loading

davidtme commented May 18, 2018 •

edited

Loading