CALCITE-6465: Rework code generator to use Flink code splitter #3901

jduo · 2024-08-02T22:50:23Z

No description provided.

jduo · 2024-08-02T22:50:55Z

This builds but introduces a dependency on flink-core. Looking for a way around this.

linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java

Integrate Flink Java code splitter and run it on all final Expressions when converting to Java code. Next step is to run it based on configuration options

JavaCodeSplitter needs it for Preconditions

jduo · 2024-09-20T21:16:14Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java

@@ -51,7 +51,7 @@ public Type getType() {
  }

  @Override public String toString() {
-    ExpressionWriter writer = new ExpressionWriter(true);
+    ExpressionWriter writer = new ExpressionWriter(true, true);


Ideally this would read from the CalciteSystemProperty but calcite-core depends on linq4j so it can't be looked up here.

Will this have any practical impact on the actual code generation? (like splitting in some circumstances where the CalciteSystemProperty has not been set to true). If not, I'd say this is acceptable.
It's true that it is a bit unfortunate that we cannot check the CalciteSystemProperty here, but I can't think of a better solution at this point.

Ideally this would read from the CalciteSystemProperty but calcite-core depends on linq4j so it can't be looked up here.

I do not know this code, but wonder if the split configuration values be passed by value into the linq4j layer. If possible , this would avoid the dependancy recursion.

I don't think this toString() method is used in code generation, just logging/debugging.
This option is passed in to Expressions static functions, which is really where ExpressionWriters are instantiated most of the time.

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java

rubenada · 2024-09-23T08:12:48Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java

@@ -57,20 +60,39 @@ public MethodDeclaration(int modifier, String name, Type resultType,
  }

  @Override public void accept(ExpressionWriter writer) {
-    String modifiers = Modifier.toString(modifier);
-    writer.append(modifiers);
+    final ExpressionWriter writerForUnsplitMethod = writer.usesMethodSplitting()


I have the impression the intent of this line is not very obvious, perhaps a comment would help to clarify it.

Done in dc1c66d

Thanks @jduo , just a small comment, because I find the comment a bit confusing (maybe I'm getting it wrong), where it says:

... serialize the method declaration directly to the supplied writer if method splitting is enabled or ...

shouldn't it say "disabled" ?

Yes that's correct. I've updated this comment, but note that it is now in ClassDeclaration instead of MethodDeclaration. The method splitter requires an enclosing class to write some of its output too (it can extract variable names to instance variables and move blocks of code to generated private methods to reduce the size of a method).

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java

- Add comments about method splitting - Add overloads with default values - Clarify condition about using a temporary writer

Method splitting has to happen at the class level so that there is a place to write fields and methods generated by reducing the size of a method

Change the property ENABLE_METHOD_SPLITTING to METHOD_SPLITTING_THRESHOLD. It is now an integer specifying the method length limit before splitting instead of a boolean to turn on a fixed limit. A value of zero indicates that method splitting is off. Change tests unrelated to method splitting to set method splitting off.

rubenada · 2024-09-30T15:05:37Z

core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java

+   * Use this setting to automatically detect and split methods larger than the specified
+   * limit.
+   */
+  public static final CalciteSystemProperty<Integer> METHOD_SPLITTING_THRESHOLD =


nit: proposal (to make the new property robust in case of negative values)

public static final CalciteSystemProperty<Integer> METHOD_SPLITTING_THRESHOLD = intProperty("calcite.linq.method_splitting_threshold", 0, v -> v >= 0);

Since 0 is a valid method length, 0 doesn't seem to be a good value to indicate "don't split".

I think that any negative value should mean "don't split", and the default should be -1.

julianhyde · 2024-09-30T20:48:44Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java

@@ -111,7 +111,10 @@ public static Bindable toBindable(Map<String, Object> parameters,
            parameters);

    final ClassDeclaration expr = relImplementor.implementRoot(rel, prefer);
-    String s = Expressions.toString(expr.memberDeclarations, "\n", false);
+    String s =
+        Expressions.toString(expr.memberDeclarations,


put "\n", false, on previous line

julianhyde · 2024-09-30T20:51:17Z

core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java

+   * Use this setting to automatically detect and split methods larger than the specified
+   * limit.
+   */
+  public static final CalciteSystemProperty<Integer> METHOD_SPLITTING_THRESHOLD =


Since 0 is a valid method length, 0 doesn't seem to be a good value to indicate "don't split".

I think that any negative value should mean "don't split", and the default should be -1.

julianhyde · 2024-09-30T20:57:04Z

core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java

@@ -420,6 +420,18 @@ public final class CalciteSystemProperty<T> {
  public static final CalciteSystemProperty<Integer> FUNCTION_LEVEL_CACHE_MAX_SIZE =
      intProperty("calcite.function.cache.maxSize", 0, v -> v >= 0);

+  /**
+   * The length of code before automatic method splitting is enabled


The name/description of the property isn't great. It doesn't include the units (characters, lines, number of statements). The javadoc doesn't say how the threshold is used - whether splitting occurs if a method is greater than or greater than or equal .

Maybe a better name can be found, I don't know.

But the doc should say something like 'A method is split if its size in characters is greater than or equal to the threshold and the threshold is non-negative.'

The field in ExpressionWriter needs better doc.

I've renamed this to be MAX_METHOD_LENGTH_IN_CHARS_BEFORE_SPLITTING and clarified that splitting starts happening if the code is longer than the limit and not when equal to the limit. This is a long name though so I'm open to better names.

On the topic of the constant to indicate no limit, I've changed this to be -1 instead of 0. I don't see a good place to hold the magic number though -- CalciteSystemProperty would be a reasonable user-facing location, but ExpressionWriter can't reference it.

I made the -1 magic number a constant in Expressions

julianhyde · 2024-09-30T20:58:54Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java

@@ -51,7 +51,8 @@ public Type getType() {
  }

  @Override public String toString() {
-    ExpressionWriter writer = new ExpressionWriter(true);
+    // Use the JVM limit for method size (4K) as the method splitting threshold.
+    ExpressionWriter writer = new ExpressionWriter(true, 4000);


I understand that you can't use the system property. But why should the default behavior be to split? I think it should be to not split.

If people want to split they should invoke by other means than toString().

julianhyde · 2024-09-30T20:59:47Z

linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java

@@ -1719,6 +1724,60 @@ public void checkBlockBuilder(boolean optimizing, String expected) {
        Expressions.toString(Expressions.constant(set)));
  }

+  @Test void testLargeMethod() {


test needs good javadoc.

What is MyClass and why is it commented out?

Clarified the meaning of the MyClass comment block and added to the header public javadoc about the test + JIRA.

julianhyde · 2024-09-30T21:11:44Z

bom/build.gradle.kts

@@ -111,6 +111,7 @@ dependencies {
        apiv("org.apache.commons:commons-pool2")
        apiv("org.apache.commons:commons-collections4")
        apiv("org.apache.commons:commons-text")
+        apiv("org.apache.flink:flink-table-code-splitter")


Add a note above this list of declarations that we import flink-table-code-splitter but we must not import core flink modules.

julianhyde · 2024-09-30T21:15:49Z

innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java

-      System.out.println("Innodb: " + Expressions.toString(enumerable));
+      System.out.println(
+          "Innodb: " + Expressions.toString(enumerable,
+          CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value()));


indentation

julianhyde · 2024-09-30T21:18:48Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java

  }

-  ExpressionWriter(boolean generics) {
+  ExpressionWriter(boolean generics, int methodSplittingThreshold) {


Need javadoc for this constructor.

You don't say the units of the threshold (bytes, lines), that it applies to source code length, what values disable splitting, and whether it's > or >=.

Expanded comments and renamed this parameter.

julianhyde · 2024-09-30T21:19:11Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java

+    this.methodSplittingThreshold = methodSplittingThreshold;
+  }
+
+  public ExpressionWriter duplicateState() {


public method needs javadoc

I made this package-private rather than adding javadoc -- it really seems like most of the methods in ExpressionWriter can be package-private.

julianhyde · 2024-09-30T21:20:33Z

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java

+
+  /**
+   * Converts an expression to Java source code.
+   * Optionally splits the generated method if the method is too large.


"too large" is vague. can do better.

Changed this to be more specific about relating to the limit.

sonarcloud · 2024-10-04T18:34:19Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

jduo · 2024-10-09T16:45:28Z

@rubenada @julianhyde , just wondering if there was some more feedback on this and would like some clarification on where system tests reside within Calcite. Thanks.

rubenada · 2024-10-16T07:00:32Z

@jduo , not sure what you mean by "system tests". If you want to add a test with a large dynamic code that fails when this feature is turn off / passes when it is turned on; maybe you could check and adapt the test that was done for the slightly related CALCITE-3094 (and then refined in CALCITE-6593 ): LargeGeneratedJoinTest. The query in that test, with the JOIN_SELECTOR_COMPACT_CODE_THRESHOLD disabled, should throw the "code grows beyond 64 KB InternalCompilerException". At that point, turning on the code splitter from this PR, should make the test pass.

asdfgh19 · 2024-10-18T04:37:28Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java

@@ -111,7 +111,9 @@ public static Bindable toBindable(Map<String, Object> parameters,
            parameters);

    final ClassDeclaration expr = relImplementor.implementRoot(rel, prefer);
-    String s = Expressions.toString(expr.memberDeclarations, "\n", false);
+    String s =
+        Expressions.toString(expr.memberDeclarations, "\n", false,


It seems that code split is not effective here. This is because the code splitting logic in this pr is implemented in ClassDeclaration, and the expr.memberDeclarations here is usually MethodDeclaration.

@asdfgh19 , thanks for your comments. Would you be able to add tests for your scenarios to the PR?

For this one, it seems like we a can pass expr to Expressions.toString(), but I'm not sure what effect that has on the calls that depend on s ( Hook.JAVA_PLAN.run(s);)

asdfgh19 · 2024-10-18T04:41:40Z

core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java

@@ -186,7 +186,9 @@ static Scalar.Producer baz(ParameterExpression context_,
    final ClassDeclaration classDeclaration =
        Expressions.classDecl(Modifier.PUBLIC, "Buzz", null,
            ImmutableList.of(Scalar.Producer.class), declarations);
-    String s = Expressions.toString(declarations, "\n", false);
+    String s =
+        Expressions.toString(declarations, "\n", false,


The declarations here usually do not look like ClassDeclarations either.

asdfgh19 · 2024-10-18T04:44:25Z

core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java

@@ -100,7 +100,9 @@ private static String compile(RexBuilder rexBuilder, List<RexNode> constExps,
        Expressions.methodDecl(Modifier.PUBLIC, Object[].class,
            BuiltInMethod.FUNCTION1_APPLY.method.getName(),
            ImmutableList.of(root0_), blockBuilder.toBlock());
-    String code = Expressions.toString(methodDecl);
+    String code =
+        Expressions.toString(methodDecl,


The methodDecl here is a MethodDeclaration, not a ClassDeclaration, so it seems that the code splitting logic implemented in the ClassDeclaration does not work here.

It seems that this code relates to RexExecutable. The method body is generated here and written to String, then gets attached as the body of a class in RexExecutable that's run directly in Janino's IClassBodyEvaluator. This will be quite complicated to rework.

jduo force-pushed the calcite-6465 branch from 9de964a to 4cbd526 Compare August 21, 2024 20:55

davidradl reviewed Sep 10, 2024

View reviewed changes

linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java Outdated Show resolved Hide resolved

jduo added 7 commits September 18, 2024 15:29

WIP: Integrate Flink code splitter

6ac2d2e

Integrate Flink Java code splitter and run it on all final Expressions when converting to Java code. Next step is to run it based on configuration options

Remove exclusion of flink-core

c63821e

JavaCodeSplitter needs it for Preconditions

Fix format violations

5556360

Refactor so that only methods get code-split

d3cf335

Remove extra newline

739c747

Update calls to Expressions.toString()

e9e20e1

Merge fixes

dc783a9

jduo force-pushed the calcite-6465 branch from 7cc5816 to dc783a9 Compare September 18, 2024 23:20

jduo added 10 commits September 19, 2024 12:22

Fix whitespace differences in generated code

b7553e4

Checker framework

f63b665

Autostyle

4216e80

Parameterize use of method splitting

2077d1e

Enable method splitting in linq4j tests

fd2de3d

Make method-splitting in library code based on a CalciteSystemProperty

8f04243

Fix commented-out code

dd5b995

Revert unintended change around assert

8f62b88

Revert unintended change

1766b35

Fix merge issues

cd33092

jduo marked this pull request as ready for review September 20, 2024 21:08

jduo commented Sep 20, 2024

View reviewed changes

rubenada reviewed Sep 23, 2024

View reviewed changes

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java Outdated Show resolved Hide resolved

rubenada reviewed Sep 23, 2024

View reviewed changes

linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java Outdated Show resolved Hide resolved

jduo added 4 commits September 24, 2024 16:13

PR feedback

dc1c66d

- Add comments about method splitting - Add overloads with default values - Clarify condition about using a temporary writer

Turn off method-splitting for EnumUtils tests

2ce8cac

Make method splitting happen on class declarations

6d545db

Method splitting has to happen at the class level so that there is a place to write fields and methods generated by reducing the size of a method

Simplify the test case

6200d82

jduo added 5 commits September 27, 2024 17:08

Fix typo in comment

39e14a4

Revert accidental change

232eb8e

Fix some bugs introduced during refactoring

2887c0d

Fix Javadoc linter errors

fdd0d18

rubenada reviewed Sep 30, 2024

View reviewed changes

julianhyde requested changes Sep 30, 2024

View reviewed changes

jduo added 4 commits October 3, 2024 13:26

PR feedback

80c8587

Clarify testLargeMethod

4a258d8

Add comments about circular dependency concerns

c28f962

Expand on method-splitting tests

4f95aa0

asdfgh19 reviewed Oct 18, 2024

View reviewed changes

CALCITE-6465: Rework code generator to use Flink code splitter #3901

Are you sure you want to change the base?

CALCITE-6465: Rework code generator to use Flink code splitter #3901

Conversation

jduo commented Aug 2, 2024

jduo commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 4, 2024

Quality Gate passed

jduo commented Oct 9, 2024

rubenada commented Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenada commented Oct 16, 2024 •

edited

Loading