[Perf] Bypass SqlBulkCopy Graph Column Mapping if Not Copying Graph Tables - #4535
[Perf] Bypass SqlBulkCopy Graph Column Mapping if Not Copying Graph Tables#4535benrr101 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves SqlBulkCopy performance by avoiding SQL Graph column-alias resolution work (temp table creation + sys catalog scans + extra resultset parsing) when the bulk copy operation isn’t using SQL Graph alias destination columns ($edge_id, $to_id, $from_id, $node_id). This targets a regression observed in perf suites where bulk-copy setup/teardown dominated runtime.
Changes:
- Conditionally generate/execute the
#Column_Aliasestemp table + population queries only when destination column mappings reference SQL Graph alias names. - Make cached-metadata reuse aware of whether the column-alias resultset is required/present, and guard alias-application logic on resultset presence.
- Update ManualTests statistics expectations to reflect the reduced query/DDL/DML work in the common non-graph pathway.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/ManualTests/BulkCopy/CopyAllFromReader.cs | Updates expected SqlConnection statistics counters to match the optimized non-graph bulk copy behavior. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs | Bypasses graph alias temp-table/query generation unless needed; adds result-count checks for cached metadata and alias resultset handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:156
- The comment implies the initial query may return the column-alias result set based on destination table shape, but the new behavior is actually conditional on whether column mappings reference SQL Graph alias names (see ShouldResolveColumnAliases()). Clarifying this prevents confusion when reading stats/result-set expectations.
// The initial query will return three tables, and may return a fourth for column aliases.
|
I think test failures are related to changes here, please take a look. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4535 +/- ##
==========================================
- Coverage 64.78% 62.85% -1.93%
==========================================
Files 288 283 -5
Lines 44418 67456 +23038
==========================================
+ Hits 28774 42398 +13624
- Misses 15644 25058 +9414
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Check if we have valid cached metadata for the current destination table | ||
| if (CachedMetadata != null) | ||
| if (CachedMetadata != null | ||
| && (!ShouldResolveColumnAliases() || CachedMetadata.Count > ColumnAliasesResultId)) |
There was a problem hiding this comment.
ShouldResolveColumnAliases() is evaluated here and again in CreateInitialQuery,
walking the mapping collection each time. Beyond the (minor) duplicated work, the bigger
concern is that these two call sites must agree — if they ever observe different mapping
state, the cache-validity check and the actual query shape diverge, and
AnalyzeTargetAndCreateUpdateBulkCommand silently skips alias resolution.
We should consider computing it once per operation (e.g. in WriteRowSourceToServerCommon, right
after _localColumnMappings is finalized) and storing it in a field, so the query shape
and the cache check are guaranteed to be derived from the same evaluation.
| _localColumnMappings.ValidateCollection(); | ||
| foreach (SqlBulkCopyColumnMapping bulkCopyColumn in _localColumnMappings) | ||
| { | ||
| bulkCopyColumn.MappedDestinationColumn = null; |
There was a problem hiding this comment.
This reset is inside a loop that breaks early, so mappings after the first one with
_internalSourceColumnOrdinal == -1 never get cleared.
_internalSourceColumnOrdinal is assigned during WriteRowSourceToServerCommon and is
never reset to -1 afterwards, and ColumnMappings.ReadOnly goes back to false at the
end of the operation — so a caller can mutate mappings and reuse the instance. If they
set SourceColumn on an early mapping (which resets that ordinal to -1), the loop breaks
before reaching a later graph-alias mapping, and it keeps the stale
MappedDestinationColumn resolved against the previous destination table.
Suggest hoisting it into its own unconditional pass before the ordinal scan:
foreach (SqlBulkCopyColumnMapping bulkCopyColumn in _localColumnMappings)
{
bulkCopyColumn.MappedDestinationColumn = null;
}
foreach (SqlBulkCopyColumnMapping bulkCopyColumn in _localColumnMappings)
{
if (bulkCopyColumn._internalSourceColumnOrdinal == -1)
{
unspecifiedColumnOrdinals = true;
break;
}
}
| query.Append(typeName); | ||
| } | ||
|
|
||
| private bool ShouldResolveColumnAliases() |
There was a problem hiding this comment.
Could we get direct coverage for the bypass itself? The CopyAllFromReader stat changes
verify it only indirectly. Three cases worth pinning down:
- Copying to a real graph table by ordinal (no alias in
ColumnMappings) — confirms the
bypass doesn't regress Feature | Support SQL Graph column aliases in SqlBulkCopy #3677 when the alias names never appear in the mappings. - Reusing one
SqlBulkCopyacross twoWriteToServercalls, first with an alias mapping
and then without (and vice versa) — covers theMappedDestinationColumnreset. SqlBulkCopyOptions.CacheMetadatacombined with alias mappings — exercises the new
CachedMetadata.Count > ColumnAliasesResultIdguard in both directions.
| string objectName = ADP.BuildMultiPartName(parts); | ||
| string escapedObjectName = SqlServerEscapeHelper.EscapeStringAsLiteral(objectName); | ||
| string catalogNameStringLiteral = CatalogName is null ? null : SqlServerEscapeHelper.EscapeStringAsLiteral(CatalogName); | ||
| bool resolveColumnAliases = ShouldResolveColumnAliases(); |
There was a problem hiding this comment.
Nit: when the fragments collapse to string.Empty, the interpolation holes leave stray
blank lines in the generated batch. Harmless for the server, but it makes the
TryTraceEvent dump of the initial query noisier when diagnosing bulk copy issues.
edwardneal
left a comment
There was a problem hiding this comment.
A few comments - one nit and one slightly more substantial.
Do you have a before/after benchmark? I agree that making a change to the SQL statement would improve performance, but if something isn't a Graph table then the result set should have zero rows and have no direct performance impact.
| string objectName = ADP.BuildMultiPartName(parts); | ||
| string escapedObjectName = SqlServerEscapeHelper.EscapeStringAsLiteral(objectName); | ||
| string catalogNameStringLiteral = CatalogName is null ? null : SqlServerEscapeHelper.EscapeStringAsLiteral(CatalogName); | ||
| bool resolveColumnAliases = ShouldResolveColumnAliases(); |
There was a problem hiding this comment.
One point to bear in mind here is that ShouldResolveColumnAliases is only a heuristic. Only SQL Server truly knows whether a table is a Graph table. Any table (whether a normal table or a Graph table) can contain [$edge_id] / etc. columns. This is fine for now, but it doesn't (and can't) provide an absolute guarantee from the client.
| EXEC sp_executesql N' | ||
| INSERT INTO #Column_Aliases ([Canonical_Column_Name], [Canonical_Column_Id], [Aliased_Column_Name]) | ||
| SELECT [name], [column_id], ''$to_id'' FROM {catalogNameStringLiteral}.[sys].[all_columns] WHERE [object_id] = @Object_ID AND COALESCE([graph_type], 0) = 8 | ||
| UNION ALL | ||
| SELECT [name], [column_id], ''$from_id'' FROM {catalogNameStringLiteral}.[sys].[all_columns] WHERE [object_id] = @Object_ID AND COALESCE([graph_type], 0) = 5 | ||
| UNION ALL | ||
| SELECT [name], [column_id], ''$edge_id'' FROM {catalogNameStringLiteral}.[sys].[all_columns] WHERE [object_id] = @Object_ID AND COALESCE([graph_type], 0) = 2 AND [name] LIKE ''$edge[_]id[_]%'' | ||
| UNION ALL | ||
| SELECT [name], [column_id], ''$node_id'' FROM {catalogNameStringLiteral}.[sys].[all_columns] WHERE [object_id] = @Object_ID AND COALESCE([graph_type], 0) = 2 AND [name] LIKE ''$node[_]id[_]%''', | ||
| N'@Object_ID INT', @Object_ID = @Object_ID |
There was a problem hiding this comment.
Is there a reason why we can't simply remove this specific statement from the executed SQL command if the client-side heuristic fails? This means that the final result set always appears (and just doesn't contain a value). We keep the heuristic logic in one place and don't need to consider the downstream impacts - they already handle a zero-length result set.
Description
This addresses a performance regression in the DataTypeReader / Async perf suites, indirectly.
In #3677, SQL Graph column alias mapping support was added. While this works great for SQL Graph tables, it was a bit over eager and invoked the entire graph table mapping behavior even when the tables involved did not contain SQL Graph tables. Thus, every bulk copy implicitly generated mapping tables, etc. This dramatically decreased perf in scenarios where simple tables were being bulk copied.
To resolve this, we introduce a mechanism to bypass the graph column alias table generation when the table does not contain any of the graph alias columns (
$edge_id,$to_id,$from_id,$node_id). This returns performance in the DataTypeReader/Async perf suites to pre-#3677 levels.It is worth nothing that this was discovered as an artifact of the way the DataTypeReader/Async perf suites are implemented. Although the suite aims to verify perf of reading various data types from a SqlDataReader, majority of the time spent running the test is spent doing setup and teardown steps - ie, SqlBulkCopy of data into the test table. So although this change doesn't actually impact the SqlDataReader performance, it does resolve the perf suite regression, and improve perf of SqlBulkCopy in the most common pathways.
🤖
Issues
N/A
Testing
Local comparison of perf suite before and after these changes suggest a significant improvement.