Add DataTable support to SqlBulkCopy. #16884

corivera · 2017-03-09T02:22:38Z

corivera · 2017-03-09T02:24:12Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

@@ -471,8 +471,6 @@ internal TdsParserStateObject GetSession(object owner)
            else
            {
                session = _physicalStateObj;
-                // But for CoreCLR we are now relying on the owner to ensure that cancellation owner comes from one source
-                session.Owner = owner;


This line was causing assert failures in the bulk copy tests. Removing it doesn't cause any test failures, and it's not present in the .NET Framework version of TdsParser.

@saurabh500 Thoughts?

Can you point me to the assert ?

https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs#L335

corivera · 2017-03-09T02:28:32Z

src/System.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/MissingTargetTable.cs

+                                bulkcopy.Close();
+                            }
+                        }
+                        catch (SqlException)


Missed these generic catches. I'll see if I can replace these with more specific asserts or message checking.

saurabh500 · 2017-03-09T02:51:47Z

src/System.Data.SqlClient/src/System/Data/Common/AdapterUtil.cs

@@ -827,10 +857,24 @@ internal static Exception TransactionZombied(DbTransaction obj)
        internal const float FailoverTimeoutStep = 0.08F;    // fraction of timeout to use for fast failover connections

        // security issue, don't rely upon public static readonly values - AS/URT 109635


Nit: You may want to remove these internal bug numbers

stephentoub · 2017-03-09T13:29:00Z

src/System.Data.SqlClient/src/System/Data/Common/AdapterUtil.cs

+            {
+                Debug.Assert(false, "valid CommandType " + value.ToString());
+            }
+#endif


Nit: could just be a Debug.Assert with the inverted condition rather than having Assert(false, ...) and using #if DEBUG.

I'm also a little confused by the condition being checked, as it differs from the condition being checked in ValidateCommandBehavior below; I'd have expected them to be identical.

Looks like InvalidCommandBehavior is only referenced by ValidateCommandBehavior, so I can just combine the two.

stephentoub · 2017-03-09T13:29:23Z

src/System.Data.SqlClient/src/System/Data/Common/AdapterUtil.cs

@@ -827,10 +857,24 @@ internal static Exception TransactionZombied(DbTransaction obj)
        internal const float FailoverTimeoutStep = 0.08F;    // fraction of timeout to use for fast failover connections

        // security issue, don't rely upon public static readonly values - AS/URT 109635
-        internal static readonly String StrEmpty = ""; // String.Empty
+        internal static readonly string StrEmpty = ""; // String.Empty


Why is this needed, rather than just using string.Empty?

I'll replace references to this field in a separate PR

stephentoub · 2017-03-09T13:30:35Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

+                                    Debug.Assert(_SqlDataReaderRowSource != null, "Should not be reading row as an XmlReader if bulk copy source is not a SqlDataReader");
+                                    return new XmlDataFeed(_SqlDataReaderRowSource.GetXmlReader(sourceOrdinal));
+                                default:
+                                    Debug.Assert(false, string.Format("Current column is marked as being a DataFeed, but no DataFeed compatible method was provided. Method: {0}", _currentRowMetadata[destRowIndex].Method));


Nit: Debug.Assert(false, ...) can instead be Debug.Fail(...)

(same applies to other uses like this throughout)

justinvp · 2017-03-09T14:15:07Z

src/System.Data.SqlClient/src/System/Data/Common/AdapterUtil.cs

+            if (null != mcd)
+            {
+                Delegate[] d = mcd.GetInvocationList();
+                for (int i = 0; i < d.Length; i++)


Nit: foreach (Delegate del in d)

justinvp · 2017-03-09T14:15:59Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

        }

-        // Unified method to read a value from the current row
+        // unified method to read a value from the current row


Nit: Why lowercase?

justinvp · 2017-03-09T14:20:37Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

+                    }
+                    resultTask = source.Task;
+                    return resultTask; // nothing to do. user passed us an empty array. Return a completed Task.
+                }


The TaskCompletionSource<T> allocation can be avoided and the above can be simplified as:

