Fix to #31365 - Query: add support for projecting JSON entities that have been composed on #31391

maumar · 2023-08-01T08:38:08Z

Adding new materialization path for JSON entities that have been converted to query roots - they look like normal entity, but materialization is somewhat different.
We need to prune synthesized key properties from materializer code (as they are not associated with any column, but rather made up on the fly)
Also added code to include all the child navigations. For normal entities we have IncludeExpressions in the shaper expression, but for JSON we prune all includes and build them in the materializer.

Fix is to recognize the scenario (entity projection but the entity is mapped to JSON) during apply projection phase and generate all the necessary info needed rather than just dictionary of property to index maps like we do for regular entity.
Then when we build materializer we use that info to prune the synthesized properties from key check and re-use existing include JSON entity code to add child navigations.

Fixes #31365

maumar · 2023-08-01T08:42:28Z

@roji, turned out bit easier than I thought (knock on wood) there are still some tests failing due to some bad assumptions in the JSON post processor. Also SelectMany is still broken (will take a look), and it looks like OPENJSON with WITH needs to add AS JSON for the primitve collection properties, otherwise they come out as null (at least in some scenarios). But when I do that, I run into some bad state in the JSON post processor. Also code still needs some major cleanup, as I hacked it rather quickly ^^'

maumar · 2023-08-01T08:44:02Z

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

-            {
-                AddToProjection(entityProjectionExpression.DiscriminatorExpression, DiscriminatorColumnAlias);
+                //achievement unlocked ;P
+                do


Ahahahaha 👍

I'm 100% sure we'll figure out why this is somehow incorrect and needs to be a regular while

ilmax · 2023-08-01T09:19:42Z

src/EFCore.Relational/Query/JsonQueryExpression.cs

+        var targetPrimaryKeyProperties = targetEntityType.FindPrimaryKey()!.Properties.Take(KeyPropertyMap.Count);
+        var sourcePrimaryKeyProperties = EntityType.FindPrimaryKey()!.Properties.Take(KeyPropertyMap.Count);
        foreach (var (target, source) in targetPrimaryKeyProperties.Zip(sourcePrimaryKeyProperties, (t, s) => (t, s)))
        {
-            newKeyPropertyMap[target] = _keyPropertyMap[source];
+            newKeyPropertyMap[target] = KeyPropertyMap[source];


nit: If accessing KeyPropertyMap is considered to be somehow expensive due to the virtual call, it can be cached into a local here

I'll leave it for our Resident Optimization Justification Investigator to decide, @roji

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

ajcvickers · 2023-08-01T12:05:16Z

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

+                    Name = jsonPropertyName,
+                    TypeMapping = property.GetRelationalTypeMapping(),
+                    Path = new PathSegment[] { new(jsonPropertyName) },
+                    //AsJson = property.GetRelationalTypeMapping().ElementTypeMapping is not null


when using OPENJSON with WITH we need to mark primitive collections with AS JSON keyword, otherwise they come out as null. When I do it, we hit some bad assumptions in the json post processor, so I commented it temporarily. Will uncomment and fix json post processor in the final version.

Note that I made some minor changes in #31369 to the JSON postprocessor, these could in theory be related (so rebase in any case).

Anyway, let me know if you want my help with this tomorrow.

ajcvickers

🐑 🇮🇹

roji · 2023-08-01T12:33:39Z

Ahh, the season of 🐑 🇮🇹 has begun :)

@maumar will also review shortly for the SelectExpression parts.

roji

My understanding here is pretty vague... But looks OK :)

Let's think about cleanup later when everything is calmer.

roji · 2023-08-01T11:53:47Z

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

@@ -1690,6 +1682,33 @@ Expression CopyProjectionToOuter(SelectExpression innerSelectExpression, Express
                                projectionIndexMap[jsonProjectionInfo.JsonColumnIndex],
                                newKeyAccessInfo));
                    }
+                    else if (constantValue is QueryableJsonProjectionInfo queryableJsonProjectionInfo)


This whole thing can be a switch 😇

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

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

roji · 2023-08-01T13:18:18Z

@maumar FYI #31369 has been merged, this can now be rebased over main.

maumar · 2023-08-02T01:59:49Z

rebased and updated, please take a final look @roji @ajcvickers

maumar · 2023-08-02T02:03:25Z

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

