From 6ac2d2eb1cf56f5fd80649cba53e3ea8c0e8428a Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 2 Aug 2024 15:42:32 -0700 Subject: [PATCH 01/30] WIP: Integrate Flink code splitter 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 --- bom/build.gradle.kts | 1 + gradle.properties | 1 + linq4j/build.gradle.kts | 3 +++ .../apache/calcite/linq4j/tree/Expressions.java | 17 ++++++++++++++++- 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/bom/build.gradle.kts b/bom/build.gradle.kts index 3128d721513..bdf6ee4fef2 100644 --- a/bom/build.gradle.kts +++ b/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") apiv("org.apache.geode:geode-core") apiv("org.apache.hadoop:hadoop-client", "hadoop") apiv("org.apache.hadoop:hadoop-common", "hadoop") diff --git a/gradle.properties b/gradle.properties index a0004d1fc81..491411b9a03 100644 --- a/gradle.properties +++ b/gradle.properties @@ -102,6 +102,7 @@ dropwizard-metrics.version=4.0.5 # do not upgrade this, new versions are Category X license. elasticsearch.version=7.10.2 embedded-redis.version=0.6 +flink-table-code-splitter.version=1.20.0 jts-core.version=1.19.0 jts-io-common.version=1.19.0 proj4j.version=1.2.2 diff --git a/linq4j/build.gradle.kts b/linq4j/build.gradle.kts index 68770959b53..ee860d6e571 100644 --- a/linq4j/build.gradle.kts +++ b/linq4j/build.gradle.kts @@ -20,4 +20,7 @@ dependencies { implementation("com.google.guava:guava") implementation("org.apache.calcite.avatica:avatica-core") + implementation("org.apache.flink:flink-table-code-splitter") { + exclude("org.apache.flink", "flink-core") + } } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index ebebd2ca692..45fe464a919 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -26,6 +26,8 @@ import com.google.common.collect.ImmutableList; +import org.apache.flink.table.codesplit.JavaCodeSplitter; + import org.checkerframework.checker.nullness.qual.Nullable; import java.lang.reflect.Constructor; @@ -64,7 +66,20 @@ public static String toString(List expressions, String sep, writer.write(expression); writer.append(sep); } - return writer.toString(); + + // From Flink TableConfigOptions.MAX_LENGTH_GENERATED_CODE + // Note that Flink uses 4000 rather than 64KB explicitly: + // "Specifies a threshold where generated code will be split into sub-function calls. + // Java has a maximum method length of 64 KB. This setting allows for finer granularity if necessary + // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with more than 8K byte code." + final int FLINK_DEFAULT_MAX_GENERATED_CODE_LENGTH = 4000; + + // From Flink TableConfigOptions.MAX_MEMBERS_GENERATED_CODE + final int FLINK_DEFAULT_MAX_MEMBERS_GENERATED_CODE = 10000; + String initialCode = writer.toString(); + + // TODO: Use code splitter conditionally based on configuration. + return JavaCodeSplitter.split(initialCode, FLINK_DEFAULT_MAX_GENERATED_CODE_LENGTH, FLINK_DEFAULT_MAX_MEMBERS_GENERATED_CODE); } /** From c63821eb4312dad0a7c9e9de359fde85c14fc74e Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 2 Aug 2024 15:47:25 -0700 Subject: [PATCH 02/30] Remove exclusion of flink-core JavaCodeSplitter needs it for Preconditions --- linq4j/build.gradle.kts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linq4j/build.gradle.kts b/linq4j/build.gradle.kts index ee860d6e571..0634c33e922 100644 --- a/linq4j/build.gradle.kts +++ b/linq4j/build.gradle.kts @@ -21,6 +21,6 @@ dependencies { implementation("com.google.guava:guava") implementation("org.apache.calcite.avatica:avatica-core") implementation("org.apache.flink:flink-table-code-splitter") { - exclude("org.apache.flink", "flink-core") + // exclude("org.apache.flink", "flink-core") } } From 5556360e2cd671dbd43273123b6e9a80dc175c3a Mon Sep 17 00:00:00 2001 From: James Duong Date: Wed, 21 Aug 2024 11:08:25 -0700 Subject: [PATCH 03/30] Fix format violations --- linq4j/build.gradle.kts | 2 +- .../apache/calcite/linq4j/tree/Expressions.java | 17 ++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/linq4j/build.gradle.kts b/linq4j/build.gradle.kts index 0634c33e922..475c1a03cba 100644 --- a/linq4j/build.gradle.kts +++ b/linq4j/build.gradle.kts @@ -21,6 +21,6 @@ dependencies { implementation("com.google.guava:guava") implementation("org.apache.calcite.avatica:avatica-core") implementation("org.apache.flink:flink-table-code-splitter") { - // exclude("org.apache.flink", "flink-core") + // exclude("org.apache.flink", "flink-core") } } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index 45fe464a919..cf0f0d64d36 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -24,10 +24,10 @@ import org.apache.calcite.linq4j.function.Predicate1; import org.apache.calcite.linq4j.function.Predicate2; -import com.google.common.collect.ImmutableList; - import org.apache.flink.table.codesplit.JavaCodeSplitter; +import com.google.common.collect.ImmutableList; + import org.checkerframework.checker.nullness.qual.Nullable; import java.lang.reflect.Constructor; @@ -70,16 +70,19 @@ public static String toString(List expressions, String sep, // From Flink TableConfigOptions.MAX_LENGTH_GENERATED_CODE // Note that Flink uses 4000 rather than 64KB explicitly: // "Specifies a threshold where generated code will be split into sub-function calls. - // Java has a maximum method length of 64 KB. This setting allows for finer granularity if necessary - // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with more than 8K byte code." - final int FLINK_DEFAULT_MAX_GENERATED_CODE_LENGTH = 4000; + // Java has a maximum method length of 64 KB. This setting allows for finer granularity if + // necessary. + // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with + // more than 8K byte code." + final int flinkDefaultMaxGeneratedCodeLength = 4000; // From Flink TableConfigOptions.MAX_MEMBERS_GENERATED_CODE - final int FLINK_DEFAULT_MAX_MEMBERS_GENERATED_CODE = 10000; + final int flinkDefaultMaxMembersGeneratedCode = 10000; String initialCode = writer.toString(); // TODO: Use code splitter conditionally based on configuration. - return JavaCodeSplitter.split(initialCode, FLINK_DEFAULT_MAX_GENERATED_CODE_LENGTH, FLINK_DEFAULT_MAX_MEMBERS_GENERATED_CODE); + return JavaCodeSplitter.split(initialCode, flinkDefaultMaxGeneratedCodeLength, + flinkDefaultMaxMembersGeneratedCode); } /** From d3cf335f72786f59af38e38059eed384543b12ce Mon Sep 17 00:00:00 2001 From: James Duong Date: Wed, 21 Aug 2024 15:56:59 -0700 Subject: [PATCH 04/30] Refactor so that only methods get code-split --- .../calcite/linq4j/tree/ExpressionWriter.java | 8 ++++++ .../calcite/linq4j/tree/Expressions.java | 19 +------------- .../linq4j/tree/MethodDeclaration.java | 26 ++++++++++++++++--- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java index 8bc640bac6e..546f7905b0d 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java @@ -33,6 +33,7 @@ class ExpressionWriter { private final Spacer spacer = new Spacer(0); private final StringBuilder buf = new StringBuilder(); private boolean indentPending; + private final boolean generics; ExpressionWriter() { @@ -43,6 +44,13 @@ class ExpressionWriter { this.generics = generics; } + public ExpressionWriter duplicateState() { + final ExpressionWriter writer = new ExpressionWriter(this.generics); + writer.indentPending = this.indentPending; + writer.spacer.add(this.spacer.get()); + return writer; + } + public void write(Node expression) { if (expression instanceof Expression) { Expression expression1 = (Expression) expression; diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index cf0f0d64d36..2822822a92f 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -24,8 +24,6 @@ import org.apache.calcite.linq4j.function.Predicate1; import org.apache.calcite.linq4j.function.Predicate2; -import org.apache.flink.table.codesplit.JavaCodeSplitter; - import com.google.common.collect.ImmutableList; import org.checkerframework.checker.nullness.qual.Nullable; @@ -67,22 +65,7 @@ public static String toString(List expressions, String sep, writer.append(sep); } - // From Flink TableConfigOptions.MAX_LENGTH_GENERATED_CODE - // Note that Flink uses 4000 rather than 64KB explicitly: - // "Specifies a threshold where generated code will be split into sub-function calls. - // Java has a maximum method length of 64 KB. This setting allows for finer granularity if - // necessary. - // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with - // more than 8K byte code." - final int flinkDefaultMaxGeneratedCodeLength = 4000; - - // From Flink TableConfigOptions.MAX_MEMBERS_GENERATED_CODE - final int flinkDefaultMaxMembersGeneratedCode = 10000; - String initialCode = writer.toString(); - - // TODO: Use code splitter conditionally based on configuration. - return JavaCodeSplitter.split(initialCode, flinkDefaultMaxGeneratedCodeLength, - flinkDefaultMaxMembersGeneratedCode); + return writer.toString(); } /** diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 444da129d5a..c20fbff2850 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -16,6 +16,8 @@ */ package org.apache.calcite.linq4j.tree; +import org.apache.flink.table.codesplit.JavaCodeSplitter; + import org.checkerframework.checker.nullness.qual.Nullable; import java.lang.reflect.Modifier; @@ -57,13 +59,14 @@ public MethodDeclaration(int modifier, String name, Type resultType, } @Override public void accept(ExpressionWriter writer) { + final ExpressionWriter tempWriter = writer.duplicateState(); String modifiers = Modifier.toString(modifier); - writer.append(modifiers); + tempWriter.append(modifiers); if (!modifiers.isEmpty()) { - writer.append(' '); + tempWriter.append(' '); } //noinspection unchecked - writer + tempWriter .append(resultType) .append(' ') .append(name) @@ -71,6 +74,23 @@ public MethodDeclaration(int modifier, String name, Type resultType, () -> (Iterator) parameters.stream().map(ParameterExpression::declString).iterator()) .append(' ') .append(body); + + // From Flink TableConfigOptions.MAX_LENGTH_GENERATED_CODE + // Note that Flink uses 4000 rather than 64KB explicitly: + // "Specifies a threshold where generated code will be split into sub-function calls. + // Java has a maximum method length of 64 KB. This setting allows for finer granularity if + // necessary. + // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with + // more than 8K byte code." + final int flinkDefaultMaxGeneratedCodeLength = 4000; + + // From Flink TableConfigOptions.MAX_MEMBERS_GENERATED_CODE + final int flinkDefaultMaxMembersGeneratedCode = 10000; + + // TODO: Use code splitter conditionally based on configuration. + writer.append( + JavaCodeSplitter.split(tempWriter.toString(), flinkDefaultMaxGeneratedCodeLength, + flinkDefaultMaxMembersGeneratedCode)); writer.newlineAndIndent(); } From 739c747f2f77d512a6e42b0cd68ec10285b8151f Mon Sep 17 00:00:00 2001 From: James Duong Date: Wed, 21 Aug 2024 15:57:50 -0700 Subject: [PATCH 05/30] Remove extra newline --- .../main/java/org/apache/calcite/linq4j/tree/Expressions.java | 1 - 1 file changed, 1 deletion(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index 2822822a92f..ebebd2ca692 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -64,7 +64,6 @@ public static String toString(List expressions, String sep, writer.write(expression); writer.append(sep); } - return writer.toString(); } From e9e20e1e5aaeaada1369a83fc9dacd9b63863414 Mon Sep 17 00:00:00 2001 From: James Duong Date: Tue, 10 Sep 2024 15:34:48 -0700 Subject: [PATCH 06/30] Update calls to Expressions.toString() --- .../enumerable/EnumerableInterpretable.java | 2 +- .../calcite/config/CalciteSystemProperty.java | 3 + .../interpreter/JaninoRexCompiler.java | 2 +- .../apache/calcite/rex/RexExecutorImpl.java | 2 +- .../adapter/enumerable/EnumUtilsTest.java | 48 ++--- .../adapter/enumerable/PhysTypeTest.java | 2 +- .../adapter/enumerable/TypeFinderTest.java | 2 +- .../innodb/InnodbToEnumerableConverter.java | 2 +- .../calcite/linq4j/tree/AbstractNode.java | 2 +- .../calcite/linq4j/tree/ExpressionWriter.java | 8 +- .../calcite/linq4j/tree/Expressions.java | 8 +- .../calcite/linq4j/test/BlockBuilderTest.java | 8 +- .../calcite/linq4j/test/ExpressionTest.java | 196 +++++++++--------- .../calcite/linq4j/test/InlinerTest.java | 8 +- .../calcite/linq4j/test/Linq4jTest.java | 20 +- .../enumerable/CodeGenerationBenchmark.java | 2 +- 16 files changed, 160 insertions(+), 155 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java index 5f32ab1c710..92bb10cf304 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java @@ -111,7 +111,7 @@ public static Bindable toBindable(Map 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, true); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); diff --git a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java index ebd7a5c02d1..0dde1efe005 100644 --- a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java +++ b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java @@ -420,6 +420,9 @@ public final class CalciteSystemProperty { public static final CalciteSystemProperty FUNCTION_LEVEL_CACHE_MAX_SIZE = intProperty("calcite.function.cache.maxSize", 0, v -> v >= 0); + public static final CalciteSystemProperty ENABLE_CODE_SPLITTING = + booleanProperty("calcite.linq.enable_code_splitting", false); + private static CalciteSystemProperty booleanProperty(String key, boolean defaultValue) { // Note that "" -> true (convenient for command-lines flags like '-Dflag') diff --git a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java b/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java index bca4f85ef50..2a55cf7c650 100644 --- a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java +++ b/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java @@ -186,7 +186,7 @@ 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, true); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); } diff --git a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java index b32399b02f1..676c67a032b 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java +++ b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java @@ -100,7 +100,7 @@ private static String compile(RexBuilder rexBuilder, List 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, true); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, code); } diff --git a/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java b/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java index 267aa39ca16..83369e330fc 100644 --- a/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java +++ b/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java @@ -47,9 +47,9 @@ public final class EnumUtilsTest { EnumUtils.convert(date, int.class); final Expression dateToInteger = EnumUtils.convert(date, Integer.class); - assertThat(Expressions.toString(dateToInt), + assertThat(Expressions.toString(dateToInt, true), is("org.apache.calcite.runtime.SqlFunctions.toInt(x)")); - assertThat(Expressions.toString(dateToInteger), + assertThat(Expressions.toString(dateToInteger, true), is("org.apache.calcite.runtime.SqlFunctions.toIntOptional(x)")); // java.sql.Time x; @@ -59,9 +59,9 @@ public final class EnumUtilsTest { EnumUtils.convert(time, int.class); final Expression timeToInteger = EnumUtils.convert(time, Integer.class); - assertThat(Expressions.toString(timeToInt), + assertThat(Expressions.toString(timeToInt, true), is("org.apache.calcite.runtime.SqlFunctions.toInt(x)")); - assertThat(Expressions.toString(timeToInteger), + assertThat(Expressions.toString(timeToInteger, true), is("org.apache.calcite.runtime.SqlFunctions.toIntOptional(x)")); // java.sql.TimeStamp x; @@ -71,9 +71,9 @@ public final class EnumUtilsTest { EnumUtils.convert(timestamp, long.class); final Expression timeStampToLong = EnumUtils.convert(timestamp, Long.class); - assertThat(Expressions.toString(timeStampToLongPrimitive), + assertThat(Expressions.toString(timeStampToLongPrimitive, true), is("org.apache.calcite.runtime.SqlFunctions.toLong(x)")); - assertThat(Expressions.toString(timeStampToLong), + assertThat(Expressions.toString(timeStampToLong, true), is("org.apache.calcite.runtime.SqlFunctions.toLongOptional(x)")); } @@ -86,7 +86,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, byte.class); final Expression converted0 = EnumUtils.convert(bytePrimitiveConverted, Byte.class); - assertThat(Expressions.toString(converted0), + assertThat(Expressions.toString(converted0, true), is("Byte.valueOf((byte) intV)")); // (char)(int) -> Character: Character.valueOf((char) intV) @@ -94,7 +94,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, char.class); final Expression converted1 = EnumUtils.convert(characterPrimitiveConverted, Character.class); - assertThat(Expressions.toString(converted1), + assertThat(Expressions.toString(converted1, true), is("Character.valueOf((char) intV)")); // (short)(int) -> Short: Short.valueOf((short) intV) @@ -102,7 +102,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, short.class); final Expression converted2 = EnumUtils.convert(shortPrimitiveConverted, Short.class); - assertThat(Expressions.toString(converted2), + assertThat(Expressions.toString(converted2, true), is("Short.valueOf((short) intV)")); // (long)(int) -> Long: Long.valueOf(intV) @@ -110,7 +110,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, long.class); final Expression converted3 = EnumUtils.convert(longPrimitiveConverted, Long.class); - assertThat(Expressions.toString(converted3), + assertThat(Expressions.toString(converted3, true), is("Long.valueOf(intV)")); // (float)(int) -> Float: Float.valueOf(intV) @@ -118,7 +118,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, float.class); final Expression converted4 = EnumUtils.convert(floatPrimitiveConverted, Float.class); - assertThat(Expressions.toString(converted4), + assertThat(Expressions.toString(converted4, true), is("Float.valueOf(intV)")); // (double)(int) -> Double: Double.valueOf(intV) @@ -126,37 +126,37 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, double.class); final Expression converted5 = EnumUtils.convert(doublePrimitiveConverted, Double.class); - assertThat(Expressions.toString(converted5), + assertThat(Expressions.toString(converted5, true), is("Double.valueOf(intV)")); final Expression byteConverted = EnumUtils.convert(intVariable, Byte.class); - assertThat(Expressions.toString(byteConverted), + assertThat(Expressions.toString(byteConverted, true), is("Byte.valueOf((byte) intV)")); final Expression shortConverted = EnumUtils.convert(intVariable, Short.class); - assertThat(Expressions.toString(shortConverted), + assertThat(Expressions.toString(shortConverted, true), is("Short.valueOf((short) intV)")); final Expression integerConverted = EnumUtils.convert(intVariable, Integer.class); - assertThat(Expressions.toString(integerConverted), + assertThat(Expressions.toString(integerConverted, true), is("Integer.valueOf(intV)")); final Expression longConverted = EnumUtils.convert(intVariable, Long.class); - assertThat(Expressions.toString(longConverted), + assertThat(Expressions.toString(longConverted, true), is("Long.valueOf((long) intV)")); final Expression floatConverted = EnumUtils.convert(intVariable, Float.class); - assertThat(Expressions.toString(floatConverted), + assertThat(Expressions.toString(floatConverted, true), is("Float.valueOf((float) intV)")); final Expression doubleConverted = EnumUtils.convert(intVariable, Double.class); - assertThat(Expressions.toString(doubleConverted), + assertThat(Expressions.toString(doubleConverted, true), is("Double.valueOf((double) intV)")); } @@ -167,8 +167,8 @@ public final class EnumUtilsTest { final ConstantExpression nullLiteral2 = Expressions.constant(null, Object.class); final Expression e1 = EnumUtils.convert(nullLiteral1, String.class); final Expression e2 = EnumUtils.convert(nullLiteral2, String.class); - assertThat(Expressions.toString(e1), is("(String) null")); - assertThat(Expressions.toString(e2), is("(String) (Object) null")); + assertThat(Expressions.toString(e1, true), is("(String) null")); + assertThat(Expressions.toString(e2, true), is("(String) (Object) null")); } @Test void testMethodCallExpression() { @@ -178,7 +178,7 @@ public final class EnumUtilsTest { final MethodCallExpression arrayMethodCall = EnumUtils.call(null, SqlFunctions.class, BuiltInMethod.ARRAY.getMethodName(), Arrays.asList(arg0, arg1)); - assertThat(Expressions.toString(arrayMethodCall), + assertThat(Expressions.toString(arrayMethodCall, true), is("org.apache.calcite.runtime.SqlFunctions.array(1, \"x\")")); // test for Object.class argument type @@ -187,7 +187,7 @@ public final class EnumUtilsTest { EnumUtils.call(null, XmlFunctions.class, BuiltInMethod.EXTRACT_VALUE.getMethodName(), Arrays.asList(arg1, nullLiteral)); - assertThat(Expressions.toString(xmlExtractMethodCall), + assertThat(Expressions.toString(xmlExtractMethodCall, true), is("org.apache.calcite.runtime.XmlFunctions.extractValue(\"x\", (String) null)")); // test "mod(decimal, long)" match to "mod(decimal, decimal)" @@ -196,7 +196,7 @@ public final class EnumUtilsTest { final MethodCallExpression modMethodCall = EnumUtils.call(null, SqlFunctions.class, "mod", Arrays.asList(arg2, arg3)); - assertThat(Expressions.toString(modMethodCall), + assertThat(Expressions.toString(modMethodCall, true), is("org.apache.calcite.runtime.SqlFunctions.mod(" + "java.math.BigDecimal.valueOf(125L, 1), " + "new java.math.BigDecimal(\n 3L))")); @@ -207,7 +207,7 @@ public final class EnumUtilsTest { final MethodCallExpression geoMethodCall = EnumUtils.call(null, SpatialTypeFunctions.class, "ST_MakePoint", Arrays.asList(arg4, arg5)); - assertThat(Expressions.toString(geoMethodCall), + assertThat(Expressions.toString(geoMethodCall, true), is("org.apache.calcite.runtime.SpatialTypeFunctions.ST_MakePoint(" + "new java.math.BigDecimal(\n 1), " + "new java.math.BigDecimal(\n 2))")); diff --git a/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java b/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java index 98dc6a9cd1e..e094d43985e 100644 --- a/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java +++ b/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java @@ -95,6 +95,6 @@ public final class PhysTypeTest { + " }\n" + "}\n" + ")"; - assertEquals(Expressions.toString(e), expected); + assertEquals(Expressions.toString(e, true), expected); } } diff --git a/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java b/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java index 583bcd8d670..2ac9b616dab 100644 --- a/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java +++ b/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java @@ -97,7 +97,7 @@ private void assertJavaCodeContains(String expected, Node node) { } private void assertJavaCodeContains(String expected, List nodes) { - final String javaCode = Expressions.toString(nodes, "\n", false); + final String javaCode = Expressions.toString(nodes, "\n", false, true); assertThat(javaCode, containsString(expected)); } diff --git a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java index 858ddbb3a0d..545b1432c82 100644 --- a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java +++ b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java @@ -150,7 +150,7 @@ static List innodbFieldNames(final RelDataType rowType) { InnodbMethod.INNODB_QUERYABLE_QUERY.method, fields, selectFields, cond, ascOrder)); if (CalciteSystemProperty.DEBUG.value()) { - System.out.println("Innodb: " + Expressions.toString(enumerable)); + System.out.println("Innodb: " + Expressions.toString(enumerable, true)); } list.add(Expressions.return_(null, enumerable)); return implementor.result(physType, list.toBlock()); diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java index a6024112386..58bc4134c38 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java +++ b/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); accept(writer, 0, 0); return writer.toString(); } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java index 546f7905b0d..eadfda003af 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java @@ -35,17 +35,19 @@ class ExpressionWriter { private boolean indentPending; private final boolean generics; + private final boolean methodSplitting; ExpressionWriter() { - this(true); + this(true, true); } - ExpressionWriter(boolean generics) { + ExpressionWriter(boolean generics, boolean methodSplitting) { this.generics = generics; + this.methodSplitting = methodSplitting; } public ExpressionWriter duplicateState() { - final ExpressionWriter writer = new ExpressionWriter(this.generics); + final ExpressionWriter writer = new ExpressionWriter(this.generics, this.methodSplitting); writer.indentPending = this.indentPending; writer.spacer.add(this.spacer.get()); return writer; diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index ebebd2ca692..88535507673 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -58,8 +58,8 @@ private Expressions() {} * extra type information in generics. */ public static String toString(List expressions, String sep, - boolean generics) { - final ExpressionWriter writer = new ExpressionWriter(generics); + boolean generics, boolean methodSplit) { + final ExpressionWriter writer = new ExpressionWriter(generics, methodSplit); for (Node expression : expressions) { writer.write(expression); writer.append(sep); @@ -70,8 +70,8 @@ public static String toString(List expressions, String sep, /** * Converts an expression to Java source code. */ - public static String toString(Node expression) { - return toString(Collections.singletonList(expression), "", true); + public static String toString(Node expression, boolean methodSplit) { + return toString(Collections.singletonList(expression), "", true, methodSplit); } /** diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java index 07023ae48df..50e05b3755a 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java @@ -108,7 +108,7 @@ private BlockBuilder appendBlockWithSameVariable( + " x = 1;\n" + " int x0;\n" + " x0 = 42;\n" - + "}\n", Expressions.toString(outer.toBlock()), + + "}\n", Expressions.toString(outer.toBlock(), false), "x in the second block should be renamed to avoid name clash"); } @@ -122,7 +122,7 @@ private BlockBuilder appendBlockWithSameVariable( + " x = 1;\n" + " int x0 = 8;\n" + " x0 = 42;\n" - + "}\n", Expressions.toString(outer.toBlock()), + + "}\n", Expressions.toString(outer.toBlock(), false), "x in the second block should be renamed to avoid name clash"); } @@ -143,7 +143,7 @@ private BlockBuilder appendBlockWithSameVariable( + " final Object _i = new org.apache.calcite.linq4j.test.BlockBuilderTest.Identity()" + ".apply(\"test\");\n" + "}\n", - Expressions.toString(bb.toBlock())); + Expressions.toString(bb.toBlock(), false)); } @@ -160,7 +160,7 @@ private BlockBuilder appendBlockWithSameVariable( assertEquals("{\n" + " return false;\n" - + "}\n", Expressions.toString(outer.toBlock()), + + "}\n", Expressions.toString(outer.toBlock(), false), "Expected to optimize Boolean.FALSE = null to false"); } diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index 52577617583..0ad71669bb3 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -90,7 +90,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(int arg) {\n" @@ -133,7 +133,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(short arg) {\n" @@ -176,7 +176,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(byte arg) {\n" @@ -218,7 +218,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public double apply(double arg) {\n" @@ -259,7 +259,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(2L)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public long apply(long arg) {\n" @@ -300,7 +300,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(2.0f)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public float apply(float arg) {\n" @@ -341,7 +341,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(10)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public long apply(long arg) {\n" @@ -382,7 +382,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(10.1d)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public double apply(double arg) {\n" @@ -425,7 +425,7 @@ public class ExpressionTest { Arrays.asList(paramExpr, param2Expr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr); + String s = Expressions.toString(lambdaExpr, false); assertEquals("new org.apache.calcite.linq4j.function.Function2() {\n" + " public int apply(int key, int key2) {\n" + " return key;\n" @@ -477,11 +477,11 @@ public class ExpressionTest { assertEquals( "true", Expressions.toString( - Expressions.foldAnd(list0))); + Expressions.foldAnd(list0), false)); assertEquals( "false", Expressions.toString( - Expressions.foldOr(list0))); + Expressions.foldOr(list0), false)); final List list1 = Arrays.asList( @@ -494,12 +494,12 @@ public class ExpressionTest { assertEquals( "1 == 2 && 3 == 4 && 5 == 6", Expressions.toString( - Expressions.foldAnd(list1))); + Expressions.foldAnd(list1), false)); // a single true makes OR true assertEquals( "true", Expressions.toString( - Expressions.foldOr(list1))); + Expressions.foldOr(list1), false)); final List list2 = Collections.singletonList( @@ -507,11 +507,11 @@ public class ExpressionTest { assertEquals( "true", Expressions.toString( - Expressions.foldAnd(list2))); + Expressions.foldAnd(list2), false)); assertEquals( "true", Expressions.toString( - Expressions.foldOr(list2))); + Expressions.foldOr(list2), false)); final List list3 = Arrays.asList( @@ -523,11 +523,11 @@ public class ExpressionTest { assertEquals( "false", Expressions.toString( - Expressions.foldAnd(list3))); + Expressions.foldAnd(list3), false)); assertEquals( "1 == 2 || 5 == 6", Expressions.toString( - Expressions.foldOr(list3))); + Expressions.foldOr(list3), false)); } @Test void testWrite() { @@ -540,13 +540,13 @@ public class ExpressionTest { Expressions.constant(1), Expressions.constant(2F, Float.TYPE)), Expressions.constant(3L, Long.TYPE)), - Expressions.constant(4L, Long.class)))); + Expressions.constant(4L, Long.class)), false)); assertEquals( "java.math.BigDecimal.valueOf(31415926L, 7)", Expressions.toString( Expressions.constant( - BigDecimal.valueOf(314159260, 8)))); + BigDecimal.valueOf(314159260, 8)), false)); // Parentheses needed, to override the left-associativity of +. assertEquals( @@ -556,7 +556,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.add( Expressions.constant(2), - Expressions.constant(3))))); + Expressions.constant(3))), false)); // No parentheses needed; higher precedence of * achieves the desired // effect. @@ -567,7 +567,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.multiply( Expressions.constant(2), - Expressions.constant(3))))); + Expressions.constant(3))), false)); assertEquals( "1 * (2 + 3)", @@ -576,7 +576,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.add( Expressions.constant(2), - Expressions.constant(3))))); + Expressions.constant(3))), false)); // Parentheses needed, to overcome right-associativity of =. assertEquals( @@ -585,7 +585,7 @@ public class ExpressionTest { Expressions.assign( Expressions.assign( Expressions.constant(1), Expressions.constant(2)), - Expressions.constant(3)))); + Expressions.constant(3)), false)); // Ternary operator. assertEquals( @@ -606,7 +606,7 @@ public class ExpressionTest { Expressions.constant(7), Expressions.constant(8)), Expressions.constant(9), - Expressions.constant(10))))); + Expressions.constant(10))), false)); assertEquals( "0 + (double) (2 + 3)", @@ -616,7 +616,7 @@ public class ExpressionTest { Expressions.convert_( Expressions.add( Expressions.constant(2), Expressions.constant(3)), - Double.TYPE)))); + Double.TYPE)), false)); // "--5" would be a syntax error assertEquals( @@ -624,27 +624,27 @@ public class ExpressionTest { Expressions.toString( Expressions.negate( Expressions.negate( - Expressions.constant(5))))); + Expressions.constant(5))), false)); assertEquals( "a.empno", Expressions.toString( Expressions.field( Expressions.parameter(Linq4jTest.Employee.class, "a"), - "empno"))); + "empno"), false)); assertEquals( "a.length", Expressions.toString( Expressions.field( Expressions.parameter(Object[].class, "a"), - "length"))); + "length"), false)); assertEquals( "java.util.Collections.EMPTY_LIST", Expressions.toString( Expressions.field( - null, Collections.class, "EMPTY_LIST"))); + null, Collections.class, "EMPTY_LIST"), false)); final ParameterExpression paramX = Expressions.parameter(String.class, "x"); @@ -663,7 +663,7 @@ public class ExpressionTest { Function1.class, Expressions.call( paramX, "length", Collections.emptyList()), - Arrays.asList(paramX)))); + Arrays.asList(paramX)), false)); // 1-dimensional array with initializer assertEquals( @@ -676,7 +676,7 @@ public class ExpressionTest { String.class, Expressions.constant("foo"), Expressions.constant(null), - Expressions.constant("bar\"baz")))); + Expressions.constant("bar\"baz")), false)); // 2-dimensional array with initializer assertEquals( @@ -693,7 +693,7 @@ public class ExpressionTest { 2, Expressions.constant(new String[] {"foo", "bar"}), Expressions.constant(null), - Expressions.constant(new String[] {null})))); + Expressions.constant(new String[] {null})), false)); // 1-dimensional array assertEquals( @@ -704,7 +704,7 @@ public class ExpressionTest { 1, Expressions.add( Expressions.parameter(0, int.class, "x"), - Expressions.constant(1))))); + Expressions.constant(1))), false)); // 3-dimensional array assertEquals( @@ -715,7 +715,7 @@ public class ExpressionTest { 3, Expressions.add( Expressions.parameter(0, int.class, "x"), - Expressions.constant(1))))); + Expressions.constant(1))), false)); assertEquals( "(int) ((String) (Object) \"foo\").length()", @@ -729,7 +729,7 @@ public class ExpressionTest { String.class), "length", Collections.emptyList()), - Integer.TYPE))); + Integer.TYPE), false)); // resolving a static method assertEquals( @@ -739,7 +739,7 @@ public class ExpressionTest { Integer.class, "valueOf", Collections.singletonList( - Expressions.constant("0123"))))); + Expressions.constant("0123"))), false)); // precedence of not and instanceof assertEquals( @@ -748,7 +748,7 @@ public class ExpressionTest { Expressions.not( Expressions.typeIs( Expressions.parameter(Object.class, "o"), - String.class)))); + String.class)), false)); // not not assertEquals( @@ -758,7 +758,7 @@ public class ExpressionTest { Expressions.not( Expressions.typeIs( Expressions.parameter(Object.class, "o"), - String.class))))); + String.class))), false)); } @Test void testWriteConstant() { @@ -769,65 +769,65 @@ public class ExpressionTest { + " 2,\n" + " -1}", Expressions.toString( - Expressions.constant(new int[]{1, 2, -1}))); + Expressions.constant(new int[]{1, 2, -1}), false)); // primitive assertEquals( "-12", Expressions.toString( - Expressions.constant(-12))); + Expressions.constant(-12), false)); assertEquals( "(short)-12", Expressions.toString( - Expressions.constant((short) -12))); + Expressions.constant((short) -12), false)); assertEquals( "(byte)-12", Expressions.toString( - Expressions.constant((byte) -12))); + Expressions.constant((byte) -12), false)); // boxed primitives assertEquals( "Integer.valueOf(1)", Expressions.toString( - Expressions.constant(1, Integer.class))); + Expressions.constant(1, Integer.class), false)); assertEquals( "Double.valueOf(-3.14D)", Expressions.toString( - Expressions.constant(-3.14, Double.class))); + Expressions.constant(-3.14, Double.class), false)); assertEquals( "Boolean.valueOf(true)", Expressions.toString( - Expressions.constant(true, Boolean.class))); + Expressions.constant(true, Boolean.class), false)); // primitive with explicit class assertEquals( "1", Expressions.toString( - Expressions.constant(1, int.class))); + Expressions.constant(1, int.class), false)); assertEquals( "(short)1", Expressions.toString( - Expressions.constant(1, short.class))); + Expressions.constant(1, short.class), false)); assertEquals( "(byte)1", Expressions.toString( - Expressions.constant(1, byte.class))); + Expressions.constant(1, byte.class), false)); assertEquals( "-3.14D", Expressions.toString( - Expressions.constant(-3.14, double.class))); + Expressions.constant(-3.14, double.class), false)); assertEquals( "true", Expressions.toString( - Expressions.constant(true, boolean.class))); + Expressions.constant(true, boolean.class), false)); // objects and nulls assertEquals( @@ -835,19 +835,19 @@ public class ExpressionTest { + " \"foo\",\n" + " null}", Expressions.toString( - Expressions.constant(new String[] {"foo", null}))); + Expressions.constant(new String[] {"foo", null}), false)); // string assertEquals( "\"hello, \\\"world\\\"!\"", Expressions.toString( - Expressions.constant("hello, \"world\"!"))); + Expressions.constant("hello, \"world\"!"), false)); // enum assertEquals( "org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.X", Expressions.toString( - Expressions.constant(MyEnum.X))); + Expressions.constant(MyEnum.X), false)); // array of enum assertEquals( @@ -855,24 +855,24 @@ public class ExpressionTest { + " org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.X,\n" + " org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.Y}", Expressions.toString( - Expressions.constant(new MyEnum[]{MyEnum.X, MyEnum.Y}))); + Expressions.constant(new MyEnum[]{MyEnum.X, MyEnum.Y}), false)); // class assertEquals( "java.lang.String.class", Expressions.toString( - Expressions.constant(String.class))); + Expressions.constant(String.class), false)); // array class assertEquals( "int[].class", Expressions.toString( - Expressions.constant(int[].class))); + Expressions.constant(int[].class), false)); assertEquals( "java.util.List[][].class", Expressions.toString( - Expressions.constant(List[][].class))); + Expressions.constant(List[][].class), false)); // automatically call constructor if it matches fields assertEquals( @@ -894,7 +894,7 @@ public class ExpressionTest { + " \"Janet\",\n" + " 10)}", Expressions.toString( - Expressions.constant(Linq4jTest.emps))); + Expressions.constant(Linq4jTest.emps), false)); } /** Test case for @@ -919,7 +919,7 @@ public class ExpressionTest { ImmutableSet.of(createInstance(recordClass, "test1", 1), createInstance(recordClass, "test2", 2), createInstance(recordClass, "test3", 3), - createInstance(recordClass, "test4", 4))))); + createInstance(recordClass, "test4", 4))), false)); } @Test void testWriteArray() { @@ -932,7 +932,7 @@ public class ExpressionTest { Expressions.variable(int[].class, "integers"), Expressions.add( Expressions.constant(2), - Expressions.variable(int.class, "index")))))); + Expressions.variable(int.class, "index")))), false)); } @Test void testWriteAnonymousClass() { @@ -1016,7 +1016,7 @@ public class ExpressionTest { + "\n" + " };\n" + "}\n", - Expressions.toString(e)); + Expressions.toString(e, false)); } @Test void testWriteWhile() { @@ -1071,7 +1071,7 @@ public class ExpressionTest { + "} finally {\n" + " \"foo\".toUpperCase();\n" + "}\n", - Expressions.toString(node)); + Expressions.toString(node, false)); } @Test void testWriteTryFinally() { @@ -1096,7 +1096,7 @@ public class ExpressionTest { + " \"foo\".toUpperCase();\n" + " }\n" + "}\n", - Expressions.toString(node)); + Expressions.toString(node, false)); } @Test void testWriteTryCatch() { @@ -1122,7 +1122,7 @@ public class ExpressionTest { + "} catch (RuntimeException re) {\n" + " return re.toString();\n" + "}\n", - Expressions.toString(node)); + Expressions.toString(node, false)); } @Test void testType() { @@ -1237,7 +1237,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { nine)); statements.add(Expressions.return_(null, eighteen)); BlockStatement expression = statements.toBlock(); - assertEquals(expected, Expressions.toString(expression)); + assertEquals(expected, Expressions.toString(expression, false)); expression.accept(new Shuttle()); } @@ -1268,7 +1268,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " (java.util.Comparator) null);\n" + " return treeSet.add(null);\n" + "}\n"; - assertThat(Expressions.toString(expression), is(expected)); + assertThat(Expressions.toString(expression, false), is(expected)); expression.accept(new Shuttle()); } @@ -1317,7 +1317,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " final int _b0 = 1 + 3;\n" + " org.apache.calcite.linq4j.test.ExpressionTest.bar(1, _b, _c, _d, org.apache.calcite.linq4j.test.ExpressionTest.foo(_b0));\n" + "}\n", - Expressions.toString(expression)); + Expressions.toString(expression, false)); expression.accept(new Shuttle()); } @@ -1366,32 +1366,32 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testBigDecimalConstantExpression() { assertEquals("java.math.BigDecimal.valueOf(104L)", - Expressions.toString(Expressions.constant("104", BigDecimal.class))); + Expressions.toString(Expressions.constant("104", BigDecimal.class), false)); assertEquals("java.math.BigDecimal.valueOf(1L, -3)", - Expressions.toString(Expressions.constant("1000", BigDecimal.class))); + Expressions.toString(Expressions.constant("1000", BigDecimal.class), false)); assertEquals("java.math.BigDecimal.valueOf(1L, -3)", - Expressions.toString(Expressions.constant(1000, BigDecimal.class))); + Expressions.toString(Expressions.constant(1000, BigDecimal.class), false)); assertEquals("java.math.BigDecimal.valueOf(107L)", - Expressions.toString(Expressions.constant(107, BigDecimal.class))); + Expressions.toString(Expressions.constant(107, BigDecimal.class), false)); assertEquals("java.math.BigDecimal.valueOf(199999999999999L)", - Expressions.toString(Expressions.constant(199999999999999L, BigDecimal.class))); + Expressions.toString(Expressions.constant(199999999999999L, BigDecimal.class), false)); assertEquals("java.math.BigDecimal.valueOf(1234L, 2)", - Expressions.toString(Expressions.constant(12.34, BigDecimal.class))); + Expressions.toString(Expressions.constant(12.34, BigDecimal.class), false)); } @Test void testObjectConstantExpression() { assertEquals("(byte)100", - Expressions.toString(Expressions.constant((byte) 100, Object.class))); + Expressions.toString(Expressions.constant((byte) 100, Object.class), false)); assertEquals("(char)100", - Expressions.toString(Expressions.constant((char) 100, Object.class))); + Expressions.toString(Expressions.constant((char) 100, Object.class), false)); assertEquals("(short)100", - Expressions.toString(Expressions.constant((short) 100, Object.class))); + Expressions.toString(Expressions.constant((short) 100, Object.class), false)); assertEquals("100L", - Expressions.toString(Expressions.constant(100L, Object.class))); + Expressions.toString(Expressions.constant(100L, Object.class), false)); assertEquals("100.0F", - Expressions.toString(Expressions.constant(100F, Object.class))); + Expressions.toString(Expressions.constant(100F, Object.class), false)); assertEquals("100.0D", - Expressions.toString(Expressions.constant(100D, Object.class))); + Expressions.toString(Expressions.constant(100D, Object.class), false)); } @Test void testClassDecl() { @@ -1425,7 +1425,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " }\n" + " int i;\n" + "}", - Expressions.toString(newExpression)); + Expressions.toString(newExpression, false)); newExpression.accept(new Shuttle()); } @@ -1440,7 +1440,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.ifThenElse( Expressions.constant(true), Expressions.return_(null), - Expressions.return_(null, Expressions.constant(1))))); + Expressions.return_(null, Expressions.constant(1))), false)); } @Test void testIfElseIfElse() { @@ -1458,7 +1458,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.return_(null), Expressions.constant(false), Expressions.return_(null), - Expressions.return_(null, Expressions.constant(1))))); + Expressions.return_(null, Expressions.constant(1))), false)); } /** Test for common sub-expression elimination. */ @@ -1505,7 +1505,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " return (Number) v == null ? null : (" + "(Number) v).intValue() == 1997;\n" + "}\n", - Expressions.toString(builder.toBlock())); + Expressions.toString(builder.toBlock(), false)); } @Test void testFor() throws NoSuchFieldException { @@ -1530,7 +1530,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " System.out.println(i);\n" + " }\n" + "}\n", - Expressions.toString(builder.toBlock())); + Expressions.toString(builder.toBlock(), false)); } @Test void testFor2() { @@ -1560,7 +1560,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " }\n" + " }\n" + "}\n", - Expressions.toString(builder.toBlock())); + Expressions.toString(builder.toBlock(), false)); } @Test void testForEach() { @@ -1574,7 +1574,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.constant(1), Expressions.constant(2)), Expressions.break_(null)))); - assertThat(Expressions.toString(builder.toBlock()), + assertThat(Expressions.toString(builder.toBlock(), false), is("{\n" + " for (int i : list) {\n" + " if (1 < 2) {\n" @@ -1586,18 +1586,18 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testEmptyListLiteral() { assertEquals("java.util.Collections.EMPTY_LIST", - Expressions.toString(Expressions.constant(Arrays.asList()))); + Expressions.toString(Expressions.constant(Arrays.asList()), false)); } @Test void testOneElementListLiteral() { assertEquals("java.util.Arrays.asList(1)", - Expressions.toString(Expressions.constant(Arrays.asList(1)))); + Expressions.toString(Expressions.constant(Arrays.asList(1)), false)); } @Test void testTwoElementsListLiteral() { assertEquals("java.util.Arrays.asList(1,\n" + " 2)", - Expressions.toString(Expressions.constant(Arrays.asList(1, 2)))); + Expressions.toString(Expressions.constant(Arrays.asList(1, 2)), false)); } @Test void testNestedListsLiteral() { @@ -1607,7 +1607,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " 4))", Expressions.toString( Expressions.constant( - Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4))))); + Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4))), false)); } @Test void testEmptyMapLiteral() { @@ -1617,13 +1617,13 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testOneElementMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of(\"abc\", 42)", - Expressions.toString(Expressions.constant(Collections.singletonMap("abc", 42)))); + Expressions.toString(Expressions.constant(Collections.singletonMap("abc", 42)), false)); } @Test void testTwoElementsMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of(\"abc\", 42,\n" + "\"def\", 43)", - Expressions.toString(Expressions.constant(ImmutableMap.of("abc", 42, "def", 43)))); + Expressions.toString(Expressions.constant(ImmutableMap.of("abc", 42, "def", 43)), false)); } @Test void testTenElementsMapLiteral() { @@ -1641,7 +1641,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".put(\"key_7\", \"value_7\")\n" + ".put(\"key_8\", \"value_8\")\n" + ".put(\"key_9\", \"value_9\").build()", - Expressions.toString(Expressions.constant(map))); + Expressions.toString(Expressions.constant(map), false)); } @Test void testEvaluate() { @@ -1657,12 +1657,12 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testOneElementSetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of(1)", - Expressions.toString(Expressions.constant(Sets.newHashSet(1)))); + Expressions.toString(Expressions.constant(Sets.newHashSet(1)), false)); } @Test void testTwoElementsSetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of(1,2)", - Expressions.toString(Expressions.constant(ImmutableSet.of(1, 2)))); + Expressions.toString(Expressions.constant(ImmutableSet.of(1, 2)), false)); } @Test void testTenElementsSetLiteral() { @@ -1680,7 +1680,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(7)\n" + ".add(8)\n" + ".add(9).build()", - Expressions.toString(Expressions.constant(set))); + Expressions.toString(Expressions.constant(set), false)); } @Test void testTenElementsLinkedHashSetLiteral() { @@ -1698,7 +1698,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(7)\n" + ".add(8)\n" + ".add(9).build()", - Expressions.toString(Expressions.constant(set))); + Expressions.toString(Expressions.constant(set), false)); } @Test void testTenElementsSetStringLiteral() { @@ -1716,7 +1716,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(\"3\")\n" + ".add(\"2\")\n" + ".add(\"1\").build()", - Expressions.toString(Expressions.constant(set))); + Expressions.toString(Expressions.constant(set), false)); } /** An enum. */ diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java index 8c529be17fd..2d5214df1de 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java @@ -117,7 +117,7 @@ public void prepareBuilder() { + " int t;\n" + " return (t = 1) != a ? t : c;\n" + "}\n", - Expressions.toString(builder.toBlock())); + Expressions.toString(builder.toBlock(), false)); } @Test void testAssignInConditionOptimizedOut() { @@ -153,7 +153,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { Expressions.parameter(int.class, "b"), Expressions.parameter(int.class, "c"))); builder.add(Expressions.return_(null, v)); - assertThat(Expressions.toString(builder.toBlock()), is(s)); + assertThat(Expressions.toString(builder.toBlock(), false), is(s)); } /** @@ -203,7 +203,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { + " int t = 2;\n" + " return (t = 1) != a ? t : c;\n" + "}\n", - Expressions.toString(builder.toBlock())); + Expressions.toString(builder.toBlock(), false)); } @Test void testMultiPassOptimization() { @@ -223,7 +223,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { assertEquals("{\n" + " return u + v;\n" + "}\n", - Expressions.toString(builder.toBlock())); + Expressions.toString(builder.toBlock(), false)); } @Test void testInlineInTryCatchStatement() { diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java index ac3a9c94105..de926fee549 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java @@ -2134,29 +2134,29 @@ private static int count(Enumerator enumerator) { @Test void testApproxConstant() { ConstantExpression c; c = Expressions.constant(new BigDecimal("3.1"), float.class); - assertThat(Expressions.toString(c), equalTo("3.1F")); + assertThat(Expressions.toString(c, false), equalTo("3.1F")); c = Expressions.constant(new BigDecimal("-5.156"), float.class); - assertThat(Expressions.toString(c), equalTo("-5.156F")); + assertThat(Expressions.toString(c, false), equalTo("-5.156F")); c = Expressions.constant(new BigDecimal("-51.6"), Float.class); - assertThat(Expressions.toString(c), equalTo("Float.valueOf(-51.6F)")); + assertThat(Expressions.toString(c, false), equalTo("Float.valueOf(-51.6F)")); c = Expressions.constant(new BigDecimal(Float.MAX_VALUE), Float.class); - assertThat(Expressions.toString(c), + assertThat(Expressions.toString(c, false), equalTo("Float.valueOf(Float.intBitsToFloat(2139095039))")); c = Expressions.constant(new BigDecimal(Float.MIN_VALUE), Float.class); - assertThat(Expressions.toString(c), + assertThat(Expressions.toString(c, false), equalTo("Float.valueOf(Float.intBitsToFloat(1))")); c = Expressions.constant(new BigDecimal("3.1"), double.class); - assertThat(Expressions.toString(c), equalTo("3.1D")); + assertThat(Expressions.toString(c, false), equalTo("3.1D")); c = Expressions.constant(new BigDecimal("-5.156"), double.class); - assertThat(Expressions.toString(c), equalTo("-5.156D")); + assertThat(Expressions.toString(c, false), equalTo("-5.156D")); c = Expressions.constant(new BigDecimal("-51.6"), Double.class); - assertThat(Expressions.toString(c), equalTo("Double.valueOf(-51.6D)")); + assertThat(Expressions.toString(c, false), equalTo("Double.valueOf(-51.6D)")); c = Expressions.constant(new BigDecimal(Double.MAX_VALUE), Double.class); - assertThat(Expressions.toString(c), + assertThat(Expressions.toString(c, false), equalTo("Double.valueOf(Double.longBitsToDouble(9218868437227405311L))")); c = Expressions.constant(new BigDecimal(Double.MIN_VALUE), Double.class); - assertThat(Expressions.toString(c), + assertThat(Expressions.toString(c, false), equalTo("Double.valueOf(Double.longBitsToDouble(1L))")); } diff --git a/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java b/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java index 22c745dd8f8..e7f1394053c 100644 --- a/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java +++ b/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java @@ -185,7 +185,7 @@ public void setup() { new EnumerableRelImplementor(plan.getCluster().getRexBuilder(), new HashMap<>()); info.classExpr = relImplementor.implementRoot(plan, EnumerableRel.Prefer.ARRAY); info.javaCode = - Expressions.toString(info.classExpr.memberDeclarations, "\n", false); + Expressions.toString(info.classExpr.memberDeclarations, "\n", false, true); info.plan = plan; planInfos[i] = info; } From dc783a916ccba733bd781c5108bcfdf0c052c24e Mon Sep 17 00:00:00 2001 From: James Duong Date: Wed, 18 Sep 2024 16:19:33 -0700 Subject: [PATCH 07/30] Merge fixes --- .../linq4j/tree/MethodDeclaration.java | 37 +++++++++++++------ .../calcite/linq4j/test/ExpressionTest.java | 4 +- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index c20fbff2850..6634a6cd968 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -26,8 +26,6 @@ import java.util.List; import java.util.Objects; -import static java.util.Objects.requireNonNull; - /** * Declaration of a method. */ @@ -40,11 +38,15 @@ public class MethodDeclaration extends MemberDeclaration { public MethodDeclaration(int modifier, String name, Type resultType, List parameters, BlockStatement body) { + assert name != null : "name should not be null"; + assert resultType != null : "resultType should not be null"; + assert parameters != null : "parameters should not be null"; + assert body != null : "body should not be null"; this.modifier = modifier; - this.name = requireNonNull(name, "name"); - this.resultType = requireNonNull(resultType, "resultType"); - this.parameters = requireNonNull(parameters, "parameters"); - this.body = requireNonNull(body, "body"); + this.name = name; + this.resultType = resultType; + this.parameters = parameters; + this.body = body; } @Override public MemberDeclaration accept(Shuttle shuttle) { @@ -103,11 +105,24 @@ public MethodDeclaration(int modifier, String name, Type resultType, } MethodDeclaration that = (MethodDeclaration) o; - return modifier == that.modifier - && body.equals(that.body) - && name.equals(that.name) - && parameters.equals(that.parameters) - && resultType.equals(that.resultType); + + if (modifier != that.modifier) { + return false; + } + if (!body.equals(that.body)) { + return false; + } + if (!name.equals(that.name)) { + return false; + } + if (!parameters.equals(that.parameters)) { + return false; + } + if (!resultType.equals(that.resultType)) { + return false; + } + + return true; } @Override public int hashCode() { diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index 0ad71669bb3..eb1acd39e59 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -1612,7 +1612,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testEmptyMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of()", - Expressions.toString(Expressions.constant(new HashMap<>()))); + Expressions.toString(Expressions.constant(new HashMap<>()), false)); } @Test void testOneElementMapLiteral() { @@ -1652,7 +1652,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testEmptySetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of()", - Expressions.toString(Expressions.constant(new HashSet<>()))); + Expressions.toString(Expressions.constant(new HashSet<>()), false)); } @Test void testOneElementSetLiteral() { From b7553e408d043af37dadb0a8c0751191c68e8bb4 Mon Sep 17 00:00:00 2001 From: James Duong Date: Thu, 19 Sep 2024 12:22:52 -0700 Subject: [PATCH 08/30] Fix whitespace differences in generated code --- .../org/apache/calcite/linq4j/tree/MethodDeclaration.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 6634a6cd968..bde78897a9c 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -16,6 +16,7 @@ */ package org.apache.calcite.linq4j.tree; +import org.apache.commons.lang3.StringUtils; import org.apache.flink.table.codesplit.JavaCodeSplitter; import org.checkerframework.checker.nullness.qual.Nullable; @@ -90,9 +91,9 @@ public MethodDeclaration(int modifier, String name, Type resultType, final int flinkDefaultMaxMembersGeneratedCode = 10000; // TODO: Use code splitter conditionally based on configuration. - writer.append( + writer.append(StringUtils.stripStart( JavaCodeSplitter.split(tempWriter.toString(), flinkDefaultMaxGeneratedCodeLength, - flinkDefaultMaxMembersGeneratedCode)); + flinkDefaultMaxMembersGeneratedCode), null)); writer.newlineAndIndent(); } From f63b6658234ef2ba09b074253ee445d78cb85097 Mon Sep 17 00:00:00 2001 From: James Duong Date: Thu, 19 Sep 2024 12:29:14 -0700 Subject: [PATCH 09/30] Checker framework --- .../java/org/apache/calcite/linq4j/tree/MethodDeclaration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index bde78897a9c..ad084623b48 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -93,7 +93,7 @@ public MethodDeclaration(int modifier, String name, Type resultType, // TODO: Use code splitter conditionally based on configuration. writer.append(StringUtils.stripStart( JavaCodeSplitter.split(tempWriter.toString(), flinkDefaultMaxGeneratedCodeLength, - flinkDefaultMaxMembersGeneratedCode), null)); + flinkDefaultMaxMembersGeneratedCode), " ")); writer.newlineAndIndent(); } From 4216e80d36065ad4746344f85054d2295a869ee0 Mon Sep 17 00:00:00 2001 From: James Duong Date: Thu, 19 Sep 2024 12:49:46 -0700 Subject: [PATCH 10/30] Autostyle --- .../java/org/apache/calcite/linq4j/tree/MethodDeclaration.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index ad084623b48..01dcb5e1193 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -91,7 +91,8 @@ public MethodDeclaration(int modifier, String name, Type resultType, final int flinkDefaultMaxMembersGeneratedCode = 10000; // TODO: Use code splitter conditionally based on configuration. - writer.append(StringUtils.stripStart( + writer.append( + StringUtils.stripStart( JavaCodeSplitter.split(tempWriter.toString(), flinkDefaultMaxGeneratedCodeLength, flinkDefaultMaxMembersGeneratedCode), " ")); writer.newlineAndIndent(); From 2077d1ebbf54c6a6b3b5b5975bfe79d50f9d3388 Mon Sep 17 00:00:00 2001 From: James Duong Date: Thu, 19 Sep 2024 16:46:27 -0700 Subject: [PATCH 11/30] Parameterize use of method splitting --- .../calcite/linq4j/tree/ExpressionWriter.java | 7 ++- .../linq4j/tree/MethodDeclaration.java | 44 +++++++++---------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java index eadfda003af..616ff8b0edd 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java @@ -32,10 +32,9 @@ class ExpressionWriter { private final Spacer spacer = new Spacer(0); private final StringBuilder buf = new StringBuilder(); - private boolean indentPending; - private final boolean generics; private final boolean methodSplitting; + private boolean indentPending; ExpressionWriter() { this(true, true); @@ -83,6 +82,10 @@ public boolean requireParentheses(Expression expression, int lprec, return true; } + public boolean usesMethodSplitting() { + return methodSplitting; + } + /** * Increases the indentation level. */ diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 01dcb5e1193..c9e30c97489 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -62,14 +62,16 @@ public MethodDeclaration(int modifier, String name, Type resultType, } @Override public void accept(ExpressionWriter writer) { - final ExpressionWriter tempWriter = writer.duplicateState(); - String modifiers = Modifier.toString(modifier); - tempWriter.append(modifiers); + final ExpressionWriter writerForUnsplitMethod = writer.usesMethodSplitting() + ? writer.duplicateState() : writer; + + final String modifiers = Modifier.toString(modifier); + writerForUnsplitMethod.append(modifiers); if (!modifiers.isEmpty()) { - tempWriter.append(' '); + writerForUnsplitMethod.append(' '); } //noinspection unchecked - tempWriter + writerForUnsplitMethod .append(resultType) .append(' ') .append(name) @@ -78,23 +80,21 @@ public MethodDeclaration(int modifier, String name, Type resultType, .append(' ') .append(body); - // From Flink TableConfigOptions.MAX_LENGTH_GENERATED_CODE - // Note that Flink uses 4000 rather than 64KB explicitly: - // "Specifies a threshold where generated code will be split into sub-function calls. - // Java has a maximum method length of 64 KB. This setting allows for finer granularity if - // necessary. - // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with - // more than 8K byte code." - final int flinkDefaultMaxGeneratedCodeLength = 4000; - - // From Flink TableConfigOptions.MAX_MEMBERS_GENERATED_CODE - final int flinkDefaultMaxMembersGeneratedCode = 10000; - - // TODO: Use code splitter conditionally based on configuration. - writer.append( - StringUtils.stripStart( - JavaCodeSplitter.split(tempWriter.toString(), flinkDefaultMaxGeneratedCodeLength, - flinkDefaultMaxMembersGeneratedCode), " ")); + if (writer.usesMethodSplitting()) { + // Specifies a threshold where generated code will be split into sub-function calls. + // Java has a maximum method length of 64 KB. This setting allows for finer granularity if + // necessary. + // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with + // more than 8K byte code. + final int defaultMaxGeneratedCodeLength = 4000; + final int defaultMaxMembersGeneratedCode = 10000; + + writer.append( + StringUtils.stripStart( + JavaCodeSplitter.split(writerForUnsplitMethod.toString(), + defaultMaxGeneratedCodeLength, defaultMaxMembersGeneratedCode), + " ")); + } writer.newlineAndIndent(); } From fd2de3dcb61cf47bb7d06838445d3c671a5159c2 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 20 Sep 2024 12:53:45 -0700 Subject: [PATCH 12/30] Enable method splitting in linq4j tests --- .../calcite/linq4j/test/BlockBuilderTest.java | 8 +- .../calcite/linq4j/test/ExpressionTest.java | 198 +++++++++--------- .../calcite/linq4j/test/InlinerTest.java | 8 +- .../calcite/linq4j/test/Linq4jTest.java | 20 +- 4 files changed, 117 insertions(+), 117 deletions(-) diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java index 50e05b3755a..b3b20a41b7c 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java @@ -108,7 +108,7 @@ private BlockBuilder appendBlockWithSameVariable( + " x = 1;\n" + " int x0;\n" + " x0 = 42;\n" - + "}\n", Expressions.toString(outer.toBlock(), false), + + "}\n", Expressions.toString(outer.toBlock(), true), "x in the second block should be renamed to avoid name clash"); } @@ -122,7 +122,7 @@ private BlockBuilder appendBlockWithSameVariable( + " x = 1;\n" + " int x0 = 8;\n" + " x0 = 42;\n" - + "}\n", Expressions.toString(outer.toBlock(), false), + + "}\n", Expressions.toString(outer.toBlock(), true), "x in the second block should be renamed to avoid name clash"); } @@ -143,7 +143,7 @@ private BlockBuilder appendBlockWithSameVariable( + " final Object _i = new org.apache.calcite.linq4j.test.BlockBuilderTest.Identity()" + ".apply(\"test\");\n" + "}\n", - Expressions.toString(bb.toBlock(), false)); + Expressions.toString(bb.toBlock(), true)); } @@ -160,7 +160,7 @@ private BlockBuilder appendBlockWithSameVariable( assertEquals("{\n" + " return false;\n" - + "}\n", Expressions.toString(outer.toBlock(), false), + + "}\n", Expressions.toString(outer.toBlock(), true), "Expected to optimize Boolean.FALSE = null to false"); } diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index eb1acd39e59..f3b405146c6 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -90,7 +90,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(int arg) {\n" @@ -133,7 +133,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(short arg) {\n" @@ -176,7 +176,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(byte arg) {\n" @@ -218,7 +218,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public double apply(double arg) {\n" @@ -259,7 +259,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(2L)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public long apply(long arg) {\n" @@ -300,7 +300,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(2.0f)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public float apply(float arg) {\n" @@ -341,7 +341,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(10)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public long apply(long arg) {\n" @@ -382,7 +382,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(10.1d)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public double apply(double arg) {\n" @@ -425,7 +425,7 @@ public class ExpressionTest { Arrays.asList(paramExpr, param2Expr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, false); + String s = Expressions.toString(lambdaExpr, true); assertEquals("new org.apache.calcite.linq4j.function.Function2() {\n" + " public int apply(int key, int key2) {\n" + " return key;\n" @@ -477,11 +477,11 @@ public class ExpressionTest { assertEquals( "true", Expressions.toString( - Expressions.foldAnd(list0), false)); + Expressions.foldAnd(list0), true)); assertEquals( "false", Expressions.toString( - Expressions.foldOr(list0), false)); + Expressions.foldOr(list0), true)); final List list1 = Arrays.asList( @@ -494,12 +494,12 @@ public class ExpressionTest { assertEquals( "1 == 2 && 3 == 4 && 5 == 6", Expressions.toString( - Expressions.foldAnd(list1), false)); + Expressions.foldAnd(list1), true)); // a single true makes OR true assertEquals( "true", Expressions.toString( - Expressions.foldOr(list1), false)); + Expressions.foldOr(list1), true)); final List list2 = Collections.singletonList( @@ -507,11 +507,11 @@ public class ExpressionTest { assertEquals( "true", Expressions.toString( - Expressions.foldAnd(list2), false)); + Expressions.foldAnd(list2), true)); assertEquals( "true", Expressions.toString( - Expressions.foldOr(list2), false)); + Expressions.foldOr(list2), true)); final List list3 = Arrays.asList( @@ -523,11 +523,11 @@ public class ExpressionTest { assertEquals( "false", Expressions.toString( - Expressions.foldAnd(list3), false)); + Expressions.foldAnd(list3), true)); assertEquals( "1 == 2 || 5 == 6", Expressions.toString( - Expressions.foldOr(list3), false)); + Expressions.foldOr(list3), true)); } @Test void testWrite() { @@ -540,13 +540,13 @@ public class ExpressionTest { Expressions.constant(1), Expressions.constant(2F, Float.TYPE)), Expressions.constant(3L, Long.TYPE)), - Expressions.constant(4L, Long.class)), false)); + Expressions.constant(4L, Long.class)), true)); assertEquals( "java.math.BigDecimal.valueOf(31415926L, 7)", Expressions.toString( Expressions.constant( - BigDecimal.valueOf(314159260, 8)), false)); + BigDecimal.valueOf(314159260, 8)), true)); // Parentheses needed, to override the left-associativity of +. assertEquals( @@ -556,7 +556,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.add( Expressions.constant(2), - Expressions.constant(3))), false)); + Expressions.constant(3))), true)); // No parentheses needed; higher precedence of * achieves the desired // effect. @@ -567,7 +567,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.multiply( Expressions.constant(2), - Expressions.constant(3))), false)); + Expressions.constant(3))), true)); assertEquals( "1 * (2 + 3)", @@ -576,7 +576,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.add( Expressions.constant(2), - Expressions.constant(3))), false)); + Expressions.constant(3))), true)); // Parentheses needed, to overcome right-associativity of =. assertEquals( @@ -585,7 +585,7 @@ public class ExpressionTest { Expressions.assign( Expressions.assign( Expressions.constant(1), Expressions.constant(2)), - Expressions.constant(3)), false)); + Expressions.constant(3)), true)); // Ternary operator. assertEquals( @@ -606,7 +606,7 @@ public class ExpressionTest { Expressions.constant(7), Expressions.constant(8)), Expressions.constant(9), - Expressions.constant(10))), false)); + Expressions.constant(10))), true)); assertEquals( "0 + (double) (2 + 3)", @@ -616,7 +616,7 @@ public class ExpressionTest { Expressions.convert_( Expressions.add( Expressions.constant(2), Expressions.constant(3)), - Double.TYPE)), false)); + Double.TYPE)), true)); // "--5" would be a syntax error assertEquals( @@ -624,27 +624,27 @@ public class ExpressionTest { Expressions.toString( Expressions.negate( Expressions.negate( - Expressions.constant(5))), false)); + Expressions.constant(5))), true)); assertEquals( "a.empno", Expressions.toString( Expressions.field( Expressions.parameter(Linq4jTest.Employee.class, "a"), - "empno"), false)); + "empno"), true)); assertEquals( "a.length", Expressions.toString( Expressions.field( Expressions.parameter(Object[].class, "a"), - "length"), false)); + "length"), true)); assertEquals( "java.util.Collections.EMPTY_LIST", Expressions.toString( Expressions.field( - null, Collections.class, "EMPTY_LIST"), false)); + null, Collections.class, "EMPTY_LIST"), true)); final ParameterExpression paramX = Expressions.parameter(String.class, "x"); @@ -663,7 +663,7 @@ public class ExpressionTest { Function1.class, Expressions.call( paramX, "length", Collections.emptyList()), - Arrays.asList(paramX)), false)); + Arrays.asList(paramX)), true)); // 1-dimensional array with initializer assertEquals( @@ -676,7 +676,7 @@ public class ExpressionTest { String.class, Expressions.constant("foo"), Expressions.constant(null), - Expressions.constant("bar\"baz")), false)); + Expressions.constant("bar\"baz")), true)); // 2-dimensional array with initializer assertEquals( @@ -693,7 +693,7 @@ public class ExpressionTest { 2, Expressions.constant(new String[] {"foo", "bar"}), Expressions.constant(null), - Expressions.constant(new String[] {null})), false)); + Expressions.constant(new String[] {null})), true)); // 1-dimensional array assertEquals( @@ -704,7 +704,7 @@ public class ExpressionTest { 1, Expressions.add( Expressions.parameter(0, int.class, "x"), - Expressions.constant(1))), false)); + Expressions.constant(1))), true)); // 3-dimensional array assertEquals( @@ -715,7 +715,7 @@ public class ExpressionTest { 3, Expressions.add( Expressions.parameter(0, int.class, "x"), - Expressions.constant(1))), false)); + Expressions.constant(1))), true)); assertEquals( "(int) ((String) (Object) \"foo\").length()", @@ -729,7 +729,7 @@ public class ExpressionTest { String.class), "length", Collections.emptyList()), - Integer.TYPE), false)); + Integer.TYPE), true)); // resolving a static method assertEquals( @@ -739,7 +739,7 @@ public class ExpressionTest { Integer.class, "valueOf", Collections.singletonList( - Expressions.constant("0123"))), false)); + Expressions.constant("0123"))), true)); // precedence of not and instanceof assertEquals( @@ -748,7 +748,7 @@ public class ExpressionTest { Expressions.not( Expressions.typeIs( Expressions.parameter(Object.class, "o"), - String.class)), false)); + String.class)), true)); // not not assertEquals( @@ -758,7 +758,7 @@ public class ExpressionTest { Expressions.not( Expressions.typeIs( Expressions.parameter(Object.class, "o"), - String.class))), false)); + String.class))), true)); } @Test void testWriteConstant() { @@ -769,65 +769,65 @@ public class ExpressionTest { + " 2,\n" + " -1}", Expressions.toString( - Expressions.constant(new int[]{1, 2, -1}), false)); + Expressions.constant(new int[]{1, 2, -1}), true)); // primitive assertEquals( "-12", Expressions.toString( - Expressions.constant(-12), false)); + Expressions.constant(-12), true)); assertEquals( "(short)-12", Expressions.toString( - Expressions.constant((short) -12), false)); + Expressions.constant((short) -12), true)); assertEquals( "(byte)-12", Expressions.toString( - Expressions.constant((byte) -12), false)); + Expressions.constant((byte) -12), true)); // boxed primitives assertEquals( "Integer.valueOf(1)", Expressions.toString( - Expressions.constant(1, Integer.class), false)); + Expressions.constant(1, Integer.class), true)); assertEquals( "Double.valueOf(-3.14D)", Expressions.toString( - Expressions.constant(-3.14, Double.class), false)); + Expressions.constant(-3.14, Double.class), true)); assertEquals( "Boolean.valueOf(true)", Expressions.toString( - Expressions.constant(true, Boolean.class), false)); + Expressions.constant(true, Boolean.class), true)); // primitive with explicit class assertEquals( "1", Expressions.toString( - Expressions.constant(1, int.class), false)); + Expressions.constant(1, int.class), true)); assertEquals( "(short)1", Expressions.toString( - Expressions.constant(1, short.class), false)); + Expressions.constant(1, short.class), true)); assertEquals( "(byte)1", Expressions.toString( - Expressions.constant(1, byte.class), false)); + Expressions.constant(1, byte.class), true)); assertEquals( "-3.14D", Expressions.toString( - Expressions.constant(-3.14, double.class), false)); + Expressions.constant(-3.14, double.class), true)); assertEquals( "true", Expressions.toString( - Expressions.constant(true, boolean.class), false)); + Expressions.constant(true, boolean.class), true)); // objects and nulls assertEquals( @@ -835,19 +835,19 @@ public class ExpressionTest { + " \"foo\",\n" + " null}", Expressions.toString( - Expressions.constant(new String[] {"foo", null}), false)); + Expressions.constant(new String[] {"foo", null}), true)); // string assertEquals( "\"hello, \\\"world\\\"!\"", Expressions.toString( - Expressions.constant("hello, \"world\"!"), false)); + Expressions.constant("hello, \"world\"!"), true)); // enum assertEquals( "org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.X", Expressions.toString( - Expressions.constant(MyEnum.X), false)); + Expressions.constant(MyEnum.X), true)); // array of enum assertEquals( @@ -855,24 +855,24 @@ public class ExpressionTest { + " org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.X,\n" + " org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.Y}", Expressions.toString( - Expressions.constant(new MyEnum[]{MyEnum.X, MyEnum.Y}), false)); + Expressions.constant(new MyEnum[]{MyEnum.X, MyEnum.Y}), true)); // class assertEquals( "java.lang.String.class", Expressions.toString( - Expressions.constant(String.class), false)); + Expressions.constant(String.class), true)); // array class assertEquals( "int[].class", Expressions.toString( - Expressions.constant(int[].class), false)); + Expressions.constant(int[].class), true)); assertEquals( "java.util.List[][].class", Expressions.toString( - Expressions.constant(List[][].class), false)); + Expressions.constant(List[][].class), true)); // automatically call constructor if it matches fields assertEquals( @@ -894,7 +894,7 @@ public class ExpressionTest { + " \"Janet\",\n" + " 10)}", Expressions.toString( - Expressions.constant(Linq4jTest.emps), false)); + Expressions.constant(Linq4jTest.emps), true)); } /** Test case for @@ -919,7 +919,7 @@ public class ExpressionTest { ImmutableSet.of(createInstance(recordClass, "test1", 1), createInstance(recordClass, "test2", 2), createInstance(recordClass, "test3", 3), - createInstance(recordClass, "test4", 4))), false)); + createInstance(recordClass, "test4", 4))), true)); } @Test void testWriteArray() { @@ -932,7 +932,7 @@ public class ExpressionTest { Expressions.variable(int[].class, "integers"), Expressions.add( Expressions.constant(2), - Expressions.variable(int.class, "index")))), false)); + Expressions.variable(int.class, "index")))), true)); } @Test void testWriteAnonymousClass() { @@ -1016,7 +1016,7 @@ public class ExpressionTest { + "\n" + " };\n" + "}\n", - Expressions.toString(e, false)); + Expressions.toString(e, true)); } @Test void testWriteWhile() { @@ -1071,7 +1071,7 @@ public class ExpressionTest { + "} finally {\n" + " \"foo\".toUpperCase();\n" + "}\n", - Expressions.toString(node, false)); + Expressions.toString(node, true)); } @Test void testWriteTryFinally() { @@ -1096,7 +1096,7 @@ public class ExpressionTest { + " \"foo\".toUpperCase();\n" + " }\n" + "}\n", - Expressions.toString(node, false)); + Expressions.toString(node, true)); } @Test void testWriteTryCatch() { @@ -1122,7 +1122,7 @@ public class ExpressionTest { + "} catch (RuntimeException re) {\n" + " return re.toString();\n" + "}\n", - Expressions.toString(node, false)); + Expressions.toString(node, true)); } @Test void testType() { @@ -1237,7 +1237,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { nine)); statements.add(Expressions.return_(null, eighteen)); BlockStatement expression = statements.toBlock(); - assertEquals(expected, Expressions.toString(expression, false)); + assertEquals(expected, Expressions.toString(expression, true)); expression.accept(new Shuttle()); } @@ -1268,7 +1268,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " (java.util.Comparator) null);\n" + " return treeSet.add(null);\n" + "}\n"; - assertThat(Expressions.toString(expression, false), is(expected)); + assertThat(Expressions.toString(expression, true), is(expected)); expression.accept(new Shuttle()); } @@ -1317,7 +1317,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " final int _b0 = 1 + 3;\n" + " org.apache.calcite.linq4j.test.ExpressionTest.bar(1, _b, _c, _d, org.apache.calcite.linq4j.test.ExpressionTest.foo(_b0));\n" + "}\n", - Expressions.toString(expression, false)); + Expressions.toString(expression, true)); expression.accept(new Shuttle()); } @@ -1366,32 +1366,32 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testBigDecimalConstantExpression() { assertEquals("java.math.BigDecimal.valueOf(104L)", - Expressions.toString(Expressions.constant("104", BigDecimal.class), false)); + Expressions.toString(Expressions.constant("104", BigDecimal.class), true)); assertEquals("java.math.BigDecimal.valueOf(1L, -3)", - Expressions.toString(Expressions.constant("1000", BigDecimal.class), false)); + Expressions.toString(Expressions.constant("1000", BigDecimal.class), true)); assertEquals("java.math.BigDecimal.valueOf(1L, -3)", - Expressions.toString(Expressions.constant(1000, BigDecimal.class), false)); + Expressions.toString(Expressions.constant(1000, BigDecimal.class), true)); assertEquals("java.math.BigDecimal.valueOf(107L)", - Expressions.toString(Expressions.constant(107, BigDecimal.class), false)); + Expressions.toString(Expressions.constant(107, BigDecimal.class), true)); assertEquals("java.math.BigDecimal.valueOf(199999999999999L)", - Expressions.toString(Expressions.constant(199999999999999L, BigDecimal.class), false)); + Expressions.toString(Expressions.constant(199999999999999L, BigDecimal.class), true)); assertEquals("java.math.BigDecimal.valueOf(1234L, 2)", - Expressions.toString(Expressions.constant(12.34, BigDecimal.class), false)); + Expressions.toString(Expressions.constant(12.34, BigDecimal.class), true)); } @Test void testObjectConstantExpression() { assertEquals("(byte)100", - Expressions.toString(Expressions.constant((byte) 100, Object.class), false)); + Expressions.toString(Expressions.constant((byte) 100, Object.class), true)); assertEquals("(char)100", - Expressions.toString(Expressions.constant((char) 100, Object.class), false)); + Expressions.toString(Expressions.constant((char) 100, Object.class), true)); assertEquals("(short)100", - Expressions.toString(Expressions.constant((short) 100, Object.class), false)); + Expressions.toString(Expressions.constant((short) 100, Object.class), true)); assertEquals("100L", - Expressions.toString(Expressions.constant(100L, Object.class), false)); + Expressions.toString(Expressions.constant(100L, Object.class), true)); assertEquals("100.0F", - Expressions.toString(Expressions.constant(100F, Object.class), false)); + Expressions.toString(Expressions.constant(100F, Object.class), true)); assertEquals("100.0D", - Expressions.toString(Expressions.constant(100D, Object.class), false)); + Expressions.toString(Expressions.constant(100D, Object.class), true)); } @Test void testClassDecl() { @@ -1425,7 +1425,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " }\n" + " int i;\n" + "}", - Expressions.toString(newExpression, false)); + Expressions.toString(newExpression, true)); newExpression.accept(new Shuttle()); } @@ -1458,7 +1458,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.return_(null), Expressions.constant(false), Expressions.return_(null), - Expressions.return_(null, Expressions.constant(1))), false)); + Expressions.return_(null, Expressions.constant(1))), true)); } /** Test for common sub-expression elimination. */ @@ -1505,7 +1505,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " return (Number) v == null ? null : (" + "(Number) v).intValue() == 1997;\n" + "}\n", - Expressions.toString(builder.toBlock(), false)); + Expressions.toString(builder.toBlock(), true)); } @Test void testFor() throws NoSuchFieldException { @@ -1530,7 +1530,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " System.out.println(i);\n" + " }\n" + "}\n", - Expressions.toString(builder.toBlock(), false)); + Expressions.toString(builder.toBlock(), true)); } @Test void testFor2() { @@ -1560,7 +1560,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " }\n" + " }\n" + "}\n", - Expressions.toString(builder.toBlock(), false)); + Expressions.toString(builder.toBlock(), true)); } @Test void testForEach() { @@ -1574,7 +1574,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.constant(1), Expressions.constant(2)), Expressions.break_(null)))); - assertThat(Expressions.toString(builder.toBlock(), false), + assertThat(Expressions.toString(builder.toBlock(), true), is("{\n" + " for (int i : list) {\n" + " if (1 < 2) {\n" @@ -1586,18 +1586,18 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testEmptyListLiteral() { assertEquals("java.util.Collections.EMPTY_LIST", - Expressions.toString(Expressions.constant(Arrays.asList()), false)); + Expressions.toString(Expressions.constant(Arrays.asList()), true)); } @Test void testOneElementListLiteral() { assertEquals("java.util.Arrays.asList(1)", - Expressions.toString(Expressions.constant(Arrays.asList(1)), false)); + Expressions.toString(Expressions.constant(Arrays.asList(1)), true)); } @Test void testTwoElementsListLiteral() { assertEquals("java.util.Arrays.asList(1,\n" + " 2)", - Expressions.toString(Expressions.constant(Arrays.asList(1, 2)), false)); + Expressions.toString(Expressions.constant(Arrays.asList(1, 2)), true)); } @Test void testNestedListsLiteral() { @@ -1607,23 +1607,23 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " 4))", Expressions.toString( Expressions.constant( - Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4))), false)); + Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4))), true)); } @Test void testEmptyMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of()", - Expressions.toString(Expressions.constant(new HashMap<>()), false)); + Expressions.toString(Expressions.constant(new HashMap<>()), true)); } @Test void testOneElementMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of(\"abc\", 42)", - Expressions.toString(Expressions.constant(Collections.singletonMap("abc", 42)), false)); + Expressions.toString(Expressions.constant(Collections.singletonMap("abc", 42)), true)); } @Test void testTwoElementsMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of(\"abc\", 42,\n" + "\"def\", 43)", - Expressions.toString(Expressions.constant(ImmutableMap.of("abc", 42, "def", 43)), false)); + Expressions.toString(Expressions.constant(ImmutableMap.of("abc", 42, "def", 43)), true)); } @Test void testTenElementsMapLiteral() { @@ -1641,7 +1641,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".put(\"key_7\", \"value_7\")\n" + ".put(\"key_8\", \"value_8\")\n" + ".put(\"key_9\", \"value_9\").build()", - Expressions.toString(Expressions.constant(map), false)); + Expressions.toString(Expressions.constant(map), true)); } @Test void testEvaluate() { @@ -1652,17 +1652,17 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testEmptySetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of()", - Expressions.toString(Expressions.constant(new HashSet<>()), false)); + Expressions.toString(Expressions.constant(new HashSet<>()), true)); } @Test void testOneElementSetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of(1)", - Expressions.toString(Expressions.constant(Sets.newHashSet(1)), false)); + Expressions.toString(Expressions.constant(Sets.newHashSet(1)), true)); } @Test void testTwoElementsSetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of(1,2)", - Expressions.toString(Expressions.constant(ImmutableSet.of(1, 2)), false)); + Expressions.toString(Expressions.constant(ImmutableSet.of(1, 2)), true)); } @Test void testTenElementsSetLiteral() { @@ -1680,7 +1680,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(7)\n" + ".add(8)\n" + ".add(9).build()", - Expressions.toString(Expressions.constant(set), false)); + Expressions.toString(Expressions.constant(set), true)); } @Test void testTenElementsLinkedHashSetLiteral() { @@ -1698,7 +1698,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(7)\n" + ".add(8)\n" + ".add(9).build()", - Expressions.toString(Expressions.constant(set), false)); + Expressions.toString(Expressions.constant(set), true)); } @Test void testTenElementsSetStringLiteral() { @@ -1716,7 +1716,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(\"3\")\n" + ".add(\"2\")\n" + ".add(\"1\").build()", - Expressions.toString(Expressions.constant(set), false)); + Expressions.toString(Expressions.constant(set), true)); } /** An enum. */ diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java index 2d5214df1de..c37f5ac12e7 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java @@ -117,7 +117,7 @@ public void prepareBuilder() { + " int t;\n" + " return (t = 1) != a ? t : c;\n" + "}\n", - Expressions.toString(builder.toBlock(), false)); + Expressions.toString(builder.toBlock(), true)); } @Test void testAssignInConditionOptimizedOut() { @@ -153,7 +153,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { Expressions.parameter(int.class, "b"), Expressions.parameter(int.class, "c"))); builder.add(Expressions.return_(null, v)); - assertThat(Expressions.toString(builder.toBlock(), false), is(s)); + assertThat(Expressions.toString(builder.toBlock(), true), is(s)); } /** @@ -203,7 +203,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { + " int t = 2;\n" + " return (t = 1) != a ? t : c;\n" + "}\n", - Expressions.toString(builder.toBlock(), false)); + Expressions.toString(builder.toBlock(), true)); } @Test void testMultiPassOptimization() { @@ -223,7 +223,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { assertEquals("{\n" + " return u + v;\n" + "}\n", - Expressions.toString(builder.toBlock(), false)); + Expressions.toString(builder.toBlock(), true)); } @Test void testInlineInTryCatchStatement() { diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java index de926fee549..11300adacbb 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java @@ -2134,29 +2134,29 @@ private static int count(Enumerator enumerator) { @Test void testApproxConstant() { ConstantExpression c; c = Expressions.constant(new BigDecimal("3.1"), float.class); - assertThat(Expressions.toString(c, false), equalTo("3.1F")); + assertThat(Expressions.toString(c, true), equalTo("3.1F")); c = Expressions.constant(new BigDecimal("-5.156"), float.class); - assertThat(Expressions.toString(c, false), equalTo("-5.156F")); + assertThat(Expressions.toString(c, true), equalTo("-5.156F")); c = Expressions.constant(new BigDecimal("-51.6"), Float.class); - assertThat(Expressions.toString(c, false), equalTo("Float.valueOf(-51.6F)")); + assertThat(Expressions.toString(c, true), equalTo("Float.valueOf(-51.6F)")); c = Expressions.constant(new BigDecimal(Float.MAX_VALUE), Float.class); - assertThat(Expressions.toString(c, false), + assertThat(Expressions.toString(c, true), equalTo("Float.valueOf(Float.intBitsToFloat(2139095039))")); c = Expressions.constant(new BigDecimal(Float.MIN_VALUE), Float.class); - assertThat(Expressions.toString(c, false), + assertThat(Expressions.toString(c, true), equalTo("Float.valueOf(Float.intBitsToFloat(1))")); c = Expressions.constant(new BigDecimal("3.1"), double.class); - assertThat(Expressions.toString(c, false), equalTo("3.1D")); + assertThat(Expressions.toString(c, true), equalTo("3.1D")); c = Expressions.constant(new BigDecimal("-5.156"), double.class); - assertThat(Expressions.toString(c, false), equalTo("-5.156D")); + assertThat(Expressions.toString(c, true), equalTo("-5.156D")); c = Expressions.constant(new BigDecimal("-51.6"), Double.class); - assertThat(Expressions.toString(c, false), equalTo("Double.valueOf(-51.6D)")); + assertThat(Expressions.toString(c, true), equalTo("Double.valueOf(-51.6D)")); c = Expressions.constant(new BigDecimal(Double.MAX_VALUE), Double.class); - assertThat(Expressions.toString(c, false), + assertThat(Expressions.toString(c, true), equalTo("Double.valueOf(Double.longBitsToDouble(9218868437227405311L))")); c = Expressions.constant(new BigDecimal(Double.MIN_VALUE), Double.class); - assertThat(Expressions.toString(c, false), + assertThat(Expressions.toString(c, true), equalTo("Double.valueOf(Double.longBitsToDouble(1L))")); } From 8f0424372ae79602b90ab9ce3fad3d69fba7244a Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 20 Sep 2024 13:39:52 -0700 Subject: [PATCH 13/30] Make method-splitting in library code based on a CalciteSystemProperty --- .../adapter/enumerable/EnumerableInterpretable.java | 5 ++++- .../org/apache/calcite/config/CalciteSystemProperty.java | 8 ++++++-- .../org/apache/calcite/interpreter/JaninoRexCompiler.java | 4 +++- .../main/java/org/apache/calcite/rex/RexExecutorImpl.java | 3 ++- .../adapter/innodb/InnodbToEnumerableConverter.java | 4 +++- 5 files changed, 18 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java index 92bb10cf304..9db29216edd 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java @@ -111,7 +111,10 @@ public static Bindable toBindable(Map parameters, parameters); final ClassDeclaration expr = relImplementor.implementRoot(rel, prefer); - String s = Expressions.toString(expr.memberDeclarations, "\n", false, true); + String s = + Expressions.toString(expr.memberDeclarations, + "\n", false, + CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); diff --git a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java index 0dde1efe005..ce0c592f010 100644 --- a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java +++ b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java @@ -420,8 +420,12 @@ public final class CalciteSystemProperty { public static final CalciteSystemProperty FUNCTION_LEVEL_CACHE_MAX_SIZE = intProperty("calcite.function.cache.maxSize", 0, v -> v >= 0); - public static final CalciteSystemProperty ENABLE_CODE_SPLITTING = - booleanProperty("calcite.linq.enable_code_splitting", false); + /** Whether to enable automatic method splitting when generating Java code. + * + *

Some queries can generate methods exceeding the JVM limit of 4000 characters per method. + * Enable this feature to automatically detect and split methods larger than the limit. */ + public static final CalciteSystemProperty ENABLE_METHOD_SPLITTING = + booleanProperty("calcite.linq.enable_method_splitting", false); private static CalciteSystemProperty booleanProperty(String key, boolean defaultValue) { diff --git a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java b/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java index 2a55cf7c650..cf21f632819 100644 --- a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java +++ b/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, true); + String s = + Expressions.toString(declarations, "\n", false, + CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); } diff --git a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java index 676c67a032b..9ec13ec10a3 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java +++ b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java @@ -100,7 +100,8 @@ private static String compile(RexBuilder rexBuilder, List constExps, Expressions.methodDecl(Modifier.PUBLIC, Object[].class, BuiltInMethod.FUNCTION1_APPLY.method.getName(), ImmutableList.of(root0_), blockBuilder.toBlock()); - String code = Expressions.toString(methodDecl, true); + String code = + Expressions.toString(methodDecl, CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, code); } diff --git a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java index 545b1432c82..d56e2ca0871 100644 --- a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java +++ b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java @@ -150,7 +150,9 @@ static List innodbFieldNames(final RelDataType rowType) { InnodbMethod.INNODB_QUERYABLE_QUERY.method, fields, selectFields, cond, ascOrder)); if (CalciteSystemProperty.DEBUG.value()) { - System.out.println("Innodb: " + Expressions.toString(enumerable, true)); + System.out.println( + "Innodb: " + Expressions.toString(enumerable, + CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value())); } list.add(Expressions.return_(null, enumerable)); return implementor.result(physType, list.toBlock()); From dd5b9954d17ba58ee9f6b4d667ba74d6f4aa0ea8 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 20 Sep 2024 14:02:44 -0700 Subject: [PATCH 14/30] Fix commented-out code --- linq4j/build.gradle.kts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/linq4j/build.gradle.kts b/linq4j/build.gradle.kts index 475c1a03cba..fe99d7abbd2 100644 --- a/linq4j/build.gradle.kts +++ b/linq4j/build.gradle.kts @@ -20,7 +20,5 @@ dependencies { implementation("com.google.guava:guava") implementation("org.apache.calcite.avatica:avatica-core") - implementation("org.apache.flink:flink-table-code-splitter") { - // exclude("org.apache.flink", "flink-core") - } + implementation("org.apache.flink:flink-table-code-splitter") } From 8f62b889dcbad95a6f234480175b89e55cc5abce Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 20 Sep 2024 14:03:03 -0700 Subject: [PATCH 15/30] Revert unintended change around assert --- .../calcite/linq4j/tree/MethodDeclaration.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index c9e30c97489..10acaa25063 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -16,6 +16,8 @@ */ package org.apache.calcite.linq4j.tree; +import static java.util.Objects.requireNonNull; + import org.apache.commons.lang3.StringUtils; import org.apache.flink.table.codesplit.JavaCodeSplitter; @@ -39,15 +41,10 @@ public class MethodDeclaration extends MemberDeclaration { public MethodDeclaration(int modifier, String name, Type resultType, List parameters, BlockStatement body) { - assert name != null : "name should not be null"; - assert resultType != null : "resultType should not be null"; - assert parameters != null : "parameters should not be null"; - assert body != null : "body should not be null"; - this.modifier = modifier; - this.name = name; - this.resultType = resultType; - this.parameters = parameters; - this.body = body; + this.name = requireNonNull(name, "name"); + this.resultType = requireNonNull(resultType, "resultType"); + this.parameters = requireNonNull(parameters, "parameters"); + this.body = requireNonNull(body, "body"); } @Override public MemberDeclaration accept(Shuttle shuttle) { From 1766b35d568072b2da7b76fe96acc446fdf28bf6 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 20 Sep 2024 14:04:32 -0700 Subject: [PATCH 16/30] Revert unintended change --- .../linq4j/tree/MethodDeclaration.java | 23 ++++--------------- 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 10acaa25063..12f9f9ebfbe 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -104,24 +104,11 @@ public MethodDeclaration(int modifier, String name, Type resultType, } MethodDeclaration that = (MethodDeclaration) o; - - if (modifier != that.modifier) { - return false; - } - if (!body.equals(that.body)) { - return false; - } - if (!name.equals(that.name)) { - return false; - } - if (!parameters.equals(that.parameters)) { - return false; - } - if (!resultType.equals(that.resultType)) { - return false; - } - - return true; + return modifier == that.modifier + && body.equals(that.body) + && name.equals(that.name) + && parameters.equals(that.parameters) + && resultType.equals(that.resultType); } @Override public int hashCode() { From cd33092a63c63315e2b5bc402731276e4bc36c6a Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 20 Sep 2024 14:06:47 -0700 Subject: [PATCH 17/30] Fix merge issues --- .../org/apache/calcite/linq4j/tree/MethodDeclaration.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 12f9f9ebfbe..5410ad2fd6c 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -16,8 +16,6 @@ */ package org.apache.calcite.linq4j.tree; -import static java.util.Objects.requireNonNull; - import org.apache.commons.lang3.StringUtils; import org.apache.flink.table.codesplit.JavaCodeSplitter; @@ -29,6 +27,8 @@ import java.util.List; import java.util.Objects; +import static java.util.Objects.requireNonNull; + /** * Declaration of a method. */ @@ -41,6 +41,7 @@ public class MethodDeclaration extends MemberDeclaration { public MethodDeclaration(int modifier, String name, Type resultType, List parameters, BlockStatement body) { + this.modifier = modifier; this.name = requireNonNull(name, "name"); this.resultType = requireNonNull(resultType, "resultType"); this.parameters = requireNonNull(parameters, "parameters"); From dc1c66d14b38ad122312b99861fca20794701504 Mon Sep 17 00:00:00 2001 From: James Duong Date: Tue, 24 Sep 2024 16:13:56 -0700 Subject: [PATCH 18/30] PR feedback - Add comments about method splitting - Add overloads with default values - Clarify condition about using a temporary writer --- .../calcite/linq4j/tree/Expressions.java | 18 ++++++++++++++++++ .../calcite/linq4j/tree/MethodDeclaration.java | 3 +++ 2 files changed, 21 insertions(+) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index 88535507673..9a0585634e9 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -57,6 +57,16 @@ private Expressions() {} * Converts a list of expressions to Java source code, optionally emitting * extra type information in generics. */ + public static String toString(List expressions, String sep, + boolean generics) { + return toString(expressions, sep, generics, false); + } + + /** + * Converts a list of expressions to Java source code, optionally emitting + * extra type information in generics. Optionally splits the generated method if + * the method is too large. + */ public static String toString(List expressions, String sep, boolean generics, boolean methodSplit) { final ExpressionWriter writer = new ExpressionWriter(generics, methodSplit); @@ -70,6 +80,14 @@ public static String toString(List expressions, String sep, /** * Converts an expression to Java source code. */ + public static String toString(Node expression) { + return toString(expression, false); + } + + /** + * Converts an expression to Java source code. + * Optionally splits the generated method if the method is too large. + */ public static String toString(Node expression, boolean methodSplit) { return toString(Collections.singletonList(expression), "", true, methodSplit); } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 5410ad2fd6c..62537218961 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -60,6 +60,9 @@ public MethodDeclaration(int modifier, String name, Type resultType, } @Override public void accept(ExpressionWriter writer) { + // Conditionally serialize the method declaration directly to the supplied writer if method + // splitting is enabled or to a temporary writer that will generate code for the method that + // can be split before being serialized to the supplied writer. final ExpressionWriter writerForUnsplitMethod = writer.usesMethodSplitting() ? writer.duplicateState() : writer; From 2ce8cac4546327a60e19892ec0a9e83da67326a0 Mon Sep 17 00:00:00 2001 From: James Duong Date: Wed, 25 Sep 2024 14:20:26 -0700 Subject: [PATCH 19/30] Turn off method-splitting for EnumUtils tests --- .../adapter/enumerable/EnumUtilsTest.java | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java b/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java index 83369e330fc..267aa39ca16 100644 --- a/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java +++ b/core/src/test/java/org/apache/calcite/adapter/enumerable/EnumUtilsTest.java @@ -47,9 +47,9 @@ public final class EnumUtilsTest { EnumUtils.convert(date, int.class); final Expression dateToInteger = EnumUtils.convert(date, Integer.class); - assertThat(Expressions.toString(dateToInt, true), + assertThat(Expressions.toString(dateToInt), is("org.apache.calcite.runtime.SqlFunctions.toInt(x)")); - assertThat(Expressions.toString(dateToInteger, true), + assertThat(Expressions.toString(dateToInteger), is("org.apache.calcite.runtime.SqlFunctions.toIntOptional(x)")); // java.sql.Time x; @@ -59,9 +59,9 @@ public final class EnumUtilsTest { EnumUtils.convert(time, int.class); final Expression timeToInteger = EnumUtils.convert(time, Integer.class); - assertThat(Expressions.toString(timeToInt, true), + assertThat(Expressions.toString(timeToInt), is("org.apache.calcite.runtime.SqlFunctions.toInt(x)")); - assertThat(Expressions.toString(timeToInteger, true), + assertThat(Expressions.toString(timeToInteger), is("org.apache.calcite.runtime.SqlFunctions.toIntOptional(x)")); // java.sql.TimeStamp x; @@ -71,9 +71,9 @@ public final class EnumUtilsTest { EnumUtils.convert(timestamp, long.class); final Expression timeStampToLong = EnumUtils.convert(timestamp, Long.class); - assertThat(Expressions.toString(timeStampToLongPrimitive, true), + assertThat(Expressions.toString(timeStampToLongPrimitive), is("org.apache.calcite.runtime.SqlFunctions.toLong(x)")); - assertThat(Expressions.toString(timeStampToLong, true), + assertThat(Expressions.toString(timeStampToLong), is("org.apache.calcite.runtime.SqlFunctions.toLongOptional(x)")); } @@ -86,7 +86,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, byte.class); final Expression converted0 = EnumUtils.convert(bytePrimitiveConverted, Byte.class); - assertThat(Expressions.toString(converted0, true), + assertThat(Expressions.toString(converted0), is("Byte.valueOf((byte) intV)")); // (char)(int) -> Character: Character.valueOf((char) intV) @@ -94,7 +94,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, char.class); final Expression converted1 = EnumUtils.convert(characterPrimitiveConverted, Character.class); - assertThat(Expressions.toString(converted1, true), + assertThat(Expressions.toString(converted1), is("Character.valueOf((char) intV)")); // (short)(int) -> Short: Short.valueOf((short) intV) @@ -102,7 +102,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, short.class); final Expression converted2 = EnumUtils.convert(shortPrimitiveConverted, Short.class); - assertThat(Expressions.toString(converted2, true), + assertThat(Expressions.toString(converted2), is("Short.valueOf((short) intV)")); // (long)(int) -> Long: Long.valueOf(intV) @@ -110,7 +110,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, long.class); final Expression converted3 = EnumUtils.convert(longPrimitiveConverted, Long.class); - assertThat(Expressions.toString(converted3, true), + assertThat(Expressions.toString(converted3), is("Long.valueOf(intV)")); // (float)(int) -> Float: Float.valueOf(intV) @@ -118,7 +118,7 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, float.class); final Expression converted4 = EnumUtils.convert(floatPrimitiveConverted, Float.class); - assertThat(Expressions.toString(converted4, true), + assertThat(Expressions.toString(converted4), is("Float.valueOf(intV)")); // (double)(int) -> Double: Double.valueOf(intV) @@ -126,37 +126,37 @@ public final class EnumUtilsTest { Expressions.convert_(intVariable, double.class); final Expression converted5 = EnumUtils.convert(doublePrimitiveConverted, Double.class); - assertThat(Expressions.toString(converted5, true), + assertThat(Expressions.toString(converted5), is("Double.valueOf(intV)")); final Expression byteConverted = EnumUtils.convert(intVariable, Byte.class); - assertThat(Expressions.toString(byteConverted, true), + assertThat(Expressions.toString(byteConverted), is("Byte.valueOf((byte) intV)")); final Expression shortConverted = EnumUtils.convert(intVariable, Short.class); - assertThat(Expressions.toString(shortConverted, true), + assertThat(Expressions.toString(shortConverted), is("Short.valueOf((short) intV)")); final Expression integerConverted = EnumUtils.convert(intVariable, Integer.class); - assertThat(Expressions.toString(integerConverted, true), + assertThat(Expressions.toString(integerConverted), is("Integer.valueOf(intV)")); final Expression longConverted = EnumUtils.convert(intVariable, Long.class); - assertThat(Expressions.toString(longConverted, true), + assertThat(Expressions.toString(longConverted), is("Long.valueOf((long) intV)")); final Expression floatConverted = EnumUtils.convert(intVariable, Float.class); - assertThat(Expressions.toString(floatConverted, true), + assertThat(Expressions.toString(floatConverted), is("Float.valueOf((float) intV)")); final Expression doubleConverted = EnumUtils.convert(intVariable, Double.class); - assertThat(Expressions.toString(doubleConverted, true), + assertThat(Expressions.toString(doubleConverted), is("Double.valueOf((double) intV)")); } @@ -167,8 +167,8 @@ public final class EnumUtilsTest { final ConstantExpression nullLiteral2 = Expressions.constant(null, Object.class); final Expression e1 = EnumUtils.convert(nullLiteral1, String.class); final Expression e2 = EnumUtils.convert(nullLiteral2, String.class); - assertThat(Expressions.toString(e1, true), is("(String) null")); - assertThat(Expressions.toString(e2, true), is("(String) (Object) null")); + assertThat(Expressions.toString(e1), is("(String) null")); + assertThat(Expressions.toString(e2), is("(String) (Object) null")); } @Test void testMethodCallExpression() { @@ -178,7 +178,7 @@ public final class EnumUtilsTest { final MethodCallExpression arrayMethodCall = EnumUtils.call(null, SqlFunctions.class, BuiltInMethod.ARRAY.getMethodName(), Arrays.asList(arg0, arg1)); - assertThat(Expressions.toString(arrayMethodCall, true), + assertThat(Expressions.toString(arrayMethodCall), is("org.apache.calcite.runtime.SqlFunctions.array(1, \"x\")")); // test for Object.class argument type @@ -187,7 +187,7 @@ public final class EnumUtilsTest { EnumUtils.call(null, XmlFunctions.class, BuiltInMethod.EXTRACT_VALUE.getMethodName(), Arrays.asList(arg1, nullLiteral)); - assertThat(Expressions.toString(xmlExtractMethodCall, true), + assertThat(Expressions.toString(xmlExtractMethodCall), is("org.apache.calcite.runtime.XmlFunctions.extractValue(\"x\", (String) null)")); // test "mod(decimal, long)" match to "mod(decimal, decimal)" @@ -196,7 +196,7 @@ public final class EnumUtilsTest { final MethodCallExpression modMethodCall = EnumUtils.call(null, SqlFunctions.class, "mod", Arrays.asList(arg2, arg3)); - assertThat(Expressions.toString(modMethodCall, true), + assertThat(Expressions.toString(modMethodCall), is("org.apache.calcite.runtime.SqlFunctions.mod(" + "java.math.BigDecimal.valueOf(125L, 1), " + "new java.math.BigDecimal(\n 3L))")); @@ -207,7 +207,7 @@ public final class EnumUtilsTest { final MethodCallExpression geoMethodCall = EnumUtils.call(null, SpatialTypeFunctions.class, "ST_MakePoint", Arrays.asList(arg4, arg5)); - assertThat(Expressions.toString(geoMethodCall, true), + assertThat(Expressions.toString(geoMethodCall), is("org.apache.calcite.runtime.SpatialTypeFunctions.ST_MakePoint(" + "new java.math.BigDecimal(\n 1), " + "new java.math.BigDecimal(\n 2))")); From 6d545dbf19c19b058b5f72103cbcefc753123089 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 27 Sep 2024 16:18:33 -0700 Subject: [PATCH 20/30] Make method splitting happen on class declarations 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 --- .../calcite/linq4j/tree/ClassDeclaration.java | 37 +++++++++-- .../linq4j/tree/MethodDeclaration.java | 30 +-------- .../calcite/linq4j/test/ExpressionTest.java | 62 +++++++++++++++++++ 3 files changed, 96 insertions(+), 33 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java index 184961e354f..b5f7b9ad23d 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java @@ -16,6 +16,9 @@ */ package org.apache.calcite.linq4j.tree; +import org.apache.commons.lang3.StringUtils; +import org.apache.flink.table.codesplit.JavaCodeSplitter; + import org.checkerframework.checker.nullness.qual.Nullable; import java.lang.reflect.Modifier; @@ -46,19 +49,41 @@ public ClassDeclaration(int modifier, String name, @Nullable Type extended, } @Override public void accept(ExpressionWriter writer) { + // Conditionally serialize the class declaration directly to the supplied writer if method + // splitting is enabled or to a temporary writer that will generate code for the method that + // can be split before being serialized to the supplied writer. + final ExpressionWriter writerForClassWithoutSplitting = writer.usesMethodSplitting() + ? writer.duplicateState() : writer; + String modifiers = Modifier.toString(modifier); - writer.append(modifiers); + writerForClassWithoutSplitting.append(modifiers); if (!modifiers.isEmpty()) { - writer.append(' '); + writerForClassWithoutSplitting.append(' '); } - writer.append(classClass).append(' ').append(name); + writerForClassWithoutSplitting.append(classClass).append(' ').append(name); if (extended != null) { - writer.append(" extends ").append(extended); + writerForClassWithoutSplitting.append(" extends ").append(extended); } if (!implemented.isEmpty()) { - writer.list(" implements ", ", ", "", implemented); + writerForClassWithoutSplitting.list(" implements ", ", ", "", implemented); + } + writerForClassWithoutSplitting.list(" {\n", "", "}", memberDeclarations); + + if (writer.usesMethodSplitting()) { + // Specifies a threshold where generated code will be split into sub-function calls. + // Java has a maximum method length of 64 KB. This setting allows for finer granularity if + // necessary. + // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with + // more than 8K byte code. + final int defaultMaxGeneratedCodeLength = 4000; + final int defaultMaxMembersGeneratedCode = 10000; + + writer.append( + StringUtils.stripStart( + JavaCodeSplitter.split(writerForClassWithoutSplitting.toString(), + defaultMaxGeneratedCodeLength, defaultMaxMembersGeneratedCode), + " ")); } - writer.list(" {\n", "", "}", memberDeclarations); writer.newlineAndIndent(); } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 62537218961..97878f4c83d 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -16,9 +16,6 @@ */ package org.apache.calcite.linq4j.tree; -import org.apache.commons.lang3.StringUtils; -import org.apache.flink.table.codesplit.JavaCodeSplitter; - import org.checkerframework.checker.nullness.qual.Nullable; import java.lang.reflect.Modifier; @@ -60,19 +57,13 @@ public MethodDeclaration(int modifier, String name, Type resultType, } @Override public void accept(ExpressionWriter writer) { - // Conditionally serialize the method declaration directly to the supplied writer if method - // splitting is enabled or to a temporary writer that will generate code for the method that - // can be split before being serialized to the supplied writer. - final ExpressionWriter writerForUnsplitMethod = writer.usesMethodSplitting() - ? writer.duplicateState() : writer; - final String modifiers = Modifier.toString(modifier); - writerForUnsplitMethod.append(modifiers); + writer.append(modifiers); if (!modifiers.isEmpty()) { - writerForUnsplitMethod.append(' '); + writer.append(' '); } //noinspection unchecked - writerForUnsplitMethod + writer .append(resultType) .append(' ') .append(name) @@ -81,21 +72,6 @@ public MethodDeclaration(int modifier, String name, Type resultType, .append(' ') .append(body); - if (writer.usesMethodSplitting()) { - // Specifies a threshold where generated code will be split into sub-function calls. - // Java has a maximum method length of 64 KB. This setting allows for finer granularity if - // necessary. - // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with - // more than 8K byte code. - final int defaultMaxGeneratedCodeLength = 4000; - final int defaultMaxMembersGeneratedCode = 10000; - - writer.append( - StringUtils.stripStart( - JavaCodeSplitter.split(writerForUnsplitMethod.toString(), - defaultMaxGeneratedCodeLength, defaultMaxMembersGeneratedCode), - " ")); - } writer.newlineAndIndent(); } diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index f3b405146c6..e949431fe13 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -21,24 +21,29 @@ import org.apache.calcite.linq4j.tree.BlockStatement; import org.apache.calcite.linq4j.tree.Blocks; import org.apache.calcite.linq4j.tree.ClassDeclaration; +import org.apache.calcite.linq4j.tree.ConditionalStatement; import org.apache.calcite.linq4j.tree.DeclarationStatement; import org.apache.calcite.linq4j.tree.Expression; import org.apache.calcite.linq4j.tree.Expressions; import org.apache.calcite.linq4j.tree.FieldDeclaration; import org.apache.calcite.linq4j.tree.FunctionExpression; import org.apache.calcite.linq4j.tree.MethodCallExpression; +import org.apache.calcite.linq4j.tree.MethodDeclaration; import org.apache.calcite.linq4j.tree.NewExpression; import org.apache.calcite.linq4j.tree.Node; import org.apache.calcite.linq4j.tree.ParameterExpression; import org.apache.calcite.linq4j.tree.Shuttle; import org.apache.calcite.linq4j.tree.Types; +import org.apache.commons.lang3.StringUtils; + import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import org.checkerframework.checker.nullness.qual.Nullable; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -1719,6 +1724,63 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.toString(Expressions.constant(set), true)); } + @Test void testLargeMethod() { +// public class MyClass { +// public void hugeMethod() { +// if (true) { +// // Long variable name declaration 1; +// } else if (false) { +// // Long variable name declaration 2; +// } else if (false) { +// // Long variable name declaration 3; +// } else if (false) { +// // Long variable name declaration 4; +// } else +// // Long variable name declaration 4; +// } +// } + + // Generate long variable names. + final String var1Name = StringUtils.repeat('a', 1000); + final String var2Name = StringUtils.repeat('b', 1000); + + DeclarationStatement xDecl = + Expressions.declare(0, var1Name, Expressions.constant(10)); + DeclarationStatement yDecl = + Expressions.declare(0, var2Name, Expressions.constant(0)); + final ConditionalStatement ifThenElse = + Expressions.ifThenElse( + Expressions.constant(true), + xDecl, + Expressions.constant(false), + yDecl, + Expressions.constant(false), + yDecl, + yDecl); + + final MethodDeclaration hugeMethod = + Expressions.methodDecl(Modifier.PUBLIC, + Void.TYPE, + "hugeMethod", + Collections.emptyList(), + Blocks.toFunctionBlock(ifThenElse)); + + final ClassDeclaration classDecl = + new ClassDeclaration(Modifier.PUBLIC, + "MyClass", + null, + ImmutableList.of(), + ImmutableList.of(hugeMethod)); + + final String originalClass = Expressions.toString(classDecl); + final String classWithMethodSplitting = Expressions.toString(classDecl, true); + + // Some local variables in hugeMethod have been extracted to be fields named local$ in + // the enclosing class definition. + Assertions.assertFalse(originalClass.contains("local$")); + Assertions.assertTrue(classWithMethodSplitting.contains("local$")); + } + /** An enum. */ enum MyEnum { X, From 6200d822c4f08d8e08c0a04e412935326a776996 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 27 Sep 2024 16:49:27 -0700 Subject: [PATCH 21/30] Simplify the test case --- .../calcite/linq4j/test/ExpressionTest.java | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index e949431fe13..7c9573d4c6e 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -1741,22 +1741,19 @@ public void checkBlockBuilder(boolean optimizing, String expected) { // } // Generate long variable names. - final String var1Name = StringUtils.repeat('a', 1000); - final String var2Name = StringUtils.repeat('b', 1000); + final String longName = StringUtils.repeat('a', 5000); - DeclarationStatement xDecl = - Expressions.declare(0, var1Name, Expressions.constant(10)); - DeclarationStatement yDecl = - Expressions.declare(0, var2Name, Expressions.constant(0)); + DeclarationStatement longDecl = + Expressions.declare(0, longName, Expressions.constant(10)); final ConditionalStatement ifThenElse = Expressions.ifThenElse( Expressions.constant(true), - xDecl, + longDecl, Expressions.constant(false), - yDecl, + longDecl, Expressions.constant(false), - yDecl, - yDecl); + longDecl, + longDecl); final MethodDeclaration hugeMethod = Expressions.methodDecl(Modifier.PUBLIC, From 39e14a406bfe27f5f957869b69fe6745c3a9ab41 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 27 Sep 2024 17:08:59 -0700 Subject: [PATCH 22/30] Fix typo in comment --- .../java/org/apache/calcite/linq4j/tree/ClassDeclaration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java index b5f7b9ad23d..e7ecd0c3b1c 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java @@ -50,7 +50,7 @@ public ClassDeclaration(int modifier, String name, @Nullable Type extended, @Override public void accept(ExpressionWriter writer) { // Conditionally serialize the class declaration directly to the supplied writer if method - // splitting is enabled or to a temporary writer that will generate code for the method that + // splitting is disabled or to a temporary writer that will generate code for the method that // can be split before being serialized to the supplied writer. final ExpressionWriter writerForClassWithoutSplitting = writer.usesMethodSplitting() ? writer.duplicateState() : writer; From d969ee4f054ecea23bd2599a9a5c8f4713f2c472 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 27 Sep 2024 17:11:43 -0700 Subject: [PATCH 23/30] Make configuration of method splitting a threshold instead of a toggle 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. --- .../enumerable/EnumerableInterpretable.java | 2 +- .../calcite/config/CalciteSystemProperty.java | 11 +- .../interpreter/JaninoRexCompiler.java | 2 +- .../apache/calcite/rex/RexExecutorImpl.java | 2 +- .../adapter/enumerable/PhysTypeTest.java | 2 +- .../adapter/enumerable/TypeFinderTest.java | 2 +- .../innodb/InnodbToEnumerableConverter.java | 2 +- .../calcite/linq4j/tree/AbstractNode.java | 3 +- .../calcite/linq4j/tree/ClassDeclaration.java | 8 +- .../calcite/linq4j/tree/ExpressionWriter.java | 17 +- .../calcite/linq4j/tree/Expressions.java | 12 +- .../calcite/linq4j/test/BlockBuilderTest.java | 8 +- .../calcite/linq4j/test/ExpressionTest.java | 202 +++++++++--------- .../calcite/linq4j/test/InlinerTest.java | 8 +- .../calcite/linq4j/test/Linq4jTest.java | 20 +- 15 files changed, 152 insertions(+), 149 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java index 9db29216edd..8a5bbad0b0f 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java @@ -114,7 +114,7 @@ public static Bindable toBindable(Map parameters, String s = Expressions.toString(expr.memberDeclarations, "\n", false, - CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value()); + CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); diff --git a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java index ce0c592f010..093e1a05ee2 100644 --- a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java +++ b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java @@ -420,12 +420,15 @@ public final class CalciteSystemProperty { public static final CalciteSystemProperty FUNCTION_LEVEL_CACHE_MAX_SIZE = intProperty("calcite.function.cache.maxSize", 0, v -> v >= 0); - /** Whether to enable automatic method splitting when generating Java code. + /** The length of code before automatic method splitting is enabled + * + * A value of 0, the default, disables automatic method splitting. * *

Some queries can generate methods exceeding the JVM limit of 4000 characters per method. - * Enable this feature to automatically detect and split methods larger than the limit. */ - public static final CalciteSystemProperty ENABLE_METHOD_SPLITTING = - booleanProperty("calcite.linq.enable_method_splitting", false); + * Use this setting to automatically detect and split methods larger than the specified + * limit. */ + public static final CalciteSystemProperty METHOD_SPLITTING_THRESHOLD = + intProperty("calcite.linq.method_splitting_threshold", 0); private static CalciteSystemProperty booleanProperty(String key, boolean defaultValue) { diff --git a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java b/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java index cf21f632819..b87b7674b8c 100644 --- a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java +++ b/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java @@ -188,7 +188,7 @@ static Scalar.Producer baz(ParameterExpression context_, ImmutableList.of(Scalar.Producer.class), declarations); String s = Expressions.toString(declarations, "\n", false, - CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value()); + CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); } diff --git a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java index 9ec13ec10a3..86c6126688d 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java +++ b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java @@ -101,7 +101,7 @@ private static String compile(RexBuilder rexBuilder, List constExps, BuiltInMethod.FUNCTION1_APPLY.method.getName(), ImmutableList.of(root0_), blockBuilder.toBlock()); String code = - Expressions.toString(methodDecl, CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value()); + Expressions.toString(methodDecl, CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, code); } diff --git a/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java b/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java index e094d43985e..98dc6a9cd1e 100644 --- a/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java +++ b/core/src/test/java/org/apache/calcite/adapter/enumerable/PhysTypeTest.java @@ -95,6 +95,6 @@ public final class PhysTypeTest { + " }\n" + "}\n" + ")"; - assertEquals(Expressions.toString(e, true), expected); + assertEquals(Expressions.toString(e), expected); } } diff --git a/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java b/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java index 2ac9b616dab..583bcd8d670 100644 --- a/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java +++ b/core/src/test/java/org/apache/calcite/adapter/enumerable/TypeFinderTest.java @@ -97,7 +97,7 @@ private void assertJavaCodeContains(String expected, Node node) { } private void assertJavaCodeContains(String expected, List nodes) { - final String javaCode = Expressions.toString(nodes, "\n", false, true); + final String javaCode = Expressions.toString(nodes, "\n", false); assertThat(javaCode, containsString(expected)); } diff --git a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java index d56e2ca0871..26184cd4c52 100644 --- a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java +++ b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java @@ -152,7 +152,7 @@ static List innodbFieldNames(final RelDataType rowType) { if (CalciteSystemProperty.DEBUG.value()) { System.out.println( "Innodb: " + Expressions.toString(enumerable, - CalciteSystemProperty.ENABLE_METHOD_SPLITTING.value())); + CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value())); } list.add(Expressions.return_(null, enumerable)); return implementor.result(physType, list.toBlock()); diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java index 58bc4134c38..b4dbdc6dcc7 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java +++ b/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, true); + // Use the JVM limit for method size (4K) as the method splitting threshold. + ExpressionWriter writer = new ExpressionWriter(true, 4000); accept(writer, 0, 0); return writer.toString(); } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java index e7ecd0c3b1c..fd9f175ae5a 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java @@ -70,18 +70,12 @@ public ClassDeclaration(int modifier, String name, @Nullable Type extended, writerForClassWithoutSplitting.list(" {\n", "", "}", memberDeclarations); if (writer.usesMethodSplitting()) { - // Specifies a threshold where generated code will be split into sub-function calls. - // Java has a maximum method length of 64 KB. This setting allows for finer granularity if - // necessary. - // Default value is 4000 instead of 64KB as by default JIT refuses to work on methods with - // more than 8K byte code. - final int defaultMaxGeneratedCodeLength = 4000; final int defaultMaxMembersGeneratedCode = 10000; writer.append( StringUtils.stripStart( JavaCodeSplitter.split(writerForClassWithoutSplitting.toString(), - defaultMaxGeneratedCodeLength, defaultMaxMembersGeneratedCode), + writer.getMethodSplittingThreshold(), defaultMaxMembersGeneratedCode), " ")); } writer.newlineAndIndent(); diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java index 616ff8b0edd..7c1bd5be410 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java @@ -33,20 +33,21 @@ class ExpressionWriter { private final Spacer spacer = new Spacer(0); private final StringBuilder buf = new StringBuilder(); private final boolean generics; - private final boolean methodSplitting; + private final int methodSplittingThreshold; private boolean indentPending; ExpressionWriter() { - this(true, true); + this(true, 0); } - ExpressionWriter(boolean generics, boolean methodSplitting) { + ExpressionWriter(boolean generics, int methodSplittingThreshold) { this.generics = generics; - this.methodSplitting = methodSplitting; + this.methodSplittingThreshold = methodSplittingThreshold; } public ExpressionWriter duplicateState() { - final ExpressionWriter writer = new ExpressionWriter(this.generics, this.methodSplitting); + final ExpressionWriter writer = + new ExpressionWriter(this.generics, this.methodSplittingThreshold); writer.indentPending = this.indentPending; writer.spacer.add(this.spacer.get()); return writer; @@ -83,7 +84,11 @@ public boolean requireParentheses(Expression expression, int lprec, } public boolean usesMethodSplitting() { - return methodSplitting; + return methodSplittingThreshold != 0; + } + + public int getMethodSplittingThreshold() { + return methodSplittingThreshold; } /** diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index 9a0585634e9..796a1828952 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -59,7 +59,7 @@ private Expressions() {} */ public static String toString(List expressions, String sep, boolean generics) { - return toString(expressions, sep, generics, false); + return toString(expressions, sep, generics); } /** @@ -68,8 +68,8 @@ public static String toString(List expressions, String sep, * the method is too large. */ public static String toString(List expressions, String sep, - boolean generics, boolean methodSplit) { - final ExpressionWriter writer = new ExpressionWriter(generics, methodSplit); + boolean generics, int methodSplittingThreshold) { + final ExpressionWriter writer = new ExpressionWriter(generics, methodSplittingThreshold); for (Node expression : expressions) { writer.write(expression); writer.append(sep); @@ -81,15 +81,15 @@ public static String toString(List expressions, String sep, * Converts an expression to Java source code. */ public static String toString(Node expression) { - return toString(expression, false); + return toString(expression, 0); } /** * Converts an expression to Java source code. * Optionally splits the generated method if the method is too large. */ - public static String toString(Node expression, boolean methodSplit) { - return toString(Collections.singletonList(expression), "", true, methodSplit); + public static String toString(Node expression, int methodSplittingTheshold) { + return toString(Collections.singletonList(expression), "", true, methodSplittingTheshold); } /** diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java index b3b20a41b7c..07023ae48df 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/BlockBuilderTest.java @@ -108,7 +108,7 @@ private BlockBuilder appendBlockWithSameVariable( + " x = 1;\n" + " int x0;\n" + " x0 = 42;\n" - + "}\n", Expressions.toString(outer.toBlock(), true), + + "}\n", Expressions.toString(outer.toBlock()), "x in the second block should be renamed to avoid name clash"); } @@ -122,7 +122,7 @@ private BlockBuilder appendBlockWithSameVariable( + " x = 1;\n" + " int x0 = 8;\n" + " x0 = 42;\n" - + "}\n", Expressions.toString(outer.toBlock(), true), + + "}\n", Expressions.toString(outer.toBlock()), "x in the second block should be renamed to avoid name clash"); } @@ -143,7 +143,7 @@ private BlockBuilder appendBlockWithSameVariable( + " final Object _i = new org.apache.calcite.linq4j.test.BlockBuilderTest.Identity()" + ".apply(\"test\");\n" + "}\n", - Expressions.toString(bb.toBlock(), true)); + Expressions.toString(bb.toBlock())); } @@ -160,7 +160,7 @@ private BlockBuilder appendBlockWithSameVariable( assertEquals("{\n" + " return false;\n" - + "}\n", Expressions.toString(outer.toBlock(), true), + + "}\n", Expressions.toString(outer.toBlock()), "Expected to optimize Boolean.FALSE = null to false"); } diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index 7c9573d4c6e..13427685863 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -95,7 +95,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(int arg) {\n" @@ -138,7 +138,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(short arg) {\n" @@ -181,7 +181,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public int apply(byte arg) {\n" @@ -223,7 +223,7 @@ public class ExpressionTest { Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public double apply(double arg) {\n" @@ -264,7 +264,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(2L)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public long apply(long arg) {\n" @@ -305,7 +305,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(2.0f)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public float apply(float arg) {\n" @@ -346,7 +346,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(10)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public long apply(long arg) {\n" @@ -387,7 +387,7 @@ public class ExpressionTest { Expressions.add(paramExpr, Expressions.constant(10.1d)), Arrays.asList(paramExpr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals( "new org.apache.calcite.linq4j.function.Function1() {\n" + " public double apply(double arg) {\n" @@ -430,7 +430,7 @@ public class ExpressionTest { Arrays.asList(paramExpr, param2Expr)); // Print out the expression. - String s = Expressions.toString(lambdaExpr, true); + String s = Expressions.toString(lambdaExpr); assertEquals("new org.apache.calcite.linq4j.function.Function2() {\n" + " public int apply(int key, int key2) {\n" + " return key;\n" @@ -482,11 +482,11 @@ public class ExpressionTest { assertEquals( "true", Expressions.toString( - Expressions.foldAnd(list0), true)); + Expressions.foldAnd(list0))); assertEquals( "false", Expressions.toString( - Expressions.foldOr(list0), true)); + Expressions.foldOr(list0))); final List list1 = Arrays.asList( @@ -499,12 +499,12 @@ public class ExpressionTest { assertEquals( "1 == 2 && 3 == 4 && 5 == 6", Expressions.toString( - Expressions.foldAnd(list1), true)); + Expressions.foldAnd(list1))); // a single true makes OR true assertEquals( "true", Expressions.toString( - Expressions.foldOr(list1), true)); + Expressions.foldOr(list1))); final List list2 = Collections.singletonList( @@ -512,11 +512,11 @@ public class ExpressionTest { assertEquals( "true", Expressions.toString( - Expressions.foldAnd(list2), true)); + Expressions.foldAnd(list2))); assertEquals( "true", Expressions.toString( - Expressions.foldOr(list2), true)); + Expressions.foldOr(list2))); final List list3 = Arrays.asList( @@ -528,11 +528,11 @@ public class ExpressionTest { assertEquals( "false", Expressions.toString( - Expressions.foldAnd(list3), true)); + Expressions.foldAnd(list3))); assertEquals( "1 == 2 || 5 == 6", Expressions.toString( - Expressions.foldOr(list3), true)); + Expressions.foldOr(list3))); } @Test void testWrite() { @@ -545,13 +545,13 @@ public class ExpressionTest { Expressions.constant(1), Expressions.constant(2F, Float.TYPE)), Expressions.constant(3L, Long.TYPE)), - Expressions.constant(4L, Long.class)), true)); + Expressions.constant(4L, Long.class)))); assertEquals( "java.math.BigDecimal.valueOf(31415926L, 7)", Expressions.toString( Expressions.constant( - BigDecimal.valueOf(314159260, 8)), true)); + BigDecimal.valueOf(314159260, 8)))); // Parentheses needed, to override the left-associativity of +. assertEquals( @@ -561,7 +561,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.add( Expressions.constant(2), - Expressions.constant(3))), true)); + Expressions.constant(3))))); // No parentheses needed; higher precedence of * achieves the desired // effect. @@ -572,7 +572,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.multiply( Expressions.constant(2), - Expressions.constant(3))), true)); + Expressions.constant(3))))); assertEquals( "1 * (2 + 3)", @@ -581,7 +581,7 @@ public class ExpressionTest { Expressions.constant(1), Expressions.add( Expressions.constant(2), - Expressions.constant(3))), true)); + Expressions.constant(3))))); // Parentheses needed, to overcome right-associativity of =. assertEquals( @@ -590,7 +590,7 @@ public class ExpressionTest { Expressions.assign( Expressions.assign( Expressions.constant(1), Expressions.constant(2)), - Expressions.constant(3)), true)); + Expressions.constant(3)))); // Ternary operator. assertEquals( @@ -611,7 +611,7 @@ public class ExpressionTest { Expressions.constant(7), Expressions.constant(8)), Expressions.constant(9), - Expressions.constant(10))), true)); + Expressions.constant(10))))); assertEquals( "0 + (double) (2 + 3)", @@ -621,7 +621,7 @@ public class ExpressionTest { Expressions.convert_( Expressions.add( Expressions.constant(2), Expressions.constant(3)), - Double.TYPE)), true)); + Double.TYPE)))); // "--5" would be a syntax error assertEquals( @@ -629,27 +629,27 @@ public class ExpressionTest { Expressions.toString( Expressions.negate( Expressions.negate( - Expressions.constant(5))), true)); + Expressions.constant(5))))); assertEquals( "a.empno", Expressions.toString( Expressions.field( Expressions.parameter(Linq4jTest.Employee.class, "a"), - "empno"), true)); + "empno"))); assertEquals( "a.length", Expressions.toString( Expressions.field( Expressions.parameter(Object[].class, "a"), - "length"), true)); + "length"))); assertEquals( "java.util.Collections.EMPTY_LIST", Expressions.toString( Expressions.field( - null, Collections.class, "EMPTY_LIST"), true)); + null, Collections.class, "EMPTY_LIST"))); final ParameterExpression paramX = Expressions.parameter(String.class, "x"); @@ -668,7 +668,7 @@ public class ExpressionTest { Function1.class, Expressions.call( paramX, "length", Collections.emptyList()), - Arrays.asList(paramX)), true)); + Arrays.asList(paramX)))); // 1-dimensional array with initializer assertEquals( @@ -681,7 +681,7 @@ public class ExpressionTest { String.class, Expressions.constant("foo"), Expressions.constant(null), - Expressions.constant("bar\"baz")), true)); + Expressions.constant("bar\"baz")))); // 2-dimensional array with initializer assertEquals( @@ -698,7 +698,7 @@ public class ExpressionTest { 2, Expressions.constant(new String[] {"foo", "bar"}), Expressions.constant(null), - Expressions.constant(new String[] {null})), true)); + Expressions.constant(new String[] {null})))); // 1-dimensional array assertEquals( @@ -709,7 +709,7 @@ public class ExpressionTest { 1, Expressions.add( Expressions.parameter(0, int.class, "x"), - Expressions.constant(1))), true)); + Expressions.constant(1))))); // 3-dimensional array assertEquals( @@ -720,7 +720,7 @@ public class ExpressionTest { 3, Expressions.add( Expressions.parameter(0, int.class, "x"), - Expressions.constant(1))), true)); + Expressions.constant(1))))); assertEquals( "(int) ((String) (Object) \"foo\").length()", @@ -734,7 +734,7 @@ public class ExpressionTest { String.class), "length", Collections.emptyList()), - Integer.TYPE), true)); + Integer.TYPE))); // resolving a static method assertEquals( @@ -744,7 +744,7 @@ public class ExpressionTest { Integer.class, "valueOf", Collections.singletonList( - Expressions.constant("0123"))), true)); + Expressions.constant("0123"))))); // precedence of not and instanceof assertEquals( @@ -753,7 +753,7 @@ public class ExpressionTest { Expressions.not( Expressions.typeIs( Expressions.parameter(Object.class, "o"), - String.class)), true)); + String.class)))); // not not assertEquals( @@ -763,7 +763,7 @@ public class ExpressionTest { Expressions.not( Expressions.typeIs( Expressions.parameter(Object.class, "o"), - String.class))), true)); + String.class))))); } @Test void testWriteConstant() { @@ -774,65 +774,65 @@ public class ExpressionTest { + " 2,\n" + " -1}", Expressions.toString( - Expressions.constant(new int[]{1, 2, -1}), true)); + Expressions.constant(new int[]{1, 2, -1}))); // primitive assertEquals( "-12", Expressions.toString( - Expressions.constant(-12), true)); + Expressions.constant(-12))); assertEquals( "(short)-12", Expressions.toString( - Expressions.constant((short) -12), true)); + Expressions.constant((short) -12))); assertEquals( "(byte)-12", Expressions.toString( - Expressions.constant((byte) -12), true)); + Expressions.constant((byte) -12))); // boxed primitives assertEquals( "Integer.valueOf(1)", Expressions.toString( - Expressions.constant(1, Integer.class), true)); + Expressions.constant(1, Integer.class))); assertEquals( "Double.valueOf(-3.14D)", Expressions.toString( - Expressions.constant(-3.14, Double.class), true)); + Expressions.constant(-3.14, Double.class))); assertEquals( "Boolean.valueOf(true)", Expressions.toString( - Expressions.constant(true, Boolean.class), true)); + Expressions.constant(true, Boolean.class))); // primitive with explicit class assertEquals( "1", Expressions.toString( - Expressions.constant(1, int.class), true)); + Expressions.constant(1, int.class))); assertEquals( "(short)1", Expressions.toString( - Expressions.constant(1, short.class), true)); + Expressions.constant(1, short.class))); assertEquals( "(byte)1", Expressions.toString( - Expressions.constant(1, byte.class), true)); + Expressions.constant(1, byte.class))); assertEquals( "-3.14D", Expressions.toString( - Expressions.constant(-3.14, double.class), true)); + Expressions.constant(-3.14, double.class))); assertEquals( "true", Expressions.toString( - Expressions.constant(true, boolean.class), true)); + Expressions.constant(true, boolean.class))); // objects and nulls assertEquals( @@ -840,19 +840,19 @@ public class ExpressionTest { + " \"foo\",\n" + " null}", Expressions.toString( - Expressions.constant(new String[] {"foo", null}), true)); + Expressions.constant(new String[] {"foo", null}))); // string assertEquals( "\"hello, \\\"world\\\"!\"", Expressions.toString( - Expressions.constant("hello, \"world\"!"), true)); + Expressions.constant("hello, \"world\"!"))); // enum assertEquals( "org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.X", Expressions.toString( - Expressions.constant(MyEnum.X), true)); + Expressions.constant(MyEnum.X))); // array of enum assertEquals( @@ -860,24 +860,24 @@ public class ExpressionTest { + " org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.X,\n" + " org.apache.calcite.linq4j.test.ExpressionTest.MyEnum.Y}", Expressions.toString( - Expressions.constant(new MyEnum[]{MyEnum.X, MyEnum.Y}), true)); + Expressions.constant(new MyEnum[]{MyEnum.X, MyEnum.Y}))); // class assertEquals( "java.lang.String.class", Expressions.toString( - Expressions.constant(String.class), true)); + Expressions.constant(String.class))); // array class assertEquals( "int[].class", Expressions.toString( - Expressions.constant(int[].class), true)); + Expressions.constant(int[].class))); assertEquals( "java.util.List[][].class", Expressions.toString( - Expressions.constant(List[][].class), true)); + Expressions.constant(List[][].class))); // automatically call constructor if it matches fields assertEquals( @@ -899,7 +899,7 @@ public class ExpressionTest { + " \"Janet\",\n" + " 10)}", Expressions.toString( - Expressions.constant(Linq4jTest.emps), true)); + Expressions.constant(Linq4jTest.emps))); } /** Test case for @@ -924,7 +924,7 @@ public class ExpressionTest { ImmutableSet.of(createInstance(recordClass, "test1", 1), createInstance(recordClass, "test2", 2), createInstance(recordClass, "test3", 3), - createInstance(recordClass, "test4", 4))), true)); + createInstance(recordClass, "test4", 4))))); } @Test void testWriteArray() { @@ -937,7 +937,7 @@ public class ExpressionTest { Expressions.variable(int[].class, "integers"), Expressions.add( Expressions.constant(2), - Expressions.variable(int.class, "index")))), true)); + Expressions.variable(int.class, "index")))))); } @Test void testWriteAnonymousClass() { @@ -1021,7 +1021,7 @@ public class ExpressionTest { + "\n" + " };\n" + "}\n", - Expressions.toString(e, true)); + Expressions.toString(e)); } @Test void testWriteWhile() { @@ -1076,7 +1076,7 @@ public class ExpressionTest { + "} finally {\n" + " \"foo\".toUpperCase();\n" + "}\n", - Expressions.toString(node, true)); + Expressions.toString(node)); } @Test void testWriteTryFinally() { @@ -1101,7 +1101,7 @@ public class ExpressionTest { + " \"foo\".toUpperCase();\n" + " }\n" + "}\n", - Expressions.toString(node, true)); + Expressions.toString(node)); } @Test void testWriteTryCatch() { @@ -1127,7 +1127,7 @@ public class ExpressionTest { + "} catch (RuntimeException re) {\n" + " return re.toString();\n" + "}\n", - Expressions.toString(node, true)); + Expressions.toString(node)); } @Test void testType() { @@ -1242,7 +1242,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { nine)); statements.add(Expressions.return_(null, eighteen)); BlockStatement expression = statements.toBlock(); - assertEquals(expected, Expressions.toString(expression, true)); + assertEquals(expected, Expressions.toString(expression)); expression.accept(new Shuttle()); } @@ -1273,7 +1273,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " (java.util.Comparator) null);\n" + " return treeSet.add(null);\n" + "}\n"; - assertThat(Expressions.toString(expression, true), is(expected)); + assertThat(Expressions.toString(expression), is(expected)); expression.accept(new Shuttle()); } @@ -1322,7 +1322,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " final int _b0 = 1 + 3;\n" + " org.apache.calcite.linq4j.test.ExpressionTest.bar(1, _b, _c, _d, org.apache.calcite.linq4j.test.ExpressionTest.foo(_b0));\n" + "}\n", - Expressions.toString(expression, true)); + Expressions.toString(expression)); expression.accept(new Shuttle()); } @@ -1371,32 +1371,32 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testBigDecimalConstantExpression() { assertEquals("java.math.BigDecimal.valueOf(104L)", - Expressions.toString(Expressions.constant("104", BigDecimal.class), true)); + Expressions.toString(Expressions.constant("104", BigDecimal.class))); assertEquals("java.math.BigDecimal.valueOf(1L, -3)", - Expressions.toString(Expressions.constant("1000", BigDecimal.class), true)); + Expressions.toString(Expressions.constant("1000", BigDecimal.class))); assertEquals("java.math.BigDecimal.valueOf(1L, -3)", - Expressions.toString(Expressions.constant(1000, BigDecimal.class), true)); + Expressions.toString(Expressions.constant(1000, BigDecimal.class))); assertEquals("java.math.BigDecimal.valueOf(107L)", - Expressions.toString(Expressions.constant(107, BigDecimal.class), true)); + Expressions.toString(Expressions.constant(107, BigDecimal.class))); assertEquals("java.math.BigDecimal.valueOf(199999999999999L)", - Expressions.toString(Expressions.constant(199999999999999L, BigDecimal.class), true)); + Expressions.toString(Expressions.constant(199999999999999L, BigDecimal.class))); assertEquals("java.math.BigDecimal.valueOf(1234L, 2)", - Expressions.toString(Expressions.constant(12.34, BigDecimal.class), true)); + Expressions.toString(Expressions.constant(12.34, BigDecimal.class))); } @Test void testObjectConstantExpression() { assertEquals("(byte)100", - Expressions.toString(Expressions.constant((byte) 100, Object.class), true)); + Expressions.toString(Expressions.constant((byte) 100, Object.class))); assertEquals("(char)100", - Expressions.toString(Expressions.constant((char) 100, Object.class), true)); + Expressions.toString(Expressions.constant((char) 100, Object.class))); assertEquals("(short)100", - Expressions.toString(Expressions.constant((short) 100, Object.class), true)); + Expressions.toString(Expressions.constant((short) 100, Object.class))); assertEquals("100L", - Expressions.toString(Expressions.constant(100L, Object.class), true)); + Expressions.toString(Expressions.constant(100L, Object.class))); assertEquals("100.0F", - Expressions.toString(Expressions.constant(100F, Object.class), true)); + Expressions.toString(Expressions.constant(100F, Object.class))); assertEquals("100.0D", - Expressions.toString(Expressions.constant(100D, Object.class), true)); + Expressions.toString(Expressions.constant(100D, Object.class))); } @Test void testClassDecl() { @@ -1430,7 +1430,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " }\n" + " int i;\n" + "}", - Expressions.toString(newExpression, true)); + Expressions.toString(newExpression)); newExpression.accept(new Shuttle()); } @@ -1445,7 +1445,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.ifThenElse( Expressions.constant(true), Expressions.return_(null), - Expressions.return_(null, Expressions.constant(1))), false)); + Expressions.return_(null, Expressions.constant(1))))); } @Test void testIfElseIfElse() { @@ -1463,7 +1463,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.return_(null), Expressions.constant(false), Expressions.return_(null), - Expressions.return_(null, Expressions.constant(1))), true)); + Expressions.return_(null, Expressions.constant(1))))); } /** Test for common sub-expression elimination. */ @@ -1510,7 +1510,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " return (Number) v == null ? null : (" + "(Number) v).intValue() == 1997;\n" + "}\n", - Expressions.toString(builder.toBlock(), true)); + Expressions.toString(builder.toBlock())); } @Test void testFor() throws NoSuchFieldException { @@ -1535,7 +1535,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " System.out.println(i);\n" + " }\n" + "}\n", - Expressions.toString(builder.toBlock(), true)); + Expressions.toString(builder.toBlock())); } @Test void testFor2() { @@ -1565,7 +1565,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " }\n" + " }\n" + "}\n", - Expressions.toString(builder.toBlock(), true)); + Expressions.toString(builder.toBlock())); } @Test void testForEach() { @@ -1579,7 +1579,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.constant(1), Expressions.constant(2)), Expressions.break_(null)))); - assertThat(Expressions.toString(builder.toBlock(), true), + assertThat(Expressions.toString(builder.toBlock()), is("{\n" + " for (int i : list) {\n" + " if (1 < 2) {\n" @@ -1591,18 +1591,18 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testEmptyListLiteral() { assertEquals("java.util.Collections.EMPTY_LIST", - Expressions.toString(Expressions.constant(Arrays.asList()), true)); + Expressions.toString(Expressions.constant(Arrays.asList()))); } @Test void testOneElementListLiteral() { assertEquals("java.util.Arrays.asList(1)", - Expressions.toString(Expressions.constant(Arrays.asList(1)), true)); + Expressions.toString(Expressions.constant(Arrays.asList(1)))); } @Test void testTwoElementsListLiteral() { assertEquals("java.util.Arrays.asList(1,\n" + " 2)", - Expressions.toString(Expressions.constant(Arrays.asList(1, 2)), true)); + Expressions.toString(Expressions.constant(Arrays.asList(1, 2)))); } @Test void testNestedListsLiteral() { @@ -1612,23 +1612,23 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " 4))", Expressions.toString( Expressions.constant( - Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4))), true)); + Arrays.asList(Arrays.asList(1, 2), Arrays.asList(3, 4))))); } @Test void testEmptyMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of()", - Expressions.toString(Expressions.constant(new HashMap<>()), true)); + Expressions.toString(Expressions.constant(new HashMap<>()))); } @Test void testOneElementMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of(\"abc\", 42)", - Expressions.toString(Expressions.constant(Collections.singletonMap("abc", 42)), true)); + Expressions.toString(Expressions.constant(Collections.singletonMap("abc", 42)))); } @Test void testTwoElementsMapLiteral() { assertEquals("com.google.common.collect.ImmutableMap.of(\"abc\", 42,\n" + "\"def\", 43)", - Expressions.toString(Expressions.constant(ImmutableMap.of("abc", 42, "def", 43)), true)); + Expressions.toString(Expressions.constant(ImmutableMap.of("abc", 42, "def", 43)))); } @Test void testTenElementsMapLiteral() { @@ -1646,7 +1646,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".put(\"key_7\", \"value_7\")\n" + ".put(\"key_8\", \"value_8\")\n" + ".put(\"key_9\", \"value_9\").build()", - Expressions.toString(Expressions.constant(map), true)); + Expressions.toString(Expressions.constant(map))); } @Test void testEvaluate() { @@ -1657,17 +1657,17 @@ public void checkBlockBuilder(boolean optimizing, String expected) { @Test void testEmptySetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of()", - Expressions.toString(Expressions.constant(new HashSet<>()), true)); + Expressions.toString(Expressions.constant(new HashSet<>()))); } @Test void testOneElementSetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of(1)", - Expressions.toString(Expressions.constant(Sets.newHashSet(1)), true)); + Expressions.toString(Expressions.constant(Sets.newHashSet(1)))); } @Test void testTwoElementsSetLiteral() { assertEquals("com.google.common.collect.ImmutableSet.of(1,2)", - Expressions.toString(Expressions.constant(ImmutableSet.of(1, 2)), true)); + Expressions.toString(Expressions.constant(ImmutableSet.of(1, 2)))); } @Test void testTenElementsSetLiteral() { @@ -1685,7 +1685,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(7)\n" + ".add(8)\n" + ".add(9).build()", - Expressions.toString(Expressions.constant(set), true)); + Expressions.toString(Expressions.constant(set))); } @Test void testTenElementsLinkedHashSetLiteral() { @@ -1703,7 +1703,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(7)\n" + ".add(8)\n" + ".add(9).build()", - Expressions.toString(Expressions.constant(set), true)); + Expressions.toString(Expressions.constant(set))); } @Test void testTenElementsSetStringLiteral() { @@ -1721,7 +1721,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + ".add(\"3\")\n" + ".add(\"2\")\n" + ".add(\"1\").build()", - Expressions.toString(Expressions.constant(set), true)); + Expressions.toString(Expressions.constant(set))); } @Test void testLargeMethod() { @@ -1770,7 +1770,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { ImmutableList.of(hugeMethod)); final String originalClass = Expressions.toString(classDecl); - final String classWithMethodSplitting = Expressions.toString(classDecl, true); + final String classWithMethodSplitting = Expressions.toString(classDecl, 4000); // Some local variables in hugeMethod have been extracted to be fields named local$ in // the enclosing class definition. diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java index c37f5ac12e7..8c529be17fd 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/InlinerTest.java @@ -117,7 +117,7 @@ public void prepareBuilder() { + " int t;\n" + " return (t = 1) != a ? t : c;\n" + "}\n", - Expressions.toString(builder.toBlock(), true)); + Expressions.toString(builder.toBlock())); } @Test void testAssignInConditionOptimizedOut() { @@ -153,7 +153,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { Expressions.parameter(int.class, "b"), Expressions.parameter(int.class, "c"))); builder.add(Expressions.return_(null, v)); - assertThat(Expressions.toString(builder.toBlock(), true), is(s)); + assertThat(Expressions.toString(builder.toBlock()), is(s)); } /** @@ -203,7 +203,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { + " int t = 2;\n" + " return (t = 1) != a ? t : c;\n" + "}\n", - Expressions.toString(builder.toBlock(), true)); + Expressions.toString(builder.toBlock())); } @Test void testMultiPassOptimization() { @@ -223,7 +223,7 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { assertEquals("{\n" + " return u + v;\n" + "}\n", - Expressions.toString(builder.toBlock(), true)); + Expressions.toString(builder.toBlock())); } @Test void testInlineInTryCatchStatement() { diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java index 11300adacbb..ac3a9c94105 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/Linq4jTest.java @@ -2134,29 +2134,29 @@ private static int count(Enumerator enumerator) { @Test void testApproxConstant() { ConstantExpression c; c = Expressions.constant(new BigDecimal("3.1"), float.class); - assertThat(Expressions.toString(c, true), equalTo("3.1F")); + assertThat(Expressions.toString(c), equalTo("3.1F")); c = Expressions.constant(new BigDecimal("-5.156"), float.class); - assertThat(Expressions.toString(c, true), equalTo("-5.156F")); + assertThat(Expressions.toString(c), equalTo("-5.156F")); c = Expressions.constant(new BigDecimal("-51.6"), Float.class); - assertThat(Expressions.toString(c, true), equalTo("Float.valueOf(-51.6F)")); + assertThat(Expressions.toString(c), equalTo("Float.valueOf(-51.6F)")); c = Expressions.constant(new BigDecimal(Float.MAX_VALUE), Float.class); - assertThat(Expressions.toString(c, true), + assertThat(Expressions.toString(c), equalTo("Float.valueOf(Float.intBitsToFloat(2139095039))")); c = Expressions.constant(new BigDecimal(Float.MIN_VALUE), Float.class); - assertThat(Expressions.toString(c, true), + assertThat(Expressions.toString(c), equalTo("Float.valueOf(Float.intBitsToFloat(1))")); c = Expressions.constant(new BigDecimal("3.1"), double.class); - assertThat(Expressions.toString(c, true), equalTo("3.1D")); + assertThat(Expressions.toString(c), equalTo("3.1D")); c = Expressions.constant(new BigDecimal("-5.156"), double.class); - assertThat(Expressions.toString(c, true), equalTo("-5.156D")); + assertThat(Expressions.toString(c), equalTo("-5.156D")); c = Expressions.constant(new BigDecimal("-51.6"), Double.class); - assertThat(Expressions.toString(c, true), equalTo("Double.valueOf(-51.6D)")); + assertThat(Expressions.toString(c), equalTo("Double.valueOf(-51.6D)")); c = Expressions.constant(new BigDecimal(Double.MAX_VALUE), Double.class); - assertThat(Expressions.toString(c, true), + assertThat(Expressions.toString(c), equalTo("Double.valueOf(Double.longBitsToDouble(9218868437227405311L))")); c = Expressions.constant(new BigDecimal(Double.MIN_VALUE), Double.class); - assertThat(Expressions.toString(c, true), + assertThat(Expressions.toString(c), equalTo("Double.valueOf(Double.longBitsToDouble(1L))")); } From 232eb8ef2423b28f599eaa998af187f70ffb8daf Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 27 Sep 2024 17:19:22 -0700 Subject: [PATCH 24/30] Revert accidental change --- .../java/org/apache/calcite/linq4j/tree/MethodDeclaration.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java index 97878f4c83d..444da129d5a 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/MethodDeclaration.java @@ -57,7 +57,7 @@ public MethodDeclaration(int modifier, String name, Type resultType, } @Override public void accept(ExpressionWriter writer) { - final String modifiers = Modifier.toString(modifier); + String modifiers = Modifier.toString(modifier); writer.append(modifiers); if (!modifiers.isEmpty()) { writer.append(' '); @@ -71,7 +71,6 @@ public MethodDeclaration(int modifier, String name, Type resultType, () -> (Iterator) parameters.stream().map(ParameterExpression::declString).iterator()) .append(' ') .append(body); - writer.newlineAndIndent(); } From 2887c0d2d147d9c7d224e0454c47d6af1322ec56 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 27 Sep 2024 17:19:53 -0700 Subject: [PATCH 25/30] Fix some bugs introduced during refactoring --- .../java/org/apache/calcite/linq4j/tree/Expressions.java | 2 +- .../calcite/adapter/enumerable/CodeGenerationBenchmark.java | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index 796a1828952..6309f6c1d55 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -59,7 +59,7 @@ private Expressions() {} */ public static String toString(List expressions, String sep, boolean generics) { - return toString(expressions, sep, generics); + return toString(expressions, sep, generics, 0); } /** diff --git a/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java b/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java index e7f1394053c..397a7a3afa6 100644 --- a/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java +++ b/ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java @@ -184,8 +184,11 @@ public void setup() { EnumerableRelImplementor relImplementor = new EnumerableRelImplementor(plan.getCluster().getRexBuilder(), new HashMap<>()); info.classExpr = relImplementor.implementRoot(plan, EnumerableRel.Prefer.ARRAY); + + // Set the method splitting threshold to the JVM limit of 4K. info.javaCode = - Expressions.toString(info.classExpr.memberDeclarations, "\n", false, true); + Expressions.toString(info.classExpr.memberDeclarations, + "\n", false, 4000); info.plan = plan; planInfos[i] = info; } From fdd0d18dbc6ab8a3c0b8e50c631ca581a3be7112 Mon Sep 17 00:00:00 2001 From: James Duong Date: Mon, 30 Sep 2024 04:21:33 -0700 Subject: [PATCH 26/30] Fix Javadoc linter errors --- .../org/apache/calcite/config/CalciteSystemProperty.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java index 093e1a05ee2..ac50f6592d3 100644 --- a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java +++ b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java @@ -420,13 +420,15 @@ public final class CalciteSystemProperty { public static final CalciteSystemProperty 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 length of code before automatic method splitting is enabled * - * A value of 0, the default, disables automatic method splitting. + *

A value of 0, the default, disables automatic method splitting. * *

Some queries can generate methods exceeding the JVM limit of 4000 characters per method. * Use this setting to automatically detect and split methods larger than the specified - * limit. */ + * limit. + */ public static final CalciteSystemProperty METHOD_SPLITTING_THRESHOLD = intProperty("calcite.linq.method_splitting_threshold", 0); From 80c8587c9e73857d9b7af3c233a8893bf2bd4176 Mon Sep 17 00:00:00 2001 From: James Duong Date: Thu, 3 Oct 2024 13:26:36 -0700 Subject: [PATCH 27/30] PR feedback --- .../enumerable/EnumerableInterpretable.java | 5 +-- .../calcite/config/CalciteSystemProperty.java | 19 +++++--- .../interpreter/JaninoRexCompiler.java | 2 +- .../apache/calcite/rex/RexExecutorImpl.java | 3 +- .../innodb/InnodbToEnumerableConverter.java | 2 +- .../calcite/linq4j/tree/AbstractNode.java | 2 +- .../calcite/linq4j/tree/ClassDeclaration.java | 2 +- .../calcite/linq4j/tree/ExpressionWriter.java | 44 ++++++++++++------- .../calcite/linq4j/tree/Expressions.java | 25 +++++++---- 9 files changed, 67 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java index 8a5bbad0b0f..2cc845c9202 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableInterpretable.java @@ -112,9 +112,8 @@ public static Bindable toBindable(Map parameters, final ClassDeclaration expr = relImplementor.implementRoot(rel, prefer); String s = - Expressions.toString(expr.memberDeclarations, - "\n", false, - CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value()); + Expressions.toString(expr.memberDeclarations, "\n", false, + CalciteSystemProperty.MAX_METHOD_LENGTH_IN_CHARS_BEFORE_SPLITTING.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); diff --git a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java index ac50f6592d3..288befb635b 100644 --- a/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java +++ b/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java @@ -16,6 +16,8 @@ */ package org.apache.calcite.config; +import org.apache.calcite.linq4j.tree.Expressions; + import com.google.common.collect.ImmutableSet; import org.checkerframework.checker.nullness.qual.Nullable; @@ -421,16 +423,21 @@ public final class CalciteSystemProperty { intProperty("calcite.function.cache.maxSize", 0, v -> v >= 0); /** - * The length of code before automatic method splitting is enabled + * The length of code in characters (inclusive) before automatic method splitting is enabled. + * If the length of a generated is equal or less than this value, automatic method splitting is + * disabled. * - *

A value of 0, the default, disables automatic method splitting. + *

A value of {@link Expressions#DISABLE_METHOD_SPLITTING}, the default, disables automatic + * method splitting. * - *

Some queries can generate methods exceeding the JVM limit of 4000 characters per method. - * Use this setting to automatically detect and split methods larger than the specified + *

Some queries can generate methods exceeding the default JIT limit of 4000 characters per + * method. Use this setting to automatically detect and split methods larger than the specified * limit. */ - public static final CalciteSystemProperty METHOD_SPLITTING_THRESHOLD = - intProperty("calcite.linq.method_splitting_threshold", 0); + public static final CalciteSystemProperty MAX_METHOD_LENGTH_IN_CHARS_BEFORE_SPLITTING = + intProperty("calcite.linq.method_splitting_character_limit", + Expressions.DISABLE_METHOD_SPLITTING, + v -> v == Expressions.DISABLE_METHOD_SPLITTING || v >= 0); private static CalciteSystemProperty booleanProperty(String key, boolean defaultValue) { diff --git a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java b/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java index b87b7674b8c..bfb436ebe7b 100644 --- a/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java +++ b/core/src/main/java/org/apache/calcite/interpreter/JaninoRexCompiler.java @@ -188,7 +188,7 @@ static Scalar.Producer baz(ParameterExpression context_, ImmutableList.of(Scalar.Producer.class), declarations); String s = Expressions.toString(declarations, "\n", false, - CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value()); + CalciteSystemProperty.MAX_METHOD_LENGTH_IN_CHARS_BEFORE_SPLITTING.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, s); } diff --git a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java index 86c6126688d..acd96c114cf 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java +++ b/core/src/main/java/org/apache/calcite/rex/RexExecutorImpl.java @@ -101,7 +101,8 @@ private static String compile(RexBuilder rexBuilder, List constExps, BuiltInMethod.FUNCTION1_APPLY.method.getName(), ImmutableList.of(root0_), blockBuilder.toBlock()); String code = - Expressions.toString(methodDecl, CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value()); + Expressions.toString(methodDecl, + CalciteSystemProperty.MAX_METHOD_LENGTH_IN_CHARS_BEFORE_SPLITTING.value()); if (CalciteSystemProperty.DEBUG.value()) { Util.debugCode(System.out, code); } diff --git a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java index 26184cd4c52..f6d57e79eef 100644 --- a/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java +++ b/innodb/src/main/java/org/apache/calcite/adapter/innodb/InnodbToEnumerableConverter.java @@ -152,7 +152,7 @@ static List innodbFieldNames(final RelDataType rowType) { if (CalciteSystemProperty.DEBUG.value()) { System.out.println( "Innodb: " + Expressions.toString(enumerable, - CalciteSystemProperty.METHOD_SPLITTING_THRESHOLD.value())); + CalciteSystemProperty.MAX_METHOD_LENGTH_IN_CHARS_BEFORE_SPLITTING.value())); } list.add(Expressions.return_(null, enumerable)); return implementor.result(physType, list.toBlock()); diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java index b4dbdc6dcc7..ed33789e2b6 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/AbstractNode.java @@ -52,7 +52,7 @@ public Type getType() { @Override public String toString() { // Use the JVM limit for method size (4K) as the method splitting threshold. - ExpressionWriter writer = new ExpressionWriter(true, 4000); + ExpressionWriter writer = new ExpressionWriter(); accept(writer, 0, 0); return writer.toString(); } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java index fd9f175ae5a..93998e4bc41 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ClassDeclaration.java @@ -75,7 +75,7 @@ public ClassDeclaration(int modifier, String name, @Nullable Type extended, writer.append( StringUtils.stripStart( JavaCodeSplitter.split(writerForClassWithoutSplitting.toString(), - writer.getMethodSplittingThreshold(), defaultMaxMembersGeneratedCode), + writer.getMaxMethodLengthInChars(), defaultMaxMembersGeneratedCode), " ")); } writer.newlineAndIndent(); diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java index 7c1bd5be410..fb4d649a307 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/ExpressionWriter.java @@ -33,24 +33,30 @@ class ExpressionWriter { private final Spacer spacer = new Spacer(0); private final StringBuilder buf = new StringBuilder(); private final boolean generics; - private final int methodSplittingThreshold; + + /** The maximum number of characters (inclusive) in a method before method splitting is + * enabled. A value of {@link Expressions#DISABLE_METHOD_SPLITTING} indicates that there is no + * limit. + */ + private final int maxMethodLengthInChars; private boolean indentPending; ExpressionWriter() { - this(true, 0); + this(true, Expressions.DISABLE_METHOD_SPLITTING); } - ExpressionWriter(boolean generics, int methodSplittingThreshold) { + /** + * Creates an ExpressionWriter that can optionally emit generics and split methods. + * + * @param generics Indicates if generic type arguments should be written. + * @param maxMethodLengthInChars The maximum number of characters (inclusive) in a generated + * method before the method is split up into smaller methods. + * A value of {@link Expressions#DISABLE_METHOD_SPLITTING} + * indicates that there is no limit to method size. + */ + ExpressionWriter(boolean generics, int maxMethodLengthInChars) { this.generics = generics; - this.methodSplittingThreshold = methodSplittingThreshold; - } - - public ExpressionWriter duplicateState() { - final ExpressionWriter writer = - new ExpressionWriter(this.generics, this.methodSplittingThreshold); - writer.indentPending = this.indentPending; - writer.spacer.add(this.spacer.get()); - return writer; + this.maxMethodLengthInChars = maxMethodLengthInChars; } public void write(Node expression) { @@ -84,11 +90,11 @@ public boolean requireParentheses(Expression expression, int lprec, } public boolean usesMethodSplitting() { - return methodSplittingThreshold != 0; + return maxMethodLengthInChars != Expressions.DISABLE_METHOD_SPLITTING; } - public int getMethodSplittingThreshold() { - return methodSplittingThreshold; + public int getMaxMethodLengthInChars() { + return maxMethodLengthInChars; } /** @@ -214,4 +220,12 @@ public void backUp() { indentPending = false; } } + + ExpressionWriter duplicateState() { + final ExpressionWriter writer = + new ExpressionWriter(this.generics, this.maxMethodLengthInChars); + writer.indentPending = this.indentPending; + writer.spacer.add(this.spacer.get()); + return writer; + } } diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java index 6309f6c1d55..6baf5b8930b 100644 --- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java +++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Expressions.java @@ -51,6 +51,11 @@ * Utility methods for expressions, including a lot of factory methods. */ public abstract class Expressions { + /** + * Used to disable method splitting when generating Java code with {@link #toString(Node, int)}. + */ + public static final int DISABLE_METHOD_SPLITTING = -1; + private Expressions() {} /** @@ -59,17 +64,19 @@ private Expressions() {} */ public static String toString(List expressions, String sep, boolean generics) { - return toString(expressions, sep, generics, 0); + return toString(expressions, sep, generics, Expressions.DISABLE_METHOD_SPLITTING); } /** * Converts a list of expressions to Java source code, optionally emitting * extra type information in generics. Optionally splits the generated method if - * the method is too large. + * the method exceeds the length specified in maxMethodLengthInChars. + * A maxMethodLengthInChars value of {@link #DISABLE_METHOD_SPLITTING} + * disables method splitting. */ public static String toString(List expressions, String sep, - boolean generics, int methodSplittingThreshold) { - final ExpressionWriter writer = new ExpressionWriter(generics, methodSplittingThreshold); + boolean generics, int maxMethodLengthInChars) { + final ExpressionWriter writer = new ExpressionWriter(generics, maxMethodLengthInChars); for (Node expression : expressions) { writer.write(expression); writer.append(sep); @@ -81,15 +88,17 @@ public static String toString(List expressions, String sep, * Converts an expression to Java source code. */ public static String toString(Node expression) { - return toString(expression, 0); + return toString(expression, Expressions.DISABLE_METHOD_SPLITTING); } /** * Converts an expression to Java source code. - * Optionally splits the generated method if the method is too large. + * Optionally splits the generated method if the method exceeds the length specified in + * maxMethodLengthInChars. A maxMethodLengthInChars value of {@link #DISABLE_METHOD_SPLITTING} + * disables method splitting. */ - public static String toString(Node expression, int methodSplittingTheshold) { - return toString(Collections.singletonList(expression), "", true, methodSplittingTheshold); + public static String toString(Node expression, int maxMethodLengthInChars) { + return toString(Collections.singletonList(expression), "", true, maxMethodLengthInChars); } /** From 4a258d825090ae03168d4098623d42766f08647b Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 4 Oct 2024 06:51:29 -0700 Subject: [PATCH 28/30] Clarify testLargeMethod --- .../calcite/linq4j/test/ExpressionTest.java | 46 +++++++++++-------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index 13427685863..14940aeb95a 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -43,7 +43,6 @@ import com.google.common.collect.Sets; import org.checkerframework.checker.nullness.qual.Nullable; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -1724,26 +1723,30 @@ public void checkBlockBuilder(boolean optimizing, String expected) { Expressions.toString(Expressions.constant(set))); } + /** Test case for [CALCITE-6465] + * Rework code generation to use Flink code splitter. */ @Test void testLargeMethod() { -// public class MyClass { -// public void hugeMethod() { -// if (true) { -// // Long variable name declaration 1; -// } else if (false) { -// // Long variable name declaration 2; -// } else if (false) { -// // Long variable name declaration 3; -// } else if (false) { -// // Long variable name declaration 4; -// } else -// // Long variable name declaration 4; -// } -// } + // The set of Expression objects used in this test will result in initial Java code + // of the form: + // public class MyClass { + // public void hugeMethod() { + // if (true) { + // // Long variable name declaration 1; + // } else if (false) { + // // Long variable name declaration 2; + // } else if (false) { + // // Long variable name declaration 3; + // } else if (false) { + // // Long variable name declaration 4; + // } else + // // Long variable name declaration 4; + // } + // } // Generate long variable names. final String longName = StringUtils.repeat('a', 5000); - DeclarationStatement longDecl = + final DeclarationStatement longDecl = Expressions.declare(0, longName, Expressions.constant(10)); final ConditionalStatement ifThenElse = Expressions.ifThenElse( @@ -1772,10 +1775,13 @@ public void checkBlockBuilder(boolean optimizing, String expected) { final String originalClass = Expressions.toString(classDecl); final String classWithMethodSplitting = Expressions.toString(classDecl, 4000); - // Some local variables in hugeMethod have been extracted to be fields named local$ in - // the enclosing class definition. - Assertions.assertFalse(originalClass.contains("local$")); - Assertions.assertTrue(classWithMethodSplitting.contains("local$")); + // Validate that the Flink method splitter ran. + // Test that some local variables in hugeMethod have been extracted to be fields + // named local$ in the enclosing class definition. + assertThat("The non-split method should not generate a field containing the prefix" + + "local$", !originalClass.contains("local$")); + assertThat("The split method should rewrite the method to create a field with a name" + + " prefixed local$", classWithMethodSplitting.contains("local$")); } /** An enum. */ From c28f9624be5ddbf63422c21bf92407322b119ac4 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 4 Oct 2024 06:51:42 -0700 Subject: [PATCH 29/30] Add comments about circular dependency concerns --- linq4j/build.gradle.kts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/linq4j/build.gradle.kts b/linq4j/build.gradle.kts index fe99d7abbd2..d678ca45aea 100644 --- a/linq4j/build.gradle.kts +++ b/linq4j/build.gradle.kts @@ -20,5 +20,8 @@ dependencies { implementation("com.google.guava:guava") implementation("org.apache.calcite.avatica:avatica-core") + // Note that only the flink-table-code-splitter module (and its dependencies) is imported + // from Flink. Other Flink modules have Calcite as a dependency and can introduce circular + // dependencies. implementation("org.apache.flink:flink-table-code-splitter") } From 4f95aa0bb084057a6b8344c5c62df044a16d0cb9 Mon Sep 17 00:00:00 2001 From: James Duong Date: Fri, 4 Oct 2024 11:11:53 -0700 Subject: [PATCH 30/30] Expand on method-splitting tests --- .../calcite/linq4j/test/ExpressionTest.java | 123 +++++++++++++++++- 1 file changed, 122 insertions(+), 1 deletion(-) diff --git a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java index 14940aeb95a..0ef7b00e03d 100644 --- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java +++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/ExpressionTest.java @@ -73,6 +73,7 @@ import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.hasToString; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; /** @@ -1725,7 +1726,7 @@ public void checkBlockBuilder(boolean optimizing, String expected) { /** Test case for [CALCITE-6465] * Rework code generation to use Flink code splitter. */ - @Test void testLargeMethod() { + @Test void testLargeMethodWithMethodSplitting() { // The set of Expression objects used in this test will result in initial Java code // of the form: // public class MyClass { @@ -1743,6 +1744,10 @@ public void checkBlockBuilder(boolean optimizing, String expected) { // } // } + // This test will use Flink's method splitting to extract long local variable names in the + // "hugeMethod" method to fields of MyClass. This will in turn reduce the length of the + // method (but can increase the length of the class itself). + // Generate long variable names. final String longName = StringUtils.repeat('a', 5000); @@ -1784,6 +1789,122 @@ public void checkBlockBuilder(boolean optimizing, String expected) { + " prefixed local$", classWithMethodSplitting.contains("local$")); } + /** Test case for [CALCITE-6465] + * Rework code generation to use Flink code splitter. */ + @Test void testMethodThatDoesNotNeedToBeSplit() { + // The set of Expression objects used in this test will result in initial Java code + // of the form: + // public class MyClass { + // public void testMethod() { + // if (true) { + // int dummyVar = 0; + // } else if (false) { + // int dummyVar = 0; + // } else if (false) { + // int dummyVar = 0; + // } else if (false) { + // int dummyVar = 0; + // } else + // int dummyVar = 0; + // } + // } + + // This test verifies that the Flink method splitter does not get invoked if the method + // is already short enough to fit within the size requested. + + // Generate long variable names. + final String localVariable = "dummyVar"; + + final DeclarationStatement localDecl = + Expressions.declare(0, localVariable, Expressions.constant(10)); + final ConditionalStatement ifThenElse = + Expressions.ifThenElse( + Expressions.constant(true), + localDecl, + Expressions.constant(false), + localDecl, + Expressions.constant(false), + localDecl, + localDecl); + + final MethodDeclaration hugeMethod = + Expressions.methodDecl(Modifier.PUBLIC, + Void.TYPE, + "testMethod", + Collections.emptyList(), + Blocks.toFunctionBlock(ifThenElse)); + + final ClassDeclaration classDecl = + new ClassDeclaration(Modifier.PUBLIC, + "MyClass", + null, + ImmutableList.of(), + ImmutableList.of(hugeMethod)); + + final String originalClass = Expressions.toString(classDecl); + final String classWithMethodSplitting = Expressions.toString(classDecl, 1000); + + // Validate that the results with and without method splitting are the same. + assertThat("The generated code with and without method splitting should be the same", + originalClass.equals(classWithMethodSplitting)); + } + + /** Test case for [CALCITE-6465] + * Rework code generation to use Flink code splitter. */ + @Test void testMethodThatCannotFitWithinSizeLimit() { + // The set of Expression objects used in this test will result in initial Java code + // of the form: + // public class MyClass { + // public void testMethod() { + // if (true) { + // int dummyVar = 0; + // } else if (false) { + // int dummyVar = 0; + // } else if (false) { + // int dummyVar = 0; + // } else if (false) { + // int dummyVar = 0; + // } else + // int dummyVar = 0; + // } + // } + + // This test will attempt to use Flink's method splitting but fail because the method cannot + // be reduced to the number of characters requested. It should fail gracefully in this + // scenario. + + // Generate long variable names. + final String localVariable = "dummyVar"; + + final DeclarationStatement localDecl = + Expressions.declare(0, localVariable, Expressions.constant(10)); + final ConditionalStatement ifThenElse = + Expressions.ifThenElse( + Expressions.constant(true), + localDecl, + Expressions.constant(false), + localDecl, + Expressions.constant(false), + localDecl, + localDecl); + + final MethodDeclaration hugeMethod = + Expressions.methodDecl(Modifier.PUBLIC, + Void.TYPE, + "testMethod", + Collections.emptyList(), + Blocks.toFunctionBlock(ifThenElse)); + + final ClassDeclaration classDecl = + new ClassDeclaration(Modifier.PUBLIC, + "MyClass", + null, + ImmutableList.of(), + ImmutableList.of(hugeMethod)); + + assertDoesNotThrow(() -> Expressions.toString(classDecl, 5)); + } + /** An enum. */ enum MyEnum { X,