if (rows.Length == 0) { return cancellationToken.IsCancellationRequested ? Task.FromCanceled(cancellationToken) : Task.CompletedTask; }

justinvp · 2017-03-09T14:21:38Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlDataAdapter.cs

+            {
+                if (0 > value)
+                {
+                    throw ADP.ArgumentOutOfRange("UpdateBatchSize");


Nit: nameof(UpdateBatchSize)

saurabh500 · 2017-03-09T02:52:44Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

        }

-        // Unified method to read a value from the current row
+        // unified method to read a value from the current row


Nit: Capitalize unified.

saurabh500 · 2017-03-09T02:56:00Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

@@ -1440,12 +1800,57 @@ public Task WriteToServerAsync(DbDataReader reader, CancellationToken cancellati
            return resultTask;
        }

+        public Task WriteToServerAsync(DataTable table)


Nit: => WriteToServerAsync(table, 0, CancellationToken.None);

saurabh500 · 2017-03-09T02:56:17Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

+            return WriteToServerAsync(table, 0, CancellationToken.None);
+        }
+
+        public Task WriteToServerAsync(DataTable table, CancellationToken cancellationToken)


Nit: => WriteToServerAsync(table, 0, cancellationToken);

saurabh500 · 2017-03-09T02:56:30Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

+            return WriteToServerAsync(table, 0, cancellationToken);
+        }
+
+        public Task WriteToServerAsync(DataTable table, DataRowState rowState)


Nit: => WriteToServerAsync(table, rowState, CancellationToken.None);

saurabh500 · 2017-03-09T03:01:15Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

@@ -471,8 +471,6 @@ internal TdsParserStateObject GetSession(object owner)
            else
            {
                session = _physicalStateObj;
-                // But for CoreCLR we are now relying on the owner to ensure that cancellation owner comes from one source
-                session.Owner = owner;


Can you point me to the assert ?

saurabh500 · 2017-03-09T03:06:23Z

src/System.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/Bug84548.cs

+                dstConn.Open();
+                if (dstConn.ServerVersion.StartsWith("07"))
+                {
+                    // test skipped on 7.0 server


Which is Version 7.0 of Sql Server? Can we get rid of the condition? If its anything before 2008 R2, I think this can be removed.

7.0 is the version right before SQL 2000, so I'll get rid of this check.

saurabh500 · 2017-03-09T03:08:14Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlDataAdapter.cs

+
+namespace System.Data.SqlClient
+{
+    public sealed class SqlDataAdapter : DbDataAdapter, IDbDataAdapter, ICloneable


Why is the SqlDataAdapter here? Is this only being used in tests?

Yes, it's used in the tests. I only included the APIs that are relevant to these changes.

Do you think that SqlAdapter surface can be brought in completely so that the customers can use all the APIs?
You don't need to address this comment as part of this PR, but I think that you might as well drive this class to completion in case any APIs are left out.

saurabh500 · 2017-03-09T03:13:03Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

+            }
+        }
+
+        public void WriteToServer(IDataReader reader)


I don't see this in the contract. You should add this method in the ref assembly.

saurabh500 · 2017-03-09T20:02:08Z

Test Failures with

error CS2001: Source file '/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/src/System.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopySomeFromRowArray.cs' could not be found. [/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/src/System.Data.SqlClient/tests/ManualTests/System.Data.SqlClient.ManualTesting.Tests.csproj]

Did you miss a file in the commit?

saurabh500 · 2017-03-09T20:13:02Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

+
+                        IDataReader rowSourceAsIDataReader = (IDataReader)_rowSource;
+
+                        // Back-compat with 4.0 and 4.5 - only use IsDbNull when streaming is enabled and only for non-SqlDataReader


Does this comment still make sense?

saurabh500 · 2017-03-09T20:19:30Z

src/System.Data.SqlClient/ref/System.Data.SqlClient.cs

@@ -212,8 +212,17 @@ public sealed partial class SqlBulkCopy : System.IDisposable
        public void Close() { }
        void System.IDisposable.Dispose() { }
        public void WriteToServer(System.Data.Common.DbDataReader reader) { }
+        public void WriteToServer(System.Data.DataTable table) { }


These new APIs will need a version update to SqlClient ref assembly.
I am not sure how that mechanism is handled now. The versions used to reside in the csproj file.

@weshaggard What is the guidance here?

cc @stephentoub In case you can help with the information here?

corivera · 2017-03-09T20:45:44Z

@saurabh500 That file is present in the commit: https://github.com/corivera/corefx/blob/f96dcc07b555d58e268063adbf76f2c1dbc4bd2b/src/System.Data.SqlClient/tests/ManualTests/SQL/SqlBulkCopyTest/CopySomeFromRowarray.cs

I'll investigate the test failures.

justinvp · 2017-03-09T23:42:33Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopy.cs

+                    }
+                    resultTask = source.Task;
+                    return resultTask; // Nothing to do. user passed us an empty array. Return a completed Task.
+                }


Looks like my previous comment was missed... Any reason why the above couldn't be simplified as follows?

if (rows.Length == 0) { return cancellationToken.IsCancellationRequested ? Task.FromCanceled(cancellationToken) : Task.CompletedTask; }

Sorry, missed this. I'll fix it.

corivera · 2017-03-10T20:26:55Z

@saurabh500 Test failures were due to some uncapitalized filenames.

corivera · 2017-03-11T00:53:29Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsEnums.cs

-    /// <summary>
-    /// Column Encryption Setting to be used for the SqlCommand.
-    /// </summary>
-    public enum SqlCommandColumnEncryptionSetting


Removed since this wasn't actually being used anywhere. (originally introduced with an earlier commit in this PR)

corivera · 2017-03-15T00:04:29Z

@weshaggard @ericstj Do we need a version bump for SqlClient due to the contract changes here? We're adding in APIs already present in Framework SqlClient

weshaggard · 2017-03-15T00:08:54Z

@weshaggard @ericstj Do we need a version bump for SqlClient due to the contract changes here? We're adding in APIs already rpesent in Framework SqlClient

Given that this package will have a version that runs on .NET Framework (although just a facade) I do think we should bump the minor version of the assembly (and the package assuming that isn't already bumped). @ericstj is that your assessment as well?

saurabh500 · 2017-03-15T00:13:29Z

@weshaggard @ericstj Where is the version bump made for the libraries? The information used to be with the folder of the assemblies in the csproj files. But the version information doesn't exist there any more.

saurabh500 · 2017-03-15T00:14:52Z

@corivera I cant reply to my comments in the review above.

Can you point me to the assert ?

Is this assert still an issue?

corivera · 2017-03-15T00:16:23Z

@saurabh500 No, removing this line resolves the failing Assert. This line isn't found in Framework, either.
#16884 (diff)

weshaggard · 2017-03-15T16:43:39Z

@weshaggard @ericstj Where is the version bump made for the libraries? The information used to be with the folder of the assemblies in the csproj files. But the version information doesn't exist there any more.

https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/dir.props#L5 change that to 4.2.0.0

corivera · 2017-03-15T19:04:57Z

@weshaggard where do we update the version for the nuget package?

weshaggard · 2017-03-15T19:09:25Z

@weshaggard where do we update the version for the nuget package?

They were globally updated in master after the fork of our last release (https://github.com/dotnet/corefx/blob/master/Packaging.props#L23) so you don't need to update it.

danmoseley · 2017-03-16T00:01:39Z

D:\j\workspace\AllConfigurat---c1564d53\Tools\Packaging.targets(1090,5): error : PackageIndex from D:\j\workspace\AllConfigurat---c1564d53\pkg\Microsoft.Private.PackageBaseline\packageIndex.json is missing an assembly version entry for 4.2.0.0 for package System.Data.SqlClient.  Please run /t:UpdatePackageIndex on this project to commit an update. [D:\j\workspace\AllConfigurat---c1564d53\src\System.Data.SqlClient\pkg\System.Data.SqlClient.pkgproj]
D:\j\workspace\AllConfigurat---c1564d53\Tools\Packaging.targets(1090,5): error : PackageIndex from D:\j\workspace\AllConfigurat---c1564d53\pkg\Microsoft.Private.PackageBaseline\packageIndex.json is has an assembly version entry(s) for 4.1.2.0 which are no longer in package System.Data.SqlClient.  Please run /t:UpdatePackageIndex on this project to commit an update. [D:\j\workspace\AllConfigurat---c1564d53\src\System.Data.SqlClient\pkg\System.Data.SqlClient.pkgproj]
Command execution failed with exit code 1.

corivera · 2017-03-16T00:14:15Z

@danmosemsft Looking into it

saurabh500 · 2017-03-23T22:28:16Z

@corivera, As discussed I am OK with the removal of the Owner = owner statement. This is a bug and should be taken care of.
Open an issue to take of assigning the cancellation owner of TdsParserStateObject based on the investigation. The Cancel method should check who started the session with TdsParserStateObject and verify the cancellation is coming from the same Owner that started the session.

LGTM otherwise.

corivera · 2017-03-23T23:21:21Z

@saurabh500 https://github.com/dotnet/corefx/issues/17458

corivera · 2017-03-23T23:24:05Z

Work for adding remaining SqlDataAdapter APIs: https://github.com/dotnet/corefx/issues/17459

corivera added the area-System.Data.SqlClient label Mar 9, 2017

corivera requested review from saurabh500 and geleems March 9, 2017 02:22

dnfclas added the cla-already-signed label Mar 9, 2017

corivera commented Mar 9, 2017

View reviewed changes

saurabh500 reviewed Mar 9, 2017

View reviewed changes

stephentoub reviewed Mar 9, 2017

View reviewed changes

justinvp reviewed Mar 9, 2017

View reviewed changes

karelz assigned corivera Mar 9, 2017

saurabh500 reviewed Mar 9, 2017

View reviewed changes

justinvp reviewed Mar 9, 2017

View reviewed changes

corivera requested a review from saurabh500 March 10, 2017 22:30

corivera commented Mar 11, 2017

View reviewed changes

saurabh500 added this to the 2.0.0 milestone Mar 21, 2017

Add DataTable support to SqlBulkCopy. Also adds tests for SqlBulkCopy.

c4a3d52

saurabh500 approved these changes Mar 23, 2017

View reviewed changes

corivera merged commit 8e3f9ac into dotnet:master Mar 23, 2017

corivera deleted the AddDataTableToBulkCopy branch March 23, 2017 23:24

		@@ -827,10 +857,24 @@ internal static Exception TransactionZombied(DbTransaction obj)
		internal const float FailoverTimeoutStep = 0.08F; // fraction of timeout to use for fast failover connections

		// security issue, don't rely upon public static readonly values - AS/URT 109635


		IDataReader rowSourceAsIDataReader = (IDataReader)_rowSource;

		// Back-compat with 4.0 and 4.5 - only use IsDbNull when streaming is enabled and only for non-SqlDataReader

Add DataTable support to SqlBulkCopy. #16884

Add DataTable support to SqlBulkCopy. #16884

Conversation

corivera commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corivera Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corivera commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corivera commented Mar 10, 2017

corivera Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

corivera commented Mar 15, 2017 • edited Loading

weshaggard commented Mar 15, 2017

saurabh500 commented Mar 15, 2017

saurabh500 commented Mar 15, 2017

corivera commented Mar 15, 2017

weshaggard commented Mar 15, 2017

corivera commented Mar 15, 2017 • edited Loading

weshaggard commented Mar 15, 2017

danmoseley commented Mar 16, 2017

corivera commented Mar 16, 2017

saurabh500 commented Mar 23, 2017

corivera commented Mar 23, 2017

corivera commented Mar 23, 2017

corivera Mar 9, 2017 •

edited

Loading

corivera Mar 11, 2017 •

edited

Loading

corivera commented Mar 15, 2017 •

edited

Loading

corivera commented Mar 15, 2017 •

edited

Loading