+
+            protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
+            {
+                if (methodCallExpression is


this should be good enough - just replace reference to the ordinal key with default value for the type we expect. In case of key comparison (handled above) we make an exception because we need to convert key != null into true, rather than null != null which would have been false. In general, since this materialization path is always for non-tracking, we shouldn't have too many instances of the keys present, one other place that I haven't yet tested would be materialization interceptor. @ajcvickers

update: added test for materialization interceptor - works like a charm

maumar · 2023-08-02T02:04:00Z

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

@@ -789,7 +789,9 @@ public void ApplyDistinct()
                        // for JSON entities identifier is the key that was generated when we convert from json to query root (OPENJSON, json_each, etc)
                        // but we can't use it for distinct, as it would warp the results
                        // instead, we will treat every non-key property as identifier
-                        foreach (var property in entityProjection.EntityType.GetDeclaredProperties().Where(p => !p.IsPrimaryKey()))
+
+                        // TODO: hack/workaround, see #31398


maumar · 2023-08-02T02:04:45Z

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

@@ -239,8 +239,10 @@ static bool IsKeyColumn(SqlExpression sqlExpression, TableExpressionBase table)
                columnExpression.Table, "value", columnExpression.Type, _nvarcharMaxTypeMapping, columnExpression.IsNullable);

            Check.DebugAssert(columnInfo.Path is not null, "Path shouldn't be null in OPENJSON WITH");
-            Check.DebugAssert(!columnInfo.AsJson, "AS JSON signifies an owned sub-entity being projected out of OPENJSON/WITH. "
-                + "Columns referring to that must be wrapped be Json{Scalar,Query}Expression and will have been already dealt with above");
+            //Check.DebugAssert(


@roji uncomment to see a bunch of tests failing due to incorrect assumptions we had in the post processor.

…have been composed on Adding new materialization path for JSON entities that have been converted to query roots - they look like normal entity, but materialization is somewhat different. We need to prune synthesized key properties from materializer code (as they are not associated with any column, but rather made up on the fly) Also added code to include all the child navigations. For normal entities we have IncludeExpressions in the shaper expression, but for JSON we prune all includes and build them in the materializer. Fix is to recognize the scenario (entity projection but the entity is mapped to JSON) during apply projection phase and generate all the necessary info needed rather than just dictionary of property to index maps like we do for regular entity. Then when we build materializer we use that info to prune the synthesized properties from key check and re-use existing include JSON entity code to add child navigations. Fixes #31365

maumar requested a review from roji August 1, 2023 08:38

maumar commented Aug 1, 2023

View reviewed changes

ilmax reviewed Aug 1, 2023

View reviewed changes

ajcvickers reviewed Aug 1, 2023

View reviewed changes

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

ajcvickers reviewed Aug 1, 2023

View reviewed changes

ajcvickers approved these changes Aug 1, 2023

View reviewed changes

roji approved these changes Aug 1, 2023

View reviewed changes

maumar force-pushed the fix31365_take2 branch from db2e144 to 36d87b7 Compare August 2, 2023 01:59

maumar commented Aug 2, 2023

View reviewed changes

maumar force-pushed the fix31365_take2 branch 2 times, most recently from 443c468 to d7fd111 Compare August 2, 2023 10:28

maumar changed the base branch from main to release/8.0 September 5, 2023 21:22

maumar force-pushed the fix31365_take2 branch from d7fd111 to 88f17aa Compare September 5, 2023 22:56

maumar merged commit 9d217c7 into release/8.0 Sep 5, 2023

maumar deleted the fix31365_take2 branch September 5, 2023 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to #31365 - Query: add support for projecting JSON entities that have been composed on #31391

Fix to #31365 - Query: add support for projecting JSON entities that have been composed on #31391

maumar commented Aug 1, 2023

maumar commented Aug 1, 2023

maumar Aug 1, 2023

roji Aug 1, 2023

roji Aug 1, 2023 •

edited

Loading

ilmax Aug 1, 2023

maumar Aug 1, 2023 •

edited

Loading

ajcvickers Aug 1, 2023

maumar Aug 1, 2023

roji Aug 1, 2023

ajcvickers left a comment

roji commented Aug 1, 2023

roji left a comment

roji Aug 1, 2023

roji commented Aug 1, 2023

maumar commented Aug 2, 2023

maumar Aug 2, 2023

maumar Aug 2, 2023

maumar Aug 2, 2023

maumar Aug 2, 2023

Fix to #31365 - Query: add support for projecting JSON entities that have been composed on #31391

Fix to #31365 - Query: add support for projecting JSON entities that have been composed on #31391

Conversation

maumar commented Aug 1, 2023

maumar commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maumar Aug 1, 2023 • edited Loading

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

ajcvickers left a comment

Choose a reason for hiding this comment

roji commented Aug 1, 2023

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji commented Aug 1, 2023

maumar commented Aug 2, 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

roji Aug 1, 2023 •

edited

Loading

maumar Aug 1, 2023 •

edited

Loading