Conversation
...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
|
|
||
| _nvarchar4000TypeMapping ??= _typeMappingSource.FindMapping("nvarchar(4000)"); | ||
|
|
||
| if (columnExpression.TypeMapping!.ElementTypeMapping is not null) |
There was a problem hiding this comment.
we need to know if the underlying value was scalar or primitive array - i'm using TypeMapping.ElementTypeMapping is not null for that purpose (is there a better way?) Also, when we are building JsonScalar, in sql generator we have code to introduce casts when they are needed (based on expected type mapping), so perhaps we can avoid manually adding the cast below (in some cases?).
There was a problem hiding this comment.
OK, I understand.
First, I think possibly the more robust/correct way to recognize an OPENJSON over a primitive collection is simply to check if the path is '$'. Looking at the type mapping as you've done should work, but I think that ideally we should be pattern matching on the query tree structure. I'd still add an assertion that ElementTypeMapping isn't null in that case, just for extra checking.
Second, does this code add a JsonScalarExpression for the primitive collection case? Because I don't think that's needed (it wasn't happening before): for that case OPENJSON without WITH just returns the elements of the primitive collection (they still need to be cast to the right type because they're nvarchar(4000)).
So I'd split RewriteOpenJsonColumn a bit more along primitive vs. non-primitive lines. i.e. for primitive collections, just slap the cast on; otherwise, add JsonScalarExpression and refrain from the cast. It may even make sense to have two different functions here.
Makes sense?
06dea50 to
a4f4769
Compare
- adding support for projecting primitive arrays from JSON entities (making sure we can deal with all the Includes that owned types generate by default) - some other fixes - more tests
a4f4769 to
a6e33a4
Compare
roji
left a comment
There was a problem hiding this comment.
@maumar looks pretty good. There's definitely some cleanup potential but we can do that later. Apart from a few nits should be good for merging.
Note that if you think there's a chance you can get the projection fully working (as discussed offline) before you leave on vacation, that's obviously the high-priority thing - I can always take care of the nits and merge later. Up to you.
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.SqlServer/Query/Internal/SqlServerQueryTranslationPostprocessor.cs
Outdated
Show resolved
Hide resolved
|
|
||
| _nvarchar4000TypeMapping ??= _typeMappingSource.FindMapping("nvarchar(4000)"); | ||
|
|
||
| if (columnExpression.TypeMapping!.ElementTypeMapping is not null) |
There was a problem hiding this comment.
OK, I understand.
First, I think possibly the more robust/correct way to recognize an OPENJSON over a primitive collection is simply to check if the path is '$'. Looking at the type mapping as you've done should work, but I think that ideally we should be pattern matching on the query tree structure. I'd still add an assertion that ElementTypeMapping isn't null in that case, just for extra checking.
Second, does this code add a JsonScalarExpression for the primitive collection case? Because I don't think that's needed (it wasn't happening before): for that case OPENJSON without WITH just returns the elements of the primitive collection (they still need to be cast to the right type because they're nvarchar(4000)).
So I'd split RewriteOpenJsonColumn a bit more along primitive vs. non-primitive lines. i.e. for primitive collections, just slap the cast on; otherwise, add JsonScalarExpression and refrain from the cast. It may even make sense to have two different functions here.
Makes sense?
a621d09 to
1cb3250
Compare
|
@roji 's changes look good as well. One thing to consider - in sqlite case we could do something similar to what we do for OPENJSON without WITH - it's essentially the same case. I.e. generate faux columns in the translation phase and store the json path information on the json_each node itself (like we do for SqlServerOpenJsonExpression. And then in post translation phase convert those columns to jsonscalars - we do that for sql server when we need (i.e. when we need key column for order or as identifier), in sqlite we would do this step always. OTOH this approach might be more hacky then the current FakeMember (which is also temporary if we extend/change the projection/binding logic), but food for thought |
* Re-enable `Json_collection_of_primitives_SelectMany` Fixed by #31369. * Re-enable `Column_collection_inside_json_owned_entity`
building on top of @roji 's json query support #31095