[FLINK-40089][table] Implement JSON_LENGTH function#28688
Conversation
| public static Integer jsonLength(String input, String pathSpec) { | ||
| return jsonLength(jsonApiCommonSyntax(input, pathSpec)); | ||
| } | ||
|
|
||
| private static Integer jsonLength(JsonPathContext context) { | ||
| if (context.hasException()) { | ||
| return null; | ||
| } | ||
| final Object value = context.obj; | ||
| if (value == null) { | ||
| return null; | ||
| } | ||
| if (value instanceof Map) { | ||
| return ((Map<?, ?>) value).size(); | ||
| } | ||
| if (value instanceof Collection) { | ||
| return ((Collection<?>) value).size(); | ||
| } | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Why to we need this? Don't we have already the runtime class JsonLengthFunction?
| super(BuiltInFunctionDefinitions.JSON_LENGTH, context); | ||
| } | ||
|
|
||
| public @Nullable Integer eval(@Nullable StringData jsonInput) { |
There was a problem hiding this comment.
nit: here and everywhere else make parameters/variables that are immutable final (not enforced on Flink but nicer to read).
| public @Nullable Integer eval(@Nullable StringData jsonInput) { | |
| public @Nullable Integer eval(final @Nullable StringData jsonInput) { |
gustavodemorais
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @VasShabu! Good job on the first version of the PR. We'll go through some iterations to make sure things look good
Apart from what we already flagged above: we have two implementations of the same function. eval(json) uses a new Jackson readTree, eval(json, path) uses the existing Calcite path in SqlJsonUtils. Different parsers + different data models = they can disagree on the same input. We can make the design more consistent by using only one path. Duplicate ObjectMapper - reuse the configured one in SqlJsonUtils instead of a second bare instance. This is important because built-in functions are on the hot path. The logic might be invoked millions of times in a couple of seconds and we have to make sure they allocate as little as possible and their performance are optimized or else the engine will be wasting resources.
That said, In this case, we could go two paths
- Best performance: We build a custom parser to optimize it for length checking by skipping the children. MAPPER.getFactory().createParser(str), read first token; if START_ARRAY/START_OBJECT walk top level counting, skipping children via parser.skipChildren(). No tree alloc. Best for length-only.
- Simplest reuse the shared SqlJsonUtils path so both overloads share one parser.
Take a look at both paths and let me know what you think
| } else if (jsonNode.isTextual() | ||
| || jsonNode.isNumber() | ||
| || jsonNode.isBoolean() | ||
| || jsonNode.isBinary()) { |
There was a problem hiding this comment.
Isn't this check dead code? Can a jsonNode be binary? You can try to write a test to check if it's dead code or it's possible
There was a problem hiding this comment.
Are we sure the selected approach is the right one?
With this approach the same JSON will be parsed several times for queries like
SELECT JSON_LENGTH(my_json), JSON_LENGTH(my_json), JSON_LENGTH(my_json) FROM...or
SELECT JSON_LENGTH(my_json), JSON_VALUE(my_json, '$.a') FROM...| table: jsonLength(jsonObject[, path]) | ||
| description: | | ||
| Returns the number of elements in a JSON document, or the length of the value at the specified path if one is provided. | ||
| Returns NULL if the argument is NULL or the path does not locate a value. |
There was a problem hiding this comment.
this does not look true
simple tests show different behavior
SELECT json_length('$');
SELECT json_length('{');each returns -1 and the doc says nothing about this or did I miss anything?
There was a problem hiding this comment.
More findings:
- Doc doesn't reflect the real behavior: it returns
-1in case of invalid json, while docs says aboutNULL, e.g. querySELECT JSON_LENGTH('null'); - Strange handling of
nullvalues
like a queryboth in MySQL and in Flink it returnsSELECT json_length('{"a":[true, false, null]}', '$.a[1]')
1
nowin MySQL it returnsSELECT json_length('{"a":[true, false, null]}', '$.a[2]')
1, in Flink it returns-1why? - Seems need to check what the behavior should be in case of
lax/strictsince right now it is weirdSELECT json_length('{"x": {"a":1}, "y": {"a":[2, 3]}}', 'lax $.*.a'); -- returns 2
SELECT json_length('{"x": {"a":[1, 2, 3, 4]}, "y": {"a":[2, 3]}}', 'lax $.*.a'); -- returns 2
SELECT json_length('{"x": {"a":[3, 2 , 3]}, "y": {"a":null}, "z":{"a":1}}', 'lax *.a'); -- returns 3
SELECT json_length('{"x": {"a":[3, 2 , 3]}, "y": {"a":null}, "z":{"a":1}}', 'lax *.a[0]'); --returns 1
SELECT json_length('{"x": {"a":[3, 2 , 3]}, "y": {"a":null}, "z":{"a":1}}', 'lax *.a[1]'); --returns 1
SELECT json_length('{"x": {"a":[3, 2 , 3]}, "y": {"a":null}, "z":{"a":1}}', 'lax *.a[2]'); --returns 1
SELECT json_length('{"x": {"a":[3, 2 , 3]}, "y": {"a":null}, "z":{"a":1}}', 'lax *.a[3]'); --returns 0
| """ | ||
| return _unary_op("jsonUnquote")(self) | ||
|
|
||
| def json_length(self, path = None) -> 'Expression': |
There was a problem hiding this comment.
It seems that many databases throw an error, I am curious why we are not throwing an error. Also I see that there are best practises that we should document like using IF(string_column IS JSON, to protect against non json and avoid potential errors.
| final JsonPathContext context = jsonApiCommonSyntax(parsedInput, pathSpec); | ||
| final Object value = context.hasException() ? null : context.obj; | ||
|
|
||
| if (value instanceof LinkedList) { |
There was a problem hiding this comment.
is there a reason we expect LinkedList instead of just List?
There was a problem hiding this comment.
the comment marked as resolved however no answer yet
can you elaborate here?
| logger.codelog.name = org.apache.flink.table.runtime.generated.CompileUtils | ||
| logger.codelog.level = DEBUG | ||
| logger.codelog.appenderRef.test.ref = TestLogger |
There was a problem hiding this comment.
it might be helpful for local debug, however why do we need in repo and making it executable for every ci run?
| if (matched.size() != 1) { | ||
| if (context.mode == PathMode.STRICT || matched.isEmpty()) { | ||
| return null; | ||
| } | ||
| return matched.size(); | ||
| } |
There was a problem hiding this comment.
Could you please elaborate on this? In which conditions we get a null and in which conditions we get the size of the matched?
There was a problem hiding this comment.
looks like not addressed yet
| #logger.codelog.name = org.apache.flink.table.runtime.generated.CompileUtils | ||
| #logger.codelog.level = DEBUG | ||
| #logger.codelog.appenderRef.test.ref = TestLogger | ||
|
|
There was a problem hiding this comment.
can we just drop it?
keeping log setting for every helper here might be overkill
| generateCallWithStmtIfArgsNotNull(ctx, resultType, operands, resultNullable = true) { | ||
| argTerms => | ||
| val inputTerm = s"${argTerms.head}.toString()" | ||
| val (varName, parseCode) = | ||
| ctx.getReusableInputUnboxingExprs(inputTerm, Int.MinValue) match { | ||
| case Some(expr) => (expr.resultTerm, "") | ||
| case None => | ||
| val v = CodeGenUtils.newName(ctx, "jsonParsed") | ||
| ctx.addReusableMember( | ||
| s"${classOf[SqlJsonUtils.JsonValueContext].getName} $v;") | ||
| ctx.addReusableInputUnboxingExprs( | ||
| inputTerm, | ||
| Int.MinValue, | ||
| GeneratedExpression(v, "false", "", null)) | ||
| (v, s"$v = ${qualifyMethod(BuiltInMethods.JSON_PARSE)}($inputTerm);") | ||
| } | ||
| val (method, terms) = | ||
| if (operands.length > 1) | ||
| (BuiltInMethods.JSON_LENGTH_PATH, Seq(varName, s"${argTerms(1)}.toString()")) | ||
| else | ||
| (BuiltInMethods.JSON_LENGTH, Seq(varName)) | ||
| (parseCode, s"${qualifyMethod(method)}(${terms.mkString(", ")})") |
There was a problem hiding this comment.
is it possible to extract it ideally to java helper?
There was a problem hiding this comment.
it seems the lax problem is still not solved
to highlight the issue there are 2 queries:
1.
SELECT json_length('{"x": {"a":[3, 2 , 3]}}', 'lax *.a');which returns 3, I guess the size of x.a.
2.
SELECT json_length('{"x": {"a":[3, 2 , 3]}, "y": {"a":null}}', 'lax *.a');now returns 2 which is amount of x.a, not the amount of elements.
There is nothing about this in doc, and it is very non obvious that depending on query the same syntax will mean completely different things
and there is no info about this in docs
What is the purpose of the change
This pull request adds support for the built-in SQL function
JSON_LENGTH, which returns the number of top-level elements in a JSON array or object. Scalars have a length of 1, andNULLinput returnsNULL. Nested arrays or objects are not counted recursively.Brief change log
JsonLengthFunctionimplementing theJSON_LENGTHbuilt-in functionJSON_LENGTHas a built-in function in the function catalogJSON_LENGTHinJsonFunctionsITCaseVerifying this change
Please make sure both new and modified tests in this PR follow the conventions for tests defined in our code quality guide.
This change added tests and can be verified as follows:
mvn test -pl flink-table/flink-table-runtime -Dtest=JsonFunctionsITCaseJSON_LENGTHcovering scalars, arrays, objects, andNULLinput inJsonFunctionsITCaseDoes this pull request potentially affect one of the following parts:
@Public(Evolving): yesDocumentation
Was generative AI tooling used to co-author this PR?