Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to #30410 - JSON: optimize update path for single property element - don't wrap the value in json array #30422

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Mar 7, 2023

Rather than generate a JSON parameter and extract correct value from it, we generate the parameter value directly. Special casing is handled on the provider level by *ModificationCommand and *UpdateSqlGenerator

Fixes #30410

…t - don't wrap the value in json array

Rather than generate a JSON parameter and extract correct value from it, we generate the parameter value directly. Special casing is handled on the provider level by *ModificationCommand and *UpdateSqlGenerator

Fixes #30410
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Or at least I like the new SQL. I'll let others comment on the implementation.

I don't like that providers have to use internal API for this, is that a temporary solution?

@maumar
Copy link
Contributor Author

maumar commented Mar 14, 2023

@bricelam yes, temporary until we figure out the correct type mapping infra for JSON. Once we have it, the mapping itself should be able to render the correct representation of the parameter value and that entire internal API can be removed

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this indeed looks much better. I'm not looking too closely at the implementation since it's a temporary situation before we properly solve the JSON type mapping infra stuff.

Note the comment below though (which can be handled separately from this PR)

@p2='1'

SET IMPLICIT_TRANSACTIONS OFF;
SET NOCOUNT ON;
UPDATE [JsonEntitiesBasic] SET [OwnedCollectionRoot] = JSON_MODIFY([OwnedCollectionRoot], 'strict $[1].Number', CAST(JSON_VALUE(@p0, '$[0]') AS int)), [OwnedReferenceRoot] = JSON_MODIFY([OwnedReferenceRoot], 'strict $.Number', CAST(JSON_VALUE(@p1, '$[0]') AS int))
UPDATE [JsonEntitiesBasic] SET [OwnedCollectionRoot] = JSON_MODIFY([OwnedCollectionRoot], 'strict $[1].Number', CAST(@p0 AS int)), [OwnedReferenceRoot] = JSON_MODIFY([OwnedReferenceRoot], 'strict $.Number', CAST(@p1 AS int))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm still not clear on why we're sending 1024 as a string parameter, and then casting server side - can't we just send an int parameter instead? Effectively the following:

DECLARE @value INT = 1024;
SELECT JSON_MODIFY('{ "Number" : 1, "Bool" : true }', 'strict $.Number', @value);

Result: { "Number" : 1024, "Bool" : true }

In other words, JSON_MODIFY knows how to get a nice (non-JSON!) SqlParameter value and convert it to the right JSON representation. Essentially the same thing we were just discussing for OPENJSON, but the other way around (OPENJSON converts a JSON value to a non-JSON SQL Server value).

This even works well for bool:

DECLARE @value BIT = 0;
SELECT JSON_MODIFY('{ "Number" : 1, "Bool" : true }', 'strict $.Bool', @value);

Result: { "Number" : 1, "Bool" : false }

(but maybe the idea is for this to be resolved in a next PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have type mapping for int/bool/etc in this context. The only type mapping we have is the json column one, so all parameters are created based on it. This should all be solved with proper mapping for json.

@maumar maumar merged commit 03aa59c into main Mar 14, 2023
@maumar maumar deleted the fix30410 branch March 14, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON: optimize update path for single property element - don't wrap the value in json array
3 participants