Translate COALESCE as ISNULL in more cases#35986
Draft
ranma42 wants to merge 3 commits into
Draft
Conversation
ranma42
force-pushed
the
isnull-more
branch
4 times, most recently
from
5b4b67b to
4f4a019
Compare
Contributor
Author
|
Some additional notes: I tried to implement all of the suggestions by @roji and indeed they proved quite effective; OTOH the tests I added unveiled some additional limitations risks, which I mitigated with an approach the codebase is already taking elsewhere (basically widening the type).
Specifically the changes I implemented include:
The conversion now applies to all instances of
Even though it passes the testsuite, I think it might be a good idea to make it possible for users to opt-out, as I am afraid it might break some corner cases (especially around UDF and/or custom converters). |
roji
force-pushed
the
main
branch
2 times, most recently
from
249ae47 to
6b86657
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends SQL Server query translation to emit ISNULL instead of COALESCE in more scenarios (continuing #34171) to avoid repeated evaluation, while adjusting expected SQL baselines and adding coverage for type-mapping corner cases.
Changes:
- Expand SQL Server COALESCE translation to nested
ISNULLwith type-mapping/size adjustments. - Update a wide set of SQL Server functional test baselines to match the new
ISNULL(...)SQL (often with explicit casts). - Add new Northwind tests to cover string type-mapping correctness with conditional expressions and
string.Join, including Cosmos baseline updates.
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.SqlServer.FunctionalTests/Query/Translations/StringTranslationsSqlServerTest.cs | Updates expected SQL to ISNULL (notably around STRING_AGG) with casts. |
| test/EFCore.SqlServer.FunctionalTests/Query/Translations/Operators/MiscellaneousOperatorTranslationsSqlServerTest.cs | Updates COALESCE baseline to ISNULL for string coalesce. |
| test/EFCore.SqlServer.FunctionalTests/Query/TPTManyToManyQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/TPTManyToManyNoTrackingQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL (no-tracking variant). |
| test/EFCore.SqlServer.FunctionalTests/Query/TPTGearsOfWarQuerySqlServerTest.cs | Broad baseline updates from COALESCE→ISNULL, including string casts and subqueries. |
| test/EFCore.SqlServer.FunctionalTests/Query/TPCManyToManyQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/TPCManyToManyNoTrackingQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL (no-tracking variant). |
| test/EFCore.SqlServer.FunctionalTests/Query/TPCGearsOfWarQuerySqlServerTest.cs | Broad baseline updates from COALESCE→ISNULL (TPC variant). |
| test/EFCore.SqlServer.FunctionalTests/Query/TemporalOwnedQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL in temporal queries. |
| test/EFCore.SqlServer.FunctionalTests/Query/TemporalManyToManyQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL in temporal many-to-many queries. |
| test/EFCore.SqlServer.FunctionalTests/Query/TemporalGearsOfWarQuerySqlServerTest.cs | Broad baseline updates from COALESCE→ISNULL in temporal queries. |
| test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerTest.cs | Updates scalar-subquery COALESCE baselines to ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServer160Test.cs | Same as above for SQL Server 16.0 test variant. |
| test/EFCore.SqlServer.FunctionalTests/Query/PrecompiledQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL, including STRING_AGG and UPDATE SET expressions. |
| test/EFCore.SqlServer.FunctionalTests/Query/OwnedQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for owned types. |
| test/EFCore.SqlServer.FunctionalTests/Query/OwnedEntityQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for owned entities. |
| test/EFCore.SqlServer.FunctionalTests/Query/NullSemanticsQuerySqlServerTest.cs | Updates COALESCE baselines to nested ISNULL in null-semantics tests. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSelectQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL in various Northwind select scenarios. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindNavigationsQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL in navigation subqueries. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindMiscellaneousQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL and adds new type-mapping coverage baselines. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindJoinQuerySqlServerTest.cs | Updates string concatenation COALESCE baselines to ISNULL with casts. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindGroupByQuerySqlServerTest.cs | Updates many SUM/COALESCE baselines to ISNULL across group-by tests. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL in function/subquery aggregates. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindFunctionsQuerySqlServer160Test.cs | Same as above for SQL Server 16.0 test variant. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL for ISDATE/ISNUMERIC scenarios with casts. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindDbFunctionsQuerySqlServer160Test.cs | Same as above for SQL Server 16.0 test variant. |
| test/EFCore.SqlServer.FunctionalTests/Query/NorthwindAggregateOperatorsQuerySqlServerTest.cs | Updates aggregate operator COALESCE baselines to ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/ManyToManyQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/ManyToManyNoTrackingQuerySqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL (no-tracking variant). |
| test/EFCore.SqlServer.FunctionalTests/Query/GearsOfWarQuerySqlServerTest.cs | Broad baseline updates from COALESCE→ISNULL in GearsOfWar queries. |
| test/EFCore.SqlServer.FunctionalTests/Query/Ef6GroupBySqlServerTest.cs | Updates group-by COALESCE baselines to ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsSharedTypeQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL in complex navigation/shared-type tests. |
| test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsSharedTypeQuerySqlServer160Test.cs | Same as above for SQL Server 16.0 test variant. |
| test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL in complex navigation tests. |
| test/EFCore.SqlServer.FunctionalTests/Query/ComplexNavigationsQuerySqlServer160Test.cs | Same as above for SQL Server 16.0 test variant. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/OwnedTableSplitting/OwnedTableSplittingPrimitiveCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL in owned/table-splitting primitive collection tests. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/OwnedNavigations/OwnedNavigationsSetOperationsSqlServerTest.cs | Updates nested aggregate COALESCE baselines to ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/OwnedNavigations/OwnedNavigationsPrimitiveCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for owned navigations primitive collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/OwnedNavigations/OwnedNavigationsCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for owned navigations collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/OwnedJson/OwnedJsonPrimitiveCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for owned JSON primitive collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/OwnedJson/OwnedJsonCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for owned JSON collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/Navigations/NavigationsSetOperationsSqlServerTest.cs | Updates nested aggregate COALESCE baselines to ISNULL for non-owned navigations. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/Navigations/NavigationsPrimitiveCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for navigations primitive collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/Navigations/NavigationsCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for navigations collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/ComplexTableSplitting/ComplexTableSplittingPrimitiveCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for complex table splitting primitive collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/ComplexJson/ComplexJsonSetOperationsSqlServerTest.cs | Updates nested aggregate COALESCE baselines to ISNULL for complex JSON set-ops (non-JSON-type path). |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/ComplexJson/ComplexJsonPrimitiveCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for complex JSON primitive collections. |
| test/EFCore.SqlServer.FunctionalTests/Query/Associations/ComplexJson/ComplexJsonCollectionSqlServerTest.cs | Updates SUM/COALESCE baselines to ISNULL for complex JSON collections (non-JSON-type path). |
| test/EFCore.SqlServer.FunctionalTests/Query/AdHocNavigationsQuerySqlServerTest.cs | Updates nested COALESCE baseline to nested ISNULL. |
| test/EFCore.SqlServer.FunctionalTests/Query/AdHocMiscellaneousQuerySqlServerTest.cs | Updates COALESCE baselines to ISNULL in ad-hoc query scenarios. |
| test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NorthwindBulkUpdatesSqlServerTest.cs | Updates COALESCE baselines to ISNULL for UPDATE setters. |
| test/EFCore.SqlServer.FunctionalTests/BulkUpdates/NonSharedModelBulkUpdatesSqlServerTest.cs | Updates COALESCE baselines to ISNULL for bulk update SQL. |
| test/EFCore.Specification.Tests/Query/NorthwindMiscellaneousQueryTestBase.cs | Adds new test cases to exercise type-mapping correctness around coalesce with conditional/string join patterns. |
| test/EFCore.Cosmos.FunctionalTests/Query/NorthwindMiscellaneousQueryCosmosTest.cs | Adds Cosmos overrides/baselines for the new tests and updates a display-class index in an exception message baseline. |
| src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs | Implements expanded COALESCE→ISNULL SQL generation with type-mapping adjustments. |
Comment on lines
+275
to
+288
| var typeMapping = sqlFunctionExpression.TypeMapping switch | ||
| { | ||
| { StoreTypeNameBase: "nvarchar", Size: >= 0 and < 4000 } => _typeMappingSource.FindMapping( | ||
| typeof(string), | ||
| sqlFunctionExpression.TypeMapping.StoreTypeNameBase, | ||
| unicode: true, | ||
| size: 4000), | ||
| { StoreTypeNameBase: "varchar" or "varbinary", Size: >= 0 and < 8000 } => _typeMappingSource.FindMapping( | ||
| typeof(string), | ||
| sqlFunctionExpression.TypeMapping.StoreTypeNameBase, | ||
| unicode: false, | ||
| size: 8000), | ||
| var t => t, | ||
| }; |
Comment on lines
+272
to
+273
| var forceCast = sqlFunctionExpression.TypeMapping?.StoreTypeNameBase is | ||
| "nvarchar" or "varchar" or "varbinary"; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a continuation of #34171 that extends the conversion to handle all of the cases (and adds a couple of tests for corner-cases).