-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Change json_format to return java null when java null is received #11673
Conversation
public void jsonFormatWithJavaNullReturnsJavaNull() { | ||
GenericRow row = new GenericRow(); | ||
row.putValue("jsonMap", null); | ||
testFunction("json_format(jsonMap)", Lists.newArrayList("jsonMap"), row, 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.
This test fails if executed in master. Instead of Java null, what was returned was the String "null"
/** | ||
* If empty, FunctionsRegistry registers the method name as function name; | ||
* If not empty, FunctionsRegistry only registers the function names specified here, the method name is ignored. | ||
*/ |
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.
Just applying the Boy Scouting Rule, “Always leave the code you are working on a little bit better than you found it.”
cc @klsince |
Codecov Report
@@ Coverage Diff @@
## master #11673 +/- ##
============================================
- Coverage 63.08% 63.06% -0.03%
+ Complexity 1121 1120 -1
============================================
Files 2343 2343
Lines 125676 125676
Branches 19314 19314
============================================
- Hits 79285 79252 -33
- Misses 40739 40773 +34
+ Partials 5652 5651 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
what's the difference between "json null literal" vs. "java null literal"? a simple example could help here. thanks! |
// Whether the scalar function expects and can handle null arguments. | ||
/** | ||
* Whether the scalar function expects and can handle null arguments. | ||
* |
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.
nit: remove this empty line
In Jackson, the null json value is represented as an instance of JsonNode which returns true for JsonNode.isNull(). Meanwhile java null is the typical null we use in Java. |
Got it! Just wondering for json_format(), what kind of user input could lead to a JsonNode.isNull()? Would a "null" string value result in a JsonNode that's isNull()? |
@@ -79,6 +79,9 @@ public static String toJsonMapStr(@Nullable Map map) | |||
@ScalarFunction(nullableParameters = true) | |||
public static String jsonFormat(Object object) | |||
throws JsonProcessingException { | |||
if (object == 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.
This is intentionally added in #8382. If we want to return null, removing the nullableParameters = true
will achieve that. Any specific reason why you don't want string '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.
Changed
Yes
I guess to conserve the nullability. I don't have a strong preference, but some users have been asking to change the behavior. Maybe it is just better to keep the old behavior and to specify the null semantic in the documentation. |
I see. I think it is good to conserve the nullability. Since we have |
8e48653
to
60dacad
Compare
@@ -76,7 +76,7 @@ public static String toJsonMapStr(@Nullable Map map) | |||
/** | |||
* Convert object to Json String | |||
*/ | |||
@ScalarFunction(nullableParameters = true) | |||
@ScalarFunction |
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.
neat 👍
@gortiz , just double checking. |
@jadami10 Good catch. Yes it is a typo. Only |
This PR changes json_format in order to return the Java value null when the input is the Java value null.
The following table shows the difference between the old and new semantic:
This is a bugfix, but can also be considered as a backward incompatible change.