Implemement set operation query support for complex JSON#36417
Implemement set operation query support for complex JSON#36417roji merged 3 commits intodotnet:mainfrom
Conversation
roji
force-pushed
the
ComplexJsonSetOperations
branch
2 times, most recently
from
58d631a to
f934356
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements set operation query support for complex JSON by extending the Entity Framework Core query pipeline to handle UNION operations over complex JSON structures. It addresses various issues in navigation expansion and adds comprehensive test coverage for different relationship types.
- Adds support for set operations (UNION, INTERSECT, EXCEPT) on complex JSON properties and collections
- Fixes navigation expansion issues when dealing with complex property references in set operations
- Implements proper SQL generation for JSON-mapped complex types in set operation scenarios
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| NavigationExpandingExpressionVisitor.cs | Enhanced to handle ComplexPropertyReference and ComplexTypeReference in set operations |
| NavigationExpandingExpressionVisitor.Expressions.cs | Added ComplexTypeReference class and improved ComplexPropertyReference handling |
| SelectExpression.cs | Added comprehensive JSON handling logic for set operations with proper projection management |
| SqliteQueryableMethodTranslatingExpressionVisitor.cs | Extended JSON query transformation to include complex properties |
| Test files | Added comprehensive test coverage for set operations across different relationship types |
Comments suppressed due to low confidence (1)
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
And fix various related issues in nav expansion Part of dotnet#36296
roji
force-pushed
the
ComplexJsonSetOperations
branch
from
f934356 to
5ebed4f
Compare
| { | ||
| Check.DebugAssert(jsonQuery1.StructuralType == jsonQuery2.StructuralType); | ||
| Check.DebugAssert(jsonQuery1.Type == jsonQuery2.Type); | ||
| Check.DebugAssert(jsonQuery1.IsNullable == jsonQuery2.IsNullable); |
There was a problem hiding this comment.
Why is it safe to assume that the nullability will be the same? Are they modified to match each other earlier in the pipeline?
There was a problem hiding this comment.
Good catch - it's a wrong assertion indeed. Below there was even some logic to handle nullability based on the nullability in each side, but that too was incomplete, pushed a fix.
It's proving surprisingly difficult to cover this case with a test - I'm not sure we already had coverage in the traditional tests... Specifically for the same type being projected once in a nullable and once in a non-nullable way. The various queries I've tried fail translation for other reasons currently.
| @@ -393,14 +393,14 @@ protected override ShapedQueryExpression TransformJsonQueryToTable(JsonQueryExpr | |||
|
|
|||
| propertyJsonScalarExpression[projectionMember] = new JsonScalarExpression( | |||
There was a problem hiding this comment.
containerColumnName does not seem to be used outside of the Assert.
The comment // We're only interested in properties which actually exist in the JSON, filter out uninteresting shadow keys is incorrect as you aren't filtering out shadow properties. Also, invert the condition to reduce nesting
There was a problem hiding this comment.
Re shadow keys/properties, the intent here was to mean that we want to jump over the synthetic key properties, which don't actually exist in the JSON document - that's what the if (property.GetJsonPropertyName() is string jsonPropertyName) does.
I'll change the comment to filter out uninteresting synthetic keys to make it clearer.
|
|
||
| var projectionMember = new ProjectionMember().Append(new FakeMemberInfo(jsonNavigationName)); | ||
|
|
||
| propertyJsonScalarExpression[projectionMember] = new JsonScalarExpression( |
There was a problem hiding this comment.
It's strange that the type is called JsonScalarExpression when it also represents non-scalars
There was a problem hiding this comment.
Tell me about it - this prompted me to file #36392, which contains the details. tl;dr JsonQueryExpression doesn't actually represent JSON_QUERY() in SQL, but rather represents a JSON being projected out of the database (so shaper-side expression only). Whe projections are applied (subquery pushdown, or when translation ends), any projected JsonQueryExpression gets converted to JsonScalarExpression.
... very questionable.
| : RelationshipsSetOperationsTestBase<TFixture>(fixture) | ||
| where TFixture : ComplexPropertiesFixtureBase, new() | ||
| { | ||
| // TODO: the following is temporary until change tracking is implemented for complex JSON types (#35962) |
There was a problem hiding this comment.
It's already implemented 😉
roji
force-pushed
the
ComplexJsonSetOperations
branch
from
b40f2fb to
5e7bbc0
Compare
And fix various related issues in nav expansion
Part of #36296