-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Bring JSON type out of experimental status #36442
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,21 +254,28 @@ when _columnsToRewrite.TryGetValue((columnExpression.TableAlias, columnExpressio | |
| jsonScalar.IsNullable); | ||
| } | ||
|
|
||
| case SqlServerOpenJsonExpression openJsonExpression: | ||
| // Currently, OPENJSON does not accept a "json" type, so we must cast the value to a string. | ||
| // We do this for both the case where is a string type mapping for a top-level property with the store type | ||
| // of "json", and when there is an "element" type mapping to something in the document but is now being used | ||
| // with OPENJSON. | ||
| return openJsonExpression.JsonExpression.TypeMapping | ||
| is SqlServerStringTypeMapping { StoreType: "json" } | ||
| or SqlServerStructuralJsonTypeMapping { StoreType: "json" } | ||
| ? openJsonExpression.Update( | ||
| new SqlUnaryExpression( | ||
| ExpressionType.Convert, | ||
| (SqlExpression)Visit(openJsonExpression.JsonExpression), | ||
| typeof(string), | ||
| typeMappingSource.FindMapping(typeof(string))!)) | ||
| : base.Visit(expression); | ||
| // The SQL Server json type cannot be compared ("The JSON data type cannot be compared or sorted, except when using the | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to confirm this in design.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Design decision: we'll go ahead with allowing comparison of JSON-mapped complex types by adding this cast to
|
||
| // IS NULL operator"). So we find comparisons that involve the json type, and apply a conversion to string (nvarchar(max)) | ||
| // to both sides. We exempt this when one of the sides is a constant null (not required). | ||
| case SqlBinaryExpression | ||
| { | ||
| OperatorType: ExpressionType.Equal or ExpressionType.NotEqual, | ||
| Left: var left, | ||
| Right: var right | ||
| } comparison | ||
| when (left.TypeMapping?.StoreType is "json" || right.TypeMapping?.StoreType is "json") | ||
| && left is not SqlConstantExpression { Value: null } && right is not SqlConstantExpression { Value: null }: | ||
| { | ||
| return comparison.Update( | ||
| sqlExpressionFactory.Convert( | ||
| left, | ||
| typeof(string), | ||
| typeMappingSource.FindMapping(typeof(string))), | ||
| sqlExpressionFactory.Convert( | ||
| right, | ||
| typeof(string), | ||
| typeMappingSource.FindMapping(typeof(string)))); | ||
| } | ||
|
|
||
| default: | ||
| return base.Visit(expression); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that this cast is no longer needed - OPENJSON can just directly accept the json data type.