Use JsonScalarExpression for primitive collection indexing #30769

roji · 2023-04-26T12:30:13Z

Mostly completes #30724, except for full type inference is blocking on #30730.

roji · 2023-04-26T12:39:33Z

src/EFCore.Relational/Query/SqlExpressionFactory.cs

+        // TODO: blocked on #30730: we need to be able to construct a JSON collection type mapping based on the element's.
+        // For now, hacking to apply the default type mapping instead.
+        return new JsonScalarExpression(
+            ApplyDefaultTypeMapping(array), // Hack, until #30730


Note that here we'd infer the array's mapping from the element's inferred one, after #30730

roji · 2023-04-26T12:40:21Z

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

-
-    internal JsonScalarExpression(
-        ColumnExpression jsonColumn,
+        SqlExpression json,


This changes JsonScalarExpression to be over any arbitrary SqlExpression, not necessarily a ColumnExpression and without an IProperty. This is necessary to support indexing over JSON parameter collections.

roji · 2023-04-26T12:40:50Z

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

+            nullable |= jsonColumnExpression.IsNullable;
+        }
+
+        PathSegment[]? newPath = null;


Note that VisitChildren didn't visit inside array index expression in the path - added support for that.

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

maumar · 2023-04-27T17:58:40Z

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

+                {
+                    newSegment = new PathSegment(newArrayIndex);
+
+                    if (newPath is null)


you do it this way to avoid allocation in case all segments are the same? or is there some other reason? should we switch to this pattern everywhere we visit a collection?

Yep, that's why... And yeah, I've been gradually switching to this pattern for new code I write...

Note that we may be able to extract this out to a utility function so as not to repeat it every time...

maumar · 2023-04-27T18:07:02Z

src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs

+
+            // Extract the column projected out of the source, and simplify the subquery to a simple JsonScalarExpression
+            var shaperExpression = source.ShaperExpression;
+            if (shaperExpression is UnaryExpression { NodeType: ExpressionType.Convert } unaryExpression


we use this pattern in few places - consider DRYing (looks like a helpful method not only here but all around the query pipeline)

Absolutely, I had the same thought. There are several repetitive things that are emerging from this - should we revisit that in a separate issue?

yeah, sounds like MQ work

src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs

Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.

ghost · 2023-04-27T20:52:08Z

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2023-04-27T20:52:11Z

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

GitHub Actions

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this

From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

ghost · 2023-04-27T20:52:14Z

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

GitHub Actions

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this

From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

ghost · 2023-04-27T21:11:44Z

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this

From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

roji requested a review from maumar April 26, 2023 12:30

roji commented Apr 26, 2023

View reviewed changes

roji force-pushed the JsonIndexing branch from b991c05 to 1db7503 Compare April 26, 2023 12:43

roji marked this pull request as draft April 26, 2023 13:32

roji force-pushed the JsonIndexing branch 3 times, most recently from c5e2c85 to 903a989 Compare April 26, 2023 16:43

roji marked this pull request as ready for review April 26, 2023 16:50

maumar reviewed Apr 26, 2023

View reviewed changes

src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs Outdated Show resolved Hide resolved

maumar reviewed Apr 27, 2023

View reviewed changes

src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs Outdated Show resolved Hide resolved

maumar approved these changes Apr 27, 2023

View reviewed changes

Use JsonScalarExpression for primitive collection indexing

0e068fe

Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.

roji force-pushed the JsonIndexing branch from 903a989 to 0e068fe Compare April 27, 2023 20:51

roji added the auto-merge label Apr 27, 2023

ghost merged commit 7b1ef88 into dotnet:main Apr 27, 2023

roji deleted the JsonIndexing branch April 28, 2023 08:17

ajcvickers mentioned this pull request May 12, 2023

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

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use JsonScalarExpression for primitive collection indexing #30769

Use JsonScalarExpression for primitive collection indexing #30769

roji commented Apr 26, 2023

roji Apr 26, 2023

roji Apr 26, 2023

roji Apr 26, 2023

maumar Apr 27, 2023

roji Apr 27, 2023

maumar Apr 27, 2023

roji Apr 27, 2023

maumar Apr 27, 2023

ghost commented Apr 27, 2023

ghost commented Apr 27, 2023

From the bot dev team

ghost commented Apr 27, 2023

From the bot dev team

ghost commented Apr 27, 2023

From the bot dev team

Use JsonScalarExpression for primitive collection indexing #30769

Use JsonScalarExpression for primitive collection indexing #30769

Conversation

roji commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 27, 2023

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Apr 27, 2023

From the bot dev team

ghost commented Apr 27, 2023

From the bot dev team

ghost commented Apr 27, 2023

From the bot dev team

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.