Complex JSON query support #36355
Complex JSON query support #36355
Conversation
roji
force-pushed
the
ComplexJson
branch
2 times, most recently
from
18b8471 to
d8e268e
Compare
|
@AndriySvyryd @cincuranet this PR should be more or less ready for review. The tests still have a lot of work so I wouldn't review them too closely, but rather concentrate on the product changes. |
roji
force-pushed
the
ComplexJson
branch
2 times, most recently
from
45c36b3 to
d1ec5f8
Compare
| //github.com/ navigation. | ||
| //github.com/ </summary> | ||
| //github.com/ <returns>The accessor.</returns> | ||
| IClrCollectionAccessor? GetCollectionAccessor(); |
There was a problem hiding this comment.
@AndriySvyryd note that this was moved up to IPropertyBase. If we introduce an IRelationshipProperty abstraction it would go there.
There was a problem hiding this comment.
Note that I introduced IClrIndexedCollectionAccessor that's tailored for complex collections
There was a problem hiding this comment.
OK. For now I'm not sure whether/where that would be needed in query - maybe I'll found out when I turn on change tracking etc.
There was a problem hiding this comment.
What I am saying is that we don't need IClrCollectionAccessor on IPropertyBase
There was a problem hiding this comment.
Ah. Though that would mean we have to switch at each call-site on navigation/complex property to get the accessor? What I did effectively integrates a bit of relationship abstraction into IPropertyBase (this made sense since the collection accessor was nullable anyway, given that non-collection navigations/complex properties don't have one).
Let me know if this bothers or you prefer something else; I'll go ahead and merge this for now to move forward, but I can revert this change in a subsequent PR.
There was a problem hiding this comment.
It's probably fine at this level, just need to keep in mind what API is safe to use depending on the implementation of the interface, which goes against Liskov Substitution Principle.
src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
| return new JsonScalarExpression( | ||
| JsonColumn, | ||
| newPath, | ||
| [.. Path, new(property.GetJsonPropertyName()!)], |
There was a problem hiding this comment.
I thought that you didn't like .. 😆
There was a problem hiding this comment.
No I like ..! For this kind of usage I think it's ideal (construct a new collection from multiple existing collections/elements).
What I hate is this suggestion to turn every x.ToList() to [.. x], that's the one that doesn't make sene to me...
| if (StructuralType is not IEntityType entityType) | ||
| { | ||
| throw new UnreachableException("Navigation on complex JSON type"); | ||
| } |
There was a problem hiding this comment.
Consider converting this and the one below to DebugAssert
There was a problem hiding this comment.
The thing is that we also need to cast StructuralType to IEntityType, so there's a runtime cast in any case; that's when I use this pattern... Make sense?
There was a problem hiding this comment.
Consider introducing Check.Cast<>
There was a problem hiding this comment.
Do you dislike the pattern matching here specifically, or the fact we throw UnreachableException (which presumably we'd do in Check.Cast<>? Do you prefer the more traditional:
var entityType = StructuralType as IEntityType
?? throw new UnreachableException("Navigation on complex JSON type");There was a problem hiding this comment.
Either is fine, I just think that this pattern is now used frequently enough to merit a new method
There was a problem hiding this comment.
OK. Another lighter option is simply... to cast:
var entityType = (IEntityType)StructuralType;The only downside is the lack of a specific message, but given these are unreachable/assertions...
| @@ -614,63 +614,61 @@ protected virtual SelectExpression CreateSelect( | |||
| } | |||
There was a problem hiding this comment.
Is the commented out code above still relevant?
There was a problem hiding this comment.
I think it is... Let's remove the comment and see what happens
There was a problem hiding this comment.
Yeah, this produces some possibly odd SQL changes which I need to investigate... Putting the comment back in, and making a note to investigate separately.
...FCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.CreateSelect.cs
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
| n => n.ForeignKey.IsOwnership | ||
| && n.TargetEntityType.IsMappedToJson() | ||
| && n.ForeignKey.PrincipalToDependent == n)) | ||
| if (jsonQueryExpression.StructuralType is IEntityType entityType) |
There was a problem hiding this comment.
Add a ToDo comment for complex properties
There was a problem hiding this comment.
As above, there's nothing to be done for complex types here since we handle them lazily - only owned JSON entities are eagerly pre-populated with all possible bindings that may occur in the query.
There was a problem hiding this comment.
Consider adding a comment here too
There was a problem hiding this comment.
My bad, I actually think there's a real problem here. Added a TODO comment, will investigate.
Draft PR on complex JSON query support, to allow first reviewing. A lot of stuff is already working, specifically the shaper part (projecting out complex JSON-mapped types and collections), property binding (referencing complex properties and regular JSON-mapped properties in queries).
The main missing part is composing LINQ operators on JSON collections, e.g.
Where(b => b.JsonCollection.Count() == 2)- this is in progress. It may trigger a deeper refactor of how we represent JSON expressions in the query pipeline, affecting primitive collections as well.Note that the first commit is a further reorganizing of the relationship tests, without any product code changes - they all pass. The second commit then adds the complex JSON support and tests on top.
Part of #36296