Json full query support well almost

[maumar]

building on top of @roji 's json query support #31095

[maumar]

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?).

[roji]

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?

[roji]

@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.

[roji]

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?

[roji]

Approving @maumar's work; am taking over for the remaining nits/cleanups.

[maumar]

@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