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.
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.
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.
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.
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'?
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 |
gortiz
force-pushed
the
jsonFormat-with-null
branch
from
8e48653
to
60dacad
Compare
| @@ -76,7 +76,7 @@ public static String toJsonMapStr(@Nullable Map map) | |||
| /** | |||
| * Convert object to Json String | |||
| */ | |||
| @ScalarFunction(nullableParameters = true) | |||
| @ScalarFunction | |||
This PR changes json_extract 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.