Skip to content

[FLINK-40204][runtime] Introduce statement-level codegen for YAML Transform expressions - #4482

Open
haruki-830 wants to merge 5 commits into
apache:masterfrom
haruki-830:FLINK-40204
Open

[FLINK-40204][runtime] Introduce statement-level codegen for YAML Transform expressions#4482
haruki-830 wants to merge 5 commits into
apache:masterfrom
haruki-830:FLINK-40204

Conversation

@haruki-830

@haruki-830 haruki-830 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Summary

This commit introduces statement-level code generation for Flink CDC YAML Transform expressions. Transform expressions are now represented by a GeneratedExpression intermediate structure and compiled through Janino ScriptEvaluator, allowing nullable AND / OR short-circuit logic to be generated as explicit if / else statements instead of per-record Supplier wrappers.

Key Changes

  1. Introduce Statement-Level Transform Codegen
  • Added GeneratedExpression to carry generated statements, result term, and result Java type.
  • Added TransformExpressionEvaluator as a small wrapper around Janino ScriptEvaluator.
  • Updated projection and filter processors to compile and evaluate generated scripts.
  1. Support Statement-Level Nullable AND / OR
  • Added translateSqlNodeToGeneratedExpression(...) in JaninoCompiler.
  • Generates explicit short-circuit if / else blocks for nullable AND and OR.
  • Ensures the right operand is evaluated only when required.
  1. Improve Generated Script Diagnostics
  • Updated compile/evaluation errors and debug output to print the generated script instead of only the result term such as result$0.
  • Added validation at the script compilation entry to reject empty result terms before passing generated scripts to Janino.
  1. Test Coverage
  • Added coverage for nullable AND / OR SQL three-valued logic.
  • Added short-circuit tests for:
    • false AND (1 / 0 > 0)
    • true OR (1 / 0 > 0)
  • Added projection and filter coverage.
  • Added assertions that generated code no longer contains new Supplier.

JIRA Reference

https://issues.apache.org/jira/browse/FLINK-40204

@haruki-830

Copy link
Copy Markdown
Contributor Author

@lvyanquan PTAL when you have time, Thanks!

@haruki-830
haruki-830 marked this pull request as ready for review July 22, 2026 03:51
@haruki-830
haruki-830 marked this pull request as draft July 22, 2026 04:09
@haruki-830
haruki-830 marked this pull request as ready for review July 22, 2026 04:21
@lvyanquan lvyanquan self-assigned this Jul 22, 2026
@lvyanquan
lvyanquan requested a review from Copilot July 22, 2026 09:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR introduces statement-level Java code generation for Flink CDC YAML Transform expressions, switching evaluation to Janino ScriptEvaluator to enable explicit short-circuiting (if/else) for nullable AND/OR and improve diagnostics by surfacing full generated scripts.

Changes:

  • Add GeneratedExpression as an intermediate representation (statements + result term + result type) and generate statement-level code for transforms.
  • Switch expression compilation/evaluation from Janino ExpressionEvaluator to ScriptEvaluator via a small TransformExpressionEvaluator wrapper.
  • Update projection/filter processors and tests to assert short-circuit code generation and improved error/debug output.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/parser/TransformParser.java Adds translation entry point for filter expressions to GeneratedExpression and wires it into projection generation.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/parser/JaninoCompiler.java Introduces statement-level codegen (GeneratedExpressionGenerator) and adds nullable AND/OR support without Supplier.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/parser/GeneratedExpression.java Adds the new intermediate type representing generated statements and a returnable script.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/TransformExpressionKey.java Stores GeneratedExpression and exposes compiled/full scripts; validates non-empty result term for full script.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/TransformExpressionCompiler.java Switches compilation to ScriptEvaluator and caches TransformExpressionEvaluator.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/TransformExpressionEvaluator.java Adds a wrapper around Janino ScriptEvaluator to evaluate compiled scripts.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/TransformFilterProcessor.java Generates/uses GeneratedExpression for filters and updates error output to show compiled scripts.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/ProjectionColumnProcessor.java Uses compiled scripts for projection evaluation error reporting and GeneratedExpression for compilation keys.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/ProjectionColumn.java Stores GeneratedExpression for projections and updates string output to include compiled scripts.
flink-cdc-runtime/src/main/java/org/apache/flink/cdc/runtime/operators/transform/TransformFilter.java Updates class-level documentation to remove obsolete scriptExpression notes.
flink-cdc-runtime/src/test/java/org/apache/flink/cdc/runtime/parser/TransformParserTest.java Updates expectations for compiled scripts and adds assertions for statement-level codegen characteristics.
flink-cdc-runtime/src/test/java/org/apache/flink/cdc/runtime/parser/JaninoCompilerTest.java Adds tests compiling/evaluating generated scripts and validates empty result-term rejection.
flink-cdc-runtime/src/test/java/org/apache/flink/cdc/runtime/operators/transform/PostTransformOperatorTest.java Adds an end-to-end test ensuring nullable logical transforms work with statement-level codegen; updates error text expectation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Comment on lines +1125 to +1137
private void translateSqlNodeToGeneratedAtoms(
SqlNode sqlNode, List<GeneratedExpression> atoms) {
if (sqlNode instanceof SqlNodeList) {
for (SqlNode node : (SqlNodeList) sqlNode) {
translateSqlNodeToGeneratedAtoms(node, atoms);
}
} else if (sqlNode instanceof SqlIdentifier
|| sqlNode instanceof SqlLiteral
|| sqlNode instanceof SqlBasicCall
|| sqlNode instanceof SqlCase) {
atoms.add(translate(sqlNode, deduceGeneratedExpressionClass(context, sqlNode)));
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest commit.

@lvyanquan

Copy link
Copy Markdown
Contributor

This change offers very limited benefits but has a relatively large impact surface. I think we need to evaluate it carefully.

CC @yuxiqian

@yuxiqian yuxiqian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Haruki for the contribution!

I also doubt the necessity of such changes. Deducing types from SqlNode and manipulating generated code strings looks fragile. Is there any alternative solution to implement "short-circuit call"?

Comment on lines +49 to +54
public String asScript() {
StringBuilder script = new StringBuilder();
appendCode(script, code);
script.append("return ").append(resultTerm).append(";");
return script.toString();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better avoid string manipulations on dumped Java codes. Is Java.ReturnStatement useful here?

Comment on lines +64 to +72
private static void appendCode(StringBuilder builder, String code) {
if (code.isEmpty()) {
return;
}
builder.append(code);
if (code.charAt(code.length() - 1) != '\n') {
builder.append('\n');
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, avoid string ops

if (rvalue == null) {
throw new ParseException("Unrecognized expression: " + sqlNode);
}
return GeneratedExpression.fromExpression(rvalue.toString(), resultClass);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rvalue.toString() is for debugging purposes only and may not generate Java code with correct parentheses.

For example, SQL statement (a - b) / (c + 1) will be converted to a - b / c + 1.

return Object.class;
}

private static boolean isBooleanResultCall(SqlBasicCall sqlBasicCall) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One may write a UDF returning BOOLEAN?

@haruki-830

Copy link
Copy Markdown
Contributor Author

Thanks @lvyanquan and @yuxiqian for the feedback.

I ran a JMH benchmark comparing the Supplier path against an equivalent if/else script and found no statistically significant difference — all configurations fall in the 330k-372k ops/ms range with overlapping confidence intervals, that means the JIT escape analysis reliably eliminates the anonymous Supplier allocation, so the per-record allocation concern is not a real bottleneck in practice.

Given the lack of measurable benefit and the correctness risks @yuxiqian identified, I agree this change is not warranted. I'll go ahead and close this PR later.

@yuxiqian

Copy link
Copy Markdown
Member

Thanks @lvyanquan and @yuxiqian for the feedback.

I ran a JMH benchmark comparing the Supplier path against an equivalent if/else script and found no statistically significant difference — all configurations fall in the 330k-372k ops/ms range with overlapping confidence intervals, that means the JIT escape analysis reliably eliminates the anonymous Supplier allocation, so the per-record allocation concern is not a real bottleneck in practice.

Given the lack of measurable benefit and the correctness risks @yuxiqian identified, I agree this change is not warranted. I'll go ahead and close this PR later.

Thanks for the detailed benchmark report.

rvalue.toString() is for debugging purposes only and may not generate Java code with correct parentheses.

For example, SQL statement (a - b) / (c + 1) will be converted to a - b / c + 1.

Seems this is an existing bug, not introduced by changes in this PR. Could you please investigate this later?

@haruki-830

Copy link
Copy Markdown
Contributor Author

rvalue.toString() is for debugging purposes only and may not generate Java code with correct parentheses.
For example, SQL statement (a - b) / (c + 1) will be converted to a - b / c + 1.

Seems this is an existing bug, not introduced by changes in this PR. Could you please investigate this later?

Thanks for pointing out this bug! I confirmed that BinaryOperation.toString() does lose parentheses when a sub-expression has lower precedence than its parent operator. This affects the existing translateSqlNodeToJaninoExpression path as well.

I'll open a separate PR to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants