Support primitive collections by roji · Pull Request #30738 · dotnet/efcore

roji · 2023-04-20T18:38:33Z

Implements the bulk of #30731

Closes #29427
Closes #30426
Closes #13617
Closes #30163

roji · 2023-04-20T18:40:31Z

src/EFCore.Relational/Query/QuerySqlGenerator.cs

        // data twice.
        // Note that if the type mapping differs, we do send the same data twice (e.g. the same string may be sent once as Unicode, once as
        // non-Unicode).
+        // TODO: Note that we perform Equals comparison on the value converter. We should be able to do reference comparison, but for


@ajcvickers FYI, this is #30677

roji · 2023-04-20T18:42:56Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

+    /// </summary>
+    /// <param name="selectExpression">The <see cref="SelectExpression" /> to check for ordering.</param>
+    /// <returns>Whether <paramref name="selectExpression"/> is ordered.</returns>
+    protected virtual bool IsOrdered(SelectExpression selectExpression)


Added this for PostgreSQL, where the results of unnest are guaranteed to be ordered without any explicit ORDER BY; this allows us to avoid the warning for Skip/Take over unordered collection.

roji · 2023-04-20T18:44:44Z

src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs

        }
    }
-
-    private sealed class SqlTypeMappingVerifyingExpressionVisitor : ExpressionVisitor


Note that this verification - that all SqlExpressions have a mapping - has been moved from the end of each individual SQL translations (of which there are potentially several in the query) to the end of the entire query. This is necessary since some mappings are inferred later.

roji · 2023-04-20T18:46:12Z

src/EFCore.Relational/Query/SqlExpressions/RowValueExpression.cs

+///         not used in application code.
+///     </para>
+/// </summary>
+public class RowValueExpression : SqlExpression


This will also provide infrastructure for other uses of row values, e.g. #26822

roji · 2023-04-20T18:48:14Z

src/EFCore.Relational/Query/SqlExpressions/TableValuedFunctionExpression.cs

+    public virtual IStoreFunction? StoreFunction { get; }
+
+    /// <inheritdoc />
+    ITableBase? ITableBasedExpression.Table


Before this PR, TableValuedFunctionExpression could only be used with a store function from the model; this changes it to be usable for any TVF, without any necessary model data on it (e.g. OpenJson).

roji · 2023-04-20T19:00:22Z

src/EFCore.SqlServer/Storage/Internal/SqlServerTypeMappingSource.cs

+        // 3. The element CLR type has a supported type mapping which isn't itself a collection (nested collections not yet supported).
+
+        // Note that e.g. Newtonsoft.Json's JToken is enumerable over itself, exclude that scenario to avoid stack overflow.
+        if (mappingInfo.ClrType?.TryGetElementType(typeof(IEnumerable<>)) is not { } elementClrType


@ajcvickers @AndriySvyryd this is another place where we need some type mapping infra (#30730) - at least some parts of this function should probably be in core/relational; we should also consider having providers just support collections via JSON arrays by default, without them needing to actually do anything (but allow Npgsql to override).

Note also the annoying construction of the collection type mapping below, with the cloning etc.

roji · 2023-04-20T19:03:24Z

src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs

+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    protected override ShapedQueryExpression? TranslateCount(ShapedQueryExpression source, LambdaExpression? predicate)


Note this specialized translation of LINQ Count() when applied to a primitive collection (JSON string); instead of using json_each to convert to a table and use a SQL scalar subquery to do COUNT(*), we just use a dedicated function for that.

Npgsql has a ton of such specialized pattern matching translations - it's really nice. See roji/efcore.pg@83842cf#diff-710c2323c08f93bcf612ff7124133abf291f46900fcf8f0adc89eaf5709d22e9R119 and below.

roji · 2023-04-20T19:03:57Z

src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs

+            jsonEachExpression, columnName: "value", columnType: elementClrType, columnTypeMapping: elementTypeMapping,
+            isColumnNullable: null);
+
+        // TODO: SQLite does have REAL and BLOB types, which JSON does not. Need to possibly cast to that.


roji · 2023-04-20T19:04:15Z

src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs

+
+                // The "standard" JSON timestamp representation is ISO8601, with a T between date and time; but SQLite's representation has
+                // no T. Apply a conversion on the value coming out of json_each.
+                SqliteDateTimeTypeMapping => _sqlExpressionFactory.Function(


To be replaced by #30727

roji · 2023-04-20T19:07:05Z

src/EFCore/Storage/ValueConversion/CollectionToJsonStringConverter.cs

+/// <summary>
+///     A value converter that converts a .NET primitive collection into a JSON string.
+/// </summary>
+// TODO: This currently just calls JsonSerialize.Serialize/Deserialize. It should go through the element type mapping's APIs for


This is where we'd use the JSON serialization/deserialization APIs on the type mapping, which we'd introduce in #30677.

We'll discuss this in detail in the PR for #30677.

src/EFCore.SqlServer/Infrastructure/SqlServerDbContextOptionsBuilder.cs

roji · 2023-04-21T14:03:21Z

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

-            // So we clone the expression to avoid any side-effects, but that also prevents us from directly comparing columns (since
-            // the ValuesExpression has been cloned too).
-            // We should simply be able to pattern-match directly on the projection mappings (currently private) without cloning/applying
-            var clonedSelect = selectExpression.Clone();


@maumar note this change - I think this aligns with how things are supposed to happen in the query pipeline.

Instead of cloning and applying the projection on the SelectExpression in order to check its projections, the right thing seems to be to look at the ShaperExpression instead, and use that the resolve the projection from the selectExpression - without needing to apply projections.

apply projection has some really complicated logic, for stuff like group by or general subquery projection, so that might not be a generally safe approach. But for simple pattern match like here, it should be perfectly fine

OK. Though I did remove cloning and applying the projection even for this simpler case - I now access the projection via the shaper instead (via selectExpression.GetProjection(projectionBindingExpression)). It feels more correct/safer (and even more efficient as we don't clone).

I do think we can probably improve things for this sort of pattern-matching - but we can think about it later.

src/EFCore/Query/QueryTranslationPreprocessor.cs

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

src/EFCore.Relational/Query/SqlExpressionFactory.cs

src/EFCore.SqlServer/Infrastructure/Internal/SqlServerOptionsExtension.cs

AndriySvyryd

Non-query parts look good enough.

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

...Core.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSplitQuerySqlServerTest.cs

...Relational.Specification.Tests/Query/NonSharedPrimitiveCollectionsQueryRelationalTestBase.cs

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

maumar · 2023-04-25T22:02:58Z

query stuff looking good as well! 🚀

Closes dotnet#30163

Closes dotnet#29427 Closes dotnet#30426 Closes dotnet#13617

roji requested a review from a team April 20, 2023 18:38

roji changed the title ~~Implement SQL Server compatibility level~~ Support primitive collections Apr 20, 2023

roji commented Apr 20, 2023

View reviewed changes

roji force-pushed the PrimitiveCollections branch 4 times, most recently from 527ca05 to 642e5d7 Compare April 21, 2023 08:14

ErikEJ reviewed Apr 21, 2023

View reviewed changes

src/EFCore.SqlServer/Infrastructure/SqlServerDbContextOptionsBuilder.cs Outdated Show resolved Hide resolved

ErikEJ reviewed Apr 21, 2023

View reviewed changes

src/EFCore.SqlServer/Infrastructure/SqlServerDbContextOptionsBuilder.cs Outdated Show resolved Hide resolved

roji commented Apr 21, 2023

View reviewed changes

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs Show resolved Hide resolved

AndriySvyryd reviewed Apr 24, 2023

View reviewed changes

src/EFCore.Relational/Query/SqlExpressionFactory.cs Show resolved Hide resolved

AndriySvyryd reviewed Apr 24, 2023

View reviewed changes

src/EFCore.SqlServer/Infrastructure/Internal/SqlServerOptionsExtension.cs Show resolved Hide resolved

roji mentioned this pull request Apr 24, 2023

Metadata and type mapping support for primitive collections #30730

Closed

AndriySvyryd approved these changes Apr 25, 2023

View reviewed changes

maumar reviewed Apr 25, 2023

View reviewed changes

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved

maumar reviewed Apr 25, 2023

View reviewed changes

...Core.SqlServer.FunctionalTests/Query/ComplexNavigationsCollectionsSplitQuerySqlServerTest.cs Show resolved Hide resolved

maumar reviewed Apr 25, 2023

View reviewed changes

...Relational.Specification.Tests/Query/NonSharedPrimitiveCollectionsQueryRelationalTestBase.cs Show resolved Hide resolved

roji force-pushed the PrimitiveCollections branch from 04e0025 to 6f77314 Compare April 25, 2023 10:10

maumar reviewed Apr 25, 2023

View reviewed changes

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved

maumar approved these changes Apr 25, 2023

View reviewed changes

roji added 2 commits April 26, 2023 13:06

Implement SQL Server compatibility level

e6ea591

Closes dotnet#30163

Support primitive collections

95120af

Closes dotnet#29427 Closes dotnet#30426 Closes dotnet#13617

roji force-pushed the PrimitiveCollections branch from 6f77314 to 95120af Compare April 26, 2023 11:16

roji merged commit c0f9164 into dotnet:main Apr 26, 2023

roji deleted the PrimitiveCollections branch April 26, 2023 12:09

This was referenced Apr 26, 2023

Json: add support for collection of primitive types inside JSON columns #28688

Closed

Support primitive collections (port to 8.0-preview4) #30777

Merged

ajcvickers mentioned this pull request Apr 27, 2023

Latest news and progress on .NET 8 and EF8 #29989

Closed

roji mentioned this pull request May 3, 2023

IN() list queries are not parameterized, causing increased SQL Server CPU usage #13617

Closed

Conversation

roji commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roji Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndriySvyryd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maumar commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

roji commented Apr 20, 2023 •

edited

Loading

roji Apr 26, 2023 •

edited

Loading