-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 handling null string in json_extract() #6439
Fix handling null string in json_extract() #6439
Conversation
`SIMDJsonExtractFunction` returned null directly when there was only one value mapped to the json path and it was null. However, in Presto, the function returns null string instead of null value for this case. Fixes facebookincubator#6437
✅ Deploy Preview for meta-velox canceled.
|
@@ -549,7 +549,7 @@ TEST_F(JsonFunctionsTest, jsonExtract) { | |||
EXPECT_EQ( | |||
"3", jsonExtract("{\"x\": {\"a\" : 1, \"b\" : [2, 3]} }", "$.x.b[1]")); | |||
EXPECT_EQ("2", jsonExtract("[1,2,3]", "$[1]")); | |||
EXPECT_EQ(std::nullopt, jsonExtract("[1,null,3]", "$[1]")); | |||
EXPECT_EQ("null", jsonExtract("[1,null,3]", "$[1]")); |
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.
Here is what I see in Presto,
presto:di> select json_extract('[1,null,3]', '#[1]');
_col0
-------
NULL
(1 row)
Do you see something different?
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.
I run test case of testJsonExtract()
in TestJsonExtractFunctions.java of Presto.
// pass
assertFunction(format("JSON_EXTRACT('%s', '%s')", "[1,null,3]", "$[1]"), JSON, "null");
// fail
assertFunction(format("JSON_EXTRACT('%s', '%s')", "[1,null,3]", "$[1]"), JSON, null);
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.
I see. Looks like I had a typo above:
presto:di> select json_extract('[1,null,3]', '$[1]');
_col0
-------
null
(1 row)
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.
Thank you for the fix.
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@mbasmanova merged this pull request in 524591e. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: `SIMDJsonExtractFunction` returned null directly when there was only one value mapped to the json path and it was null. However, in Presto, the function returns null string instead of null value for this case. Fixes facebookincubator#6437 Pull Request resolved: facebookincubator#6439 Reviewed By: Yuhta Differential Revision: D49009808 Pulled By: mbasmanova fbshipit-source-id: c98e8fbce8d420ebed06403a4b9d3beb5ab8ee47
Summary: `SIMDJsonExtractFunction` returned null directly when there was only one value mapped to the json path and it was null. However, in Presto, the function returns null string instead of null value for this case. Fixes facebookincubator#6437 Pull Request resolved: facebookincubator#6439 Reviewed By: Yuhta Differential Revision: D49009808 Pulled By: mbasmanova fbshipit-source-id: c98e8fbce8d420ebed06403a4b9d3beb5ab8ee47
Summary: `SIMDJsonExtractFunction` returned null directly when there was only one value mapped to the json path and it was null. However, in Presto, the function returns null string instead of null value for this case. Fixes facebookincubator#6437 Pull Request resolved: facebookincubator#6439 Reviewed By: Yuhta Differential Revision: D49009808 Pulled By: mbasmanova fbshipit-source-id: c98e8fbce8d420ebed06403a4b9d3beb5ab8ee47
Summary: `SIMDJsonExtractFunction` returned null directly when there was only one value mapped to the json path and it was null. However, in Presto, the function returns null string instead of null value for this case. Fixes facebookincubator#6437 Pull Request resolved: facebookincubator#6439 Reviewed By: Yuhta Differential Revision: D49009808 Pulled By: mbasmanova fbshipit-source-id: c98e8fbce8d420ebed06403a4b9d3beb5ab8ee47
SIMDJsonExtractFunction
returned null directly when there was only one value mapped to the json path and it was null. However, in Presto, the function returns null string instead of null value for this case.Fixes #6437