Tidy up Bulk Copy unmatched column name work #3205

paulmedynski · 2025-03-06T19:05:59Z

Simplified the tracking of unmatched columns.
Cleaned up some fragile flags and loops.

The diffs look worse than they are. Disabling whitespace helps.

- Simplified the tracking of unmatched columns. - Cleaned up some fragile flags and loops.

- Simplified the tracking of unmatched columns for NetFX. - Cleaned up some fragile flags and loops for NetFX.

- Fixed weird indentation.

PR Overview

This PR tidies up the unmatched column name handling in Bulk Copy by simplifying the tracking of unmatched columns and cleaning up fragile flags and loops. Key changes include renaming and refactoring the transaction check variable, replacing a dictionary with a HashSet to track unmatched columns, and updating error method names for consistency.

Reviewed Changes

File	Description
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	Refactored transaction flag naming and column matching logic with a new HashSet-based approach
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	Similar refactoring of transaction and column matching logic for .NET Core
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs	Updated method overload names for non‐matching column errors for consistency

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:586

[nitpick] Consider initializing 'unmatchedColumns' with an initial capacity (e.g. _localColumnMappings.Count) for consistency with the .NET Core version and minor performance improvement.

HashSet<string> unmatchedColumns = new();

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs:1260

[nitpick] Ensure that the rename from 'BulkLoadNonMatchingColumnName' to 'BulkLoadNonMatchingColumnNames' aligns with the rest of the codebase to maintain consistency.

internal static Exception BulkLoadNonMatchingColumnNames(IEnumerable<string> columnNames)

paulmedynski · 2025-03-06T19:08:06Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

-		    {
-                        columnMappingStatusLookup.Add(_localColumnMappings[assocId].DestinationColumn, false);
-		    }
+                    // Are we missing a mapping between the result column and


We should prefer early returns/breaks to avoid huge conditional blocks. I flipped this conditional around and outdented the remaining code in this loop.

paulmedynski · 2025-03-06T19:09:44Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

@@ -532,207 +532,219 @@ private string AnalyzeTargetAndCreateUpdateBulkCommand(BulkCopySimpleResultSet i

            string[] parts = MultipartIdentifier.ParseMultipartIdentifier(DestinationTableName, "[\"", "]\"", Strings.SQL_BulkCopyDestinationTableName, true);
            updateBulkCommandText.AppendFormat("insert bulk {0} (", ADP.BuildMultiPartName(parts));
-            int nmatched = 0;  // Number of columns that match and are accepted


I replaced these counters with flags.

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

codecov · 2025-03-06T22:57:10Z

Codecov Report

Attention: Patch coverage is 88.81119% with 16 lines in your changes missing coverage. Please review.

Project coverage is 72.58%. Comparing base (3f4e486) to head (253cbb2).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...etcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	88.57%	8 Missing ⚠️
.../netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs	88.88%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3205      +/-   ##
==========================================
- Coverage   72.89%   72.58%   -0.32%     
==========================================
  Files         288      288              
  Lines       59246    59546     +300     
==========================================
+ Hits        43190    43220      +30     
- Misses      16056    16326     +270

Flag	Coverage Δ
addons	`92.58% <ø> (ø)`
netcore	`75.12% <88.73%> (-0.39%)`	⬇️
netfx	`71.18% <89.04%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

benrr101

I appreciate adding the comments to explain what's happening 👍 Overall looks good, couple stylistic changes, but nothing huge.

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

paulmedynski · 2025-03-07T12:02:45Z

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs

+
+            // Start by assuming all locally mapped Destination columns will be
+            // unmatched.
+            for (int i = 0; i < _localColumnMappings.Count; ++i)


This loop is necessary due to the overall nature of the matching algorithm below, which I didn't want to completely re-work. The number of mapped columns is expected to be small, and this operation happens once per bulk copy.

- Addressed review comments.

- Fixed SNI reflection failures when running in NetFX.

paulmedynski

One bulk copy test was failing on .NET Framework, which resulted in some unrelated changes to test helpers playing fast and loose with reflection.

paulmedynski · 2025-03-07T14:05:07Z

src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/Common/InternalConnectionWrapper.cs

-            Type sniHandleType = systemDotData.GetType("Microsoft.Data.SqlClient.SNI.SNIHandle");
-            MethodInfo killConn = sniHandleType.GetMethod("KillConnection");
+            Assembly assembly = Assembly.Load(new AssemblyName(typeof(SqlConnection).GetTypeInfo().Assembly.FullName));
+            Type sniHandleType = assembly.GetType("Microsoft.Data.SqlClient.SNI.SNIHandle");


This class doesn't exist in the .NET Framework assembly, so we need to add null checks.

paulmedynski · 2025-03-07T14:06:41Z

...ata.SqlClient/tests/ManualTests/SQL/Common/SystemDataInternals/TdsParserStateObjectHelper.cs

-        private static readonly FieldInfo s_forceSyncOverAsyncAfterFirstPend = s_tdsParserStateObject.GetField("s_forceSyncOverAsyncAfterFirstPend", BindingFlags.Static | BindingFlags.NonPublic);
-        private static readonly FieldInfo s_failAsyncPends = s_tdsParserStateObject.GetField("s_failAsyncPends", BindingFlags.Static | BindingFlags.NonPublic);
-        private static readonly FieldInfo s_forcePendingReadsToWaitForUser = s_tdsParserStateObject.GetField("s_forcePendingReadsToWaitForUser", BindingFlags.Static | BindingFlags.NonPublic);
-        private static readonly Type s_tdsParserStateObjectManaged = s_systemDotData.GetType("Microsoft.Data.SqlClient.SNI.TdsParserStateObjectManaged");


This line was throwing on .NET Framework:

TypeInitializationException : The type initializer for 'Microsoft.Data.SqlClient.ManualTesting.Tests.SystemDataInternals.TdsParserStateObject Helper' threw an exception. ---- System.NullReferenceException : Object reference not set to an instance of an object.

Unfortunately, exceptions in static initialization don't seem to indicate the file/line number, so I had to add asserts to figure out where the problem occurs.

paulmedynski added 3 commits March 6, 2025 13:42

User Story 34145: Tidy up Bulk Copy unmatched column name work

a7d8a92

- Simplified the tracking of unmatched columns. - Cleaned up some fragile flags and loops.

User Story 34145: Tidy up Bulk Copy unmatched column name work

941cbe9

- Simplified the tracking of unmatched columns for NetFX. - Cleaned up some fragile flags and loops for NetFX.

User Story 34145: Tidy up Bulk Copy unmatched column name work

eac2cd9

- Fixed weird indentation.

paulmedynski requested review from a team and Copilot March 6, 2025 19:05

Copilot AI reviewed Mar 6, 2025

View reviewed changes

paulmedynski commented Mar 6, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs Show resolved Hide resolved

benrr101 approved these changes Mar 7, 2025

View reviewed changes

paulmedynski commented Mar 7, 2025

View reviewed changes

paulmedynski added 2 commits March 7, 2025 09:08

User Story 34145: Tidy up Bulk Copy unmatched column name work

309faaf

- Addressed review comments.

User Story 34145: Tidy up Bulk Copy unmatched column name work

253cbb2

- Fixed SNI reflection failures when running in NetFX.

paulmedynski commented Mar 7, 2025

View reviewed changes

paulmedynski added this to the 7.0-preview1 milestone Mar 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy up Bulk Copy unmatched column name work #3205

Tidy up Bulk Copy unmatched column name work #3205

paulmedynski commented Mar 6, 2025 •

edited

Loading

paulmedynski Mar 6, 2025

paulmedynski Mar 6, 2025

codecov bot commented Mar 6, 2025 •

edited

Loading

benrr101 left a comment

paulmedynski Mar 7, 2025

paulmedynski left a comment

paulmedynski Mar 7, 2025

paulmedynski Mar 7, 2025

Tidy up Bulk Copy unmatched column name work #3205

Are you sure you want to change the base?

Tidy up Bulk Copy unmatched column name work #3205

Conversation

paulmedynski commented Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

paulmedynski Mar 6, 2025

Choose a reason for hiding this comment

paulmedynski Mar 6, 2025

Choose a reason for hiding this comment

codecov bot commented Mar 6, 2025 • edited Loading

Codecov Report

benrr101 left a comment

Choose a reason for hiding this comment

paulmedynski Mar 7, 2025

Choose a reason for hiding this comment

paulmedynski left a comment

Choose a reason for hiding this comment

paulmedynski Mar 7, 2025

Choose a reason for hiding this comment

paulmedynski Mar 7, 2025

Choose a reason for hiding this comment

paulmedynski commented Mar 6, 2025 •

edited

Loading

codecov bot commented Mar 6, 2025 •

edited

Loading