-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Handle primitive collections as multiple parameters. #36157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4f67357 to
b878f5d
Compare
src/EFCore.Relational/Internal/ParameterizedCollectionTranslationMode.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends EF Core to support translating primitive collections into multiple SQL parameters instead of a single parameter or JSON, driven by a new ParameterizedCollectionTranslationMode.
- Changed parameter value storage and method signatures from
IReadOnlyDictionary<string, object?>to mutableDictionary<string, object?>. - Introduced
ParameterizedCollectionTranslationModeenum and threaded it through SQL processors, visitors, and the DbContext options builder. - Updated SQL generation in SQLite for multi-row
VALUES(wrapping in a sub-SELECT) and enhanced JSON/OPENJSON logic for SQL Server.
Reviewed Changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/EFCore/Query/Internal/IParameterValues.cs | Changed interface property to return mutable Dictionary. |
| src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs | Updated Parameters.ParameterValues to Dictionary. |
| src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs | Added TryTranslate helper and new JSON index translation. |
| src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs | Wrapped multi-row VALUES in SELECT * FROM (…). |
| src/EFCore.Relational/Internal/ParameterizedCollectionTranslationMode.cs | Added new enum member ParameterizeExpanded. |
| src/EFCore.Relational/Infrastructure/RelationalDbContextOptionsBuilder.cs | Added TranslateParameterizedCollectionsToExpandedParameters option. |
Multiple files across SqlNullabilityProcessor, SQL Server/SQLite processors and factories |
Threaded new mode through constructors and method signatures. |
Comments suppressed due to low confidence (2)
src/EFCore.Relational/Query/SqlNullabilityProcessor.cs:123
- [nitpick] This
VisitExtensionoverride now handles multiple translation modes with nested logic; consider extracting each mode case into its own helper method to improve readability and reduce method complexity.
protected override Expression VisitExtension(Expression node)
src/EFCore.Relational/Infrastructure/RelationalDbContextOptionsBuilder.cs:226
- New builder option for expanded parameters was added; ensure there are unit or integration tests covering this mode to verify correct SQL translation.
public virtual TBuilder TranslateParameterizedCollectionsToExpandedParameters()
eed2aa8 to
0be8385
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this, thanks @cincuranet. Let's iterate and get it to the finish line.
Of course OK to split things out to later PRs, we can discusss.
src/EFCore.Relational/Infrastructure/RelationalDbContextOptionsBuilder.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Infrastructure/RelationalDbContextOptionsBuilder.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Infrastructure/RelationalDbContextOptionsBuilder.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Internal/ParameterizedCollectionTranslationMode.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalParameterBasedSqlProcessorParameters.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
d4def1b to
3991a1e
Compare
src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/NorthwindCompiledQuerySqlServerTest.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really close now, see essentially nits and doc comments.
src/EFCore.SqlServer/Query/Internal/SqlServerJsonPostprocessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
...Relational.Specification.Tests/Query/NonSharedPrimitiveCollectionsQueryRelationalTestBase.cs
Outdated
Show resolved
Hide resolved
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerJsonTypeTest.cs
Show resolved
Hide resolved
|
FYI the Helix failure is (at the moment) expected. It depends on #36228. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, job well done @cincuranet. See remaining batch of really minor nits, feel free to merge after fixing without an additional review.
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs
Outdated
Show resolved
Hide resolved
927ea42 to
a33777d
Compare
Contributes to #36101.
Closes #34347.