You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "VasShabu (via GitHub)" <gi...@apache.org> on 2026/08/14 21:06:29 UTC
[PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
VasShabu opened a new pull request, #28978:
URL: https://github.com/apache/flink/pull/28978
<!--
*Thank you very much for contributing to Apache Flink - we are happy that you want to help us improve Flink. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
*Please understand that we do not do this to make contributions to Flink a hassle. In order to uphold a high standard of quality for code contributions, while at the same time managing a large number of contributions, we need contributors to prepare the contributions well, and give reviewers enough contextual information for the review. Please also understand that contributions that do not follow this guide will take longer to review and thus typically be picked up with lower priority by the community.*
## What is the purpose of the change
This pull request alters JSON_LENGTH's codeGen to be inline with JSON_TYPE, as these 2 functions are very similar in nature its smart to ensure that the planning stage works the same way.
*(For example: This pull request makes task deployment go through the blob server, rather than through RPC. That way we avoid re-transferring them on each deployment (during recovery).)*
## Brief change log
*(for example:)*
- Alters JSON_LENGTH's prefix regex so that it doesnt miss any cases
- isPathDefinite function got moved to the Codegen stage in order to call a validation error over a runtime error
- JSON_LENGTH test cases got changed to accomodate for the new validation error
- reuse getOrCreateParsedJson(jsonType) for jsonLength
## Verifying this change
Please make sure both new and modified tests in this PR follow [the conventions for tests defined in our code quality guide](https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#7-testing).
*(Please pick either of the following options)*
This change added tests and can be verified as follows:
./mvnw -o -pl flink-table/flink-table-planner -Dtest='JsonFunctionsITCase' -Dsurefire.failIfNoSpecifiedTests=false -Dcheckstyle.skip=true -Dspotless.check.skip=true -Drat.skip=true -e -Denforcer.skip=true test
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (yes)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (don't know)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (yno)
- If yes, how is the feature documented? (not documented) (just optimisation)
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change the checkbox below to `[X]` and replace the placeholder in the "Generated-by"
line with the tool name and version. Otherwise remove the "Generated-by" line.
See the ASF Generative Tooling Guidance for details:
https://www.apache.org/legal/generative-tooling.html
You are responsible for the quality and correctness of every change in this PR
regardless of the tooling used. Low-effort AI-generated PRs will be closed. See
AGENTS.md for the full guidance.
-->
- [X] Yes (please specify the tool below)
Generated-by: Claude Opus 5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "VasShabu (via GitHub)" <gi...@apache.org>.
VasShabu commented on PR #28978:
URL: https://github.com/apache/flink/pull/28978#issuecomment-5341597162
> Hey Vas, can you get CI to be green and I think we can merge this one?
sure will try to make CI green
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #28978:
URL: https://github.com/apache/flink/pull/28978#discussion_r3812572539
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/JsonFunctionsITCase.java:
##########
@@ -117,10 +117,12 @@ private static TestSetSpec jsonLengthSpec() {
"$",
"{\"a\":[true, false, null]}",
"{}",
- "[]")
+ "[]",
+ "{\"a\": 1}",
Review Comment:
Is this being used somewhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #28978:
URL: https://github.com/apache/flink/pull/28978#issuecomment-5303570827
your problem is that now everything is commented since you have NOT closed markdown comment started with `<!--`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "VasShabu (via GitHub)" <gi...@apache.org>.
VasShabu commented on PR #28978:
URL: https://github.com/apache/flink/pull/28978#issuecomment-5303384718
> Please do not ignore contributor's form and fill it properly
I thought I did, for some reason its not showing up on github.
<img width="496" height="876" alt="image" src="https://github.com/user-attachments/assets/b0c89d6e-71e8-4637-8ef7-dc5616315703" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on PR #28978:
URL: https://github.com/apache/flink/pull/28978#issuecomment-5358254968
Thanks for the patience and addressing all the nits, @VasShabu! Merged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "VasShabu (via GitHub)" <gi...@apache.org>.
VasShabu commented on PR #28978:
URL: https://github.com/apache/flink/pull/28978#issuecomment-5317308451
> Hey @VasShabu, thanks for following up on this. JSON_LENGTH shares JSON_TYPE's codegen and validation strategy now, but not its tests - the lax/bare-field/$["strict value"] cases added to jsonTypeSpec were never mirrored into jsonLengthSpec. Could you look into extending the tests?
What do you mean specifically by bare-field, could you please clarify, I had added the other 2 cases however.
At the moment i think airlock is not allowing me to push my changes, will try to resolve this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais merged PR #28978:
URL: https://github.com/apache/flink/pull/28978
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "VasShabu (via GitHub)" <gi...@apache.org>.
VasShabu commented on PR #28978:
URL: https://github.com/apache/flink/pull/28978#issuecomment-5304778990
> your problem is that now everything is commented since you have NOT closed markdown comment started with `<!--`
There we go, thanks Sergey.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #28978:
URL: https://github.com/apache/flink/pull/28978#issuecomment-5298238970
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "6452e1023a0e49f75c4b457d087b07a802c2e510",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "6452e1023a0e49f75c4b457d087b07a802c2e510",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 6452e1023a0e49f75c4b457d087b07a802c2e510 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40390][table] Optimised JSON_LENGTH codegen [flink]
Posted by "gustavodemorais (via GitHub)" <gi...@apache.org>.
gustavodemorais commented on code in PR #28978:
URL: https://github.com/apache/flink/pull/28978#discussion_r3820511111
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/codegen/JsonCodeGenUtils.java:
##########
@@ -39,40 +41,82 @@ private JsonCodeGenUtils() {}
*/
public static GeneratedExpression generateJsonType(
CodeGeneratorContext ctx, LogicalType returnType, Seq<GeneratedExpression> operands) {
- boolean hasPath = operands.length() == 2;
return GenerateUtils.generateCallWithStmtIfArgsNotNull(
ctx,
returnType,
operands,
true,
false,
argTerms -> {
- ParsedJson parsed = getOrCreateParsedJson(ctx, argTerms.head() + ".toString()");
- String call;
- if (hasPath) {
- String pathSpec = operands.apply(1).literalValue().get().toString();
- boolean definite = SqlJsonUtils.isPathDefinite(pathSpec);
- call =
- CodeGenUtils.qualifyMethod(BuiltInMethods.JSON_TYPE_PATH())
- + "("
- + parsed.varName
- + ", "
- + argTerms.apply(1)
- + ".toString(), "
- + definite
- + ")";
- } else {
- call =
- CodeGenUtils.qualifyMethod(BuiltInMethods.JSON_TYPE())
- + "("
- + parsed.varName
- + ")";
- }
- String resultExpr = CodeGenUtils.BINARY_STRING() + ".fromString(" + call + ")";
- return new Tuple2<>(parsed.parseCode, resultExpr);
+ Tuple2<String, String> parsedCall =
+ generateCallOnParsedInput(
+ ctx,
+ operands,
+ argTerms,
+ BuiltInMethods.JSON_TYPE(),
+ BuiltInMethods.JSON_TYPE_PATH());
+ String resultExpr =
+ CodeGenUtils.BINARY_STRING() + ".fromString(" + parsedCall._2() + ")";
+ return new Tuple2<>(parsedCall._1(), resultExpr);
});
}
+ /**
+ * Generates {@code JSON_LENGTH(jsonValue)} or {@code JSON_LENGTH(jsonValue, path)}.
+ *
+ * <p>The parsed context is shared with the other JSON functions over the same input, so the
+ * input is parsed only once per record.
+ */
+ public static GeneratedExpression generateJsonLength(
+ CodeGeneratorContext ctx, LogicalType returnType, Seq<GeneratedExpression> operands) {
+ return GenerateUtils.generateCallWithStmtIfArgsNotNull(
+ ctx,
+ returnType,
+ operands,
+ true,
+ false,
+ argTerms ->
+ generateCallOnParsedInput(
+ ctx,
+ operands,
+ argTerms,
+ BuiltInMethods.JSON_LENGTH(),
+ BuiltInMethods.JSON_LENGTH_PATH()));
+ }
+
+ /**
+ * Builds the call against the shared parsed input: the whole-document overload, or the path
+ * overload with the {@code definite} flag resolved from the path literal at plan time.
+ *
+ * @return the parse statement and the call expression
+ */
+ private static Tuple2<String, String> generateCallOnParsedInput(
+ CodeGeneratorContext ctx,
+ Seq<GeneratedExpression> operands,
+ Seq<String> argTerms,
+ Method wholeDocument,
+ Method withPath) {
+ final ParsedJson parsed = getOrCreateParsedJson(ctx, argTerms.head() + ".toString()");
+ if (argTerms.length() == 1) {
+ return new Tuple2<>(
+ parsed.parseCode,
+ CodeGenUtils.qualifyMethod(wholeDocument) + "(" + parsed.varName + ")");
+ }
+
+ final String pathSpec = operands.apply(1).literalValue().get().toString();
+ final boolean definite = SqlJsonUtils.isPathDefinite(pathSpec);
Review Comment:
Sorry @VasShabu, I have one more nit. The variable is called definite everywhere and one asks itself what it means, it the function definite? Could you rename it everywhere?
```suggestion
final boolean isPathDefinite = SqlJsonUtils.isPathDefinite(pathSpec);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org