Skip to content

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)

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

4 participants