This is an automated email from the ASF dual-hosted git repository.
mihaibudiu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/main by this push:
new 197d569217 [CALCITE-7729] Linq4j BlockBuilder.optimize can optimize
away expressions that throw
197d569217 is described below
commit 197d569217eda062142d2e9164a8e00a1db6ab61
Author: Mihai Budiu <[email protected]>
AuthorDate: Wed Aug 19 21:13:06 2026 -0700
[CALCITE-7729] Linq4j BlockBuilder.optimize can optimize away expressions
that throw
Signed-off-by: Mihai Budiu <[email protected]>
---
.../calcite/adapter/enumerable/RexImpTable.java | 28 ++++-
.../java/org/apache/calcite/test/JdbcTest.java | 3 +-
.../apache/calcite/test/ReflectiveSchemaTest.java | 4 +-
.../apache/calcite/linq4j/tree/BlockBuilder.java | 108 ++++++++++++++++---
.../calcite/linq4j/test/BlockBuilderTest.java | 116 +++++++++++++++++++++
.../apache/calcite/linq4j/test/ExpressionTest.java | 4 +-
.../apache/calcite/linq4j/test/OptimizerTest.java | 6 +-
7 files changed, 247 insertions(+), 22 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
index ae40e33c13..6ef0182b30 100644
--- a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
+++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
@@ -2498,11 +2498,33 @@ protected FirstLastValueImplementor(SeekType seekType) {
return implementResultIgnoreNulls(info, winResult);
}
- return Expressions.condition(winResult.hasRows(),
+ // Generates:
+ // T first_last_value;
+ // if (<hasRows>) {
+ // <statements that read the row>
+ // first_last_value = <value of the argument in that row>;
+ // } else {
+ // first_last_value = <default value>;
+ // }
+ //
+ // Reading a row is only valid when the frame has rows: on an empty frame
+ // computeIndex returns -1.
+ final ParameterExpression res =
+ Expressions.parameter(0, info.returnType(),
+ result.currentBlock().newName("first_last_value"));
+ final BlockBuilder thenBlock = result.nestBlock();
+ final Expression value =
winResult.rowTranslator(
winResult.computeIndex(Expressions.constant(0), seekType))
- .translate(winResult.rexArguments().get(0), info.returnType()),
- getDefaultValue(info.returnType()));
+ .translate(winResult.rexArguments().get(0), info.returnType());
+ thenBlock.add(Expressions.statement(Expressions.assign(res, value)));
+ result.exitBlock();
+ result.currentBlock().add(Expressions.declare(0, res, null));
+ result.currentBlock().add(
+ Expressions.ifThenElse(winResult.hasRows(), thenBlock.toBlock(),
+ Expressions.statement(
+ Expressions.assign(res,
getDefaultValue(info.returnType())))));
+ return res;
}
/**
diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java
b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
index d235fa327c..6dc4a48dc9 100644
--- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java
+++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
@@ -2772,6 +2772,7 @@ private void checkNullableTimestamp(CalciteAssert.Config
config) {
+ " final org.apache.calcite.test.schemata.hr.Employee
current"
+ " = (org.apache.calcite.test.schemata.hr.Employee)
inputEnumerator.current();\n"
+ " final String input_value = current.name;\n"
+ + " final int input_value0 = current.deptno;\n"
+ " Integer case_when_value;\n"
+ " if
($L4J$C$org_apache_calcite_runtime_SqlFunctions_ne_) {\n"
+ " case_when_value = $L4J$C$Integer_valueOf_1_;\n"
@@ -2780,7 +2781,7 @@ private void checkNullableTimestamp(CalciteAssert.Config
config) {
+ " }\n"
+ " final Integer binary_call_value0 = "
+ "case_when_value == null ? null : "
- + "Integer.valueOf(current.deptno + case_when_value.intValue());\n"
+ + "Integer.valueOf(input_value0 + case_when_value.intValue());\n"
+ " return input_value == null || binary_call_value0 ==
null"
+ " ? null"
+ " : org.apache.calcite.runtime.SqlFunctions.substring(input_value, "
diff --git
a/core/src/test/java/org/apache/calcite/test/ReflectiveSchemaTest.java
b/core/src/test/java/org/apache/calcite/test/ReflectiveSchemaTest.java
index e4979b95f3..cc0205c987 100644
--- a/core/src/test/java/org/apache/calcite/test/ReflectiveSchemaTest.java
+++ b/core/src/test/java/org/apache/calcite/test/ReflectiveSchemaTest.java
@@ -764,7 +764,9 @@ private void check(ResultSetMetaData metaData, String
columnName,
.planContains(
"final Long input_value = current.wrapperLong;")
.planContains(
- "return input_value == null ? null :
Long.valueOf(input_value.longValue() / current.primitiveLong);")
+ "final long input_value0 = current.primitiveLong;")
+ .planContains(
+ "return input_value == null ? null :
Long.valueOf(input_value.longValue() / input_value0);")
.returns("C=null\n");
}
diff --git
a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java
b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java
index 28aab938a1..5d811ea68c 100644
--- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java
+++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/BlockBuilder.java
@@ -360,7 +360,7 @@ private boolean optimize(Shuttle optimizer, boolean
performInline) {
for (Statement statement : statements) {
if (statement instanceof DeclarationStatement && performInline) {
DeclarationStatement decl = (DeclarationStatement) statement;
- useCounter.map.put(decl.parameter, new Slot());
+ useCounter.map.put(decl.parameter, new ParameterUse());
}
// We are added only counters up to current statement.
// It is fine to count usages as the latter declarations cannot be used
@@ -378,7 +378,7 @@ private boolean optimize(Shuttle optimizer, boolean
performInline) {
for (Statement oldStatement : oldStatements) {
if (oldStatement instanceof DeclarationStatement) {
DeclarationStatement statement = (DeclarationStatement) oldStatement;
- final Slot slot = useCounter.map.get(statement.parameter);
+ final ParameterUse slot = useCounter.map.get(statement.parameter);
int count = slot == null ? Integer.MAX_VALUE - 10 : slot.count;
if (count > 1 && isSimpleExpression(statement.initializer)) {
// Inline simple final constants
@@ -410,13 +410,15 @@ private boolean optimize(Shuttle optimizer, boolean
performInline) {
// anonymous classes.
count = Integer.MAX_VALUE;
}
- if (count == 0
- && statement.initializer != null
- && Expressions.mayThrow(statement.initializer)) {
- // Never read, but computing the value may raise a runtime error that
- // the program is expected to raise. Keep the declaration, and treat
- // it like any other statement that cannot be inlined.
- count = 100;
+ if (statement.initializer != null
+ && (count == 0 || slot != null && slot.conditional)) {
+ // The value is either never read, or read only conditionally.
+ // If it may raise a runtime error, keep the declaration where it is.
+ final Expression initializer =
+ subMap.isEmpty() ? statement.initializer :
statement.initializer.accept(visitor);
+ if (Expressions.mayThrow(initializer)) {
+ count = 100;
+ }
}
Expression normalized = normalizeDeclaration(statement);
expressionForReuse.remove(normalized);
@@ -603,15 +605,92 @@ private static class InlineVariableVisitor extends
SubstituteVariableVisitor {
/** Use counter. */
private static class UseCounter extends VisitorImpl<Void> {
- private final IdentityHashMap<ParameterExpression, Slot> map = new
IdentityHashMap<>();
+ /** Map each parameter to information about how it is used. */
+ private final IdentityHashMap<ParameterExpression, ParameterUse> map = new
IdentityHashMap<>();
+ /** Whether the node being visited is evaluated only if some other
+ * expression permits it, as "a" in the expression "c ? a : b". */
+ private boolean inConditional = false;
+
+ /** Visits a node that is evaluated only under a condition. */
+ private void acceptConditionally(Node node) {
+ final boolean prev = inConditional;
+ inConditional = true;
+ node.accept(this);
+ inConditional = prev;
+ }
+
+ @Override public Void visit(TernaryExpression ternary) {
+ if (ternary.getNodeType() != ExpressionType.Conditional) {
+ return super.visit(ternary);
+ }
+ ternary.expression0.accept(this);
+ acceptConditionally(ternary.expression1);
+ acceptConditionally(ternary.expression2);
+ return null;
+ }
+
+ @Override public Void visit(BinaryExpression binary) {
+ switch (binary.getNodeType()) {
+ case AndAlso:
+ case OrElse:
+ // The right operand is evaluated only if the left one has not already
decided the result
+ binary.expression0.accept(this);
+ acceptConditionally(binary.expression1);
+ return null;
+ default:
+ return super.visit(binary);
+ }
+ }
+
+ @Override public Void visit(WhileStatement whileStatement) {
+ // The body may never run
+ whileStatement.condition.accept(this);
+ acceptConditionally(whileStatement.body);
+ return null;
+ }
+
+ @Override public Void visit(ForStatement forStatement) {
+ // The body and the "post" expression may not run
+ Expressions.acceptNodes(forStatement.declarations, this);
+ if (forStatement.condition != null) {
+ forStatement.condition.accept(this);
+ }
+ if (forStatement.post != null) {
+ acceptConditionally(forStatement.post);
+ }
+ acceptConditionally(forStatement.body);
+ return null;
+ }
+
+ @Override public Void visit(ForEachStatement forEachStatement) {
+ // The body may not run
+ forEachStatement.parameter.accept(this);
+ forEachStatement.iterable.accept(this);
+ acceptConditionally(forEachStatement.body);
+ return null;
+ }
+
+ @Override public Void visit(ConditionalStatement conditionalStatement) {
+ // In "if (c0) s0 else if (c1) s1 ... else s", only "c0" is unconditional
+ final List<Node> list = conditionalStatement.expressionList;
+ for (int i = 0; i < list.size(); i++) {
+ if (i == 0) {
+ list.get(i).accept(this);
+ } else {
+ acceptConditionally(list.get(i));
+ }
+ }
+ return null;
+ }
@Override public Void visit(ParameterExpression parameter) {
- final Slot slot = map.get(parameter);
+ final ParameterUse slot = map.get(parameter);
if (slot != null) {
// Count use of parameter, if it's registered. It's OK if
// parameter is not registered. It might be beyond the control
// of this block.
slot.count++;
+ slot.conditional |= inConditional;
}
return super.visit(parameter);
}
@@ -626,9 +705,12 @@ private static class UseCounter extends VisitorImpl<Void> {
}
/**
- * Holds the number of times a declaration was used.
+ * Holds information about the uses of one ParameterExpression within an
expression.
*/
- private static class Slot {
+ private static class ParameterUse {
+ /** How many times the declaration is read. */
private int count;
+ /** Whether at least one read is evaluated only under a condition. */
+ private boolean conditional;
}
}
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 6950beae82..5cc772c0b5 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
@@ -83,6 +83,122 @@ public void prepareBuilder() {
+ "}\n"));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7729";>[CALCITE-7729]
+ * Linq4j BlockBuilder.optimize can optimize away expressions that
+ * throw</a>. */
+ @Test void testDeclarationUsedOnlyInBranchThatFoldsAway() {
+ final ParameterExpression i = Expressions.parameter(int.class, "i");
+ final Expression x = b.append("x", Expressions.divide(ONE, i));
+ b.add(
+ Expressions.return_(null,
+ Expressions.condition(Expressions.constant(true), TWO, x)));
+ assertThat(b.toBlock(),
+ hasToString("{\n"
+ + " final int x = 1 / i;\n"
+ + " return 2;\n"
+ + "}\n"));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7729";>[CALCITE-7729]
+ * Linq4j BlockBuilder.optimize can optimize away expressions that
+ * throw</a>. */
+ @Test void testDeclarationUsedInBranchThatSurvives() {
+ final ParameterExpression i = Expressions.parameter(int.class, "i");
+ final ParameterExpression c = Expressions.parameter(boolean.class, "c");
+ final Expression x = b.append("x", Expressions.divide(ONE, i));
+ b.add(Expressions.return_(null, Expressions.condition(c, TWO, x)));
+ assertThat(b.toBlock(),
+ hasToString("{\n"
+ + " final int x = 1 / i;\n"
+ + " return c ? 2 : x;\n"
+ + "}\n"));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7729";>[CALCITE-7729]
+ * Linq4j BlockBuilder.optimize can optimize away expressions that
+ * throw</a>.
+ *
+ * <p>Nested conditionals: "x" is read in a branch of an inner conditional,
+ * which is itself in a branch. */
+ @Test void testNestedConditionalBranch() {
+ final ParameterExpression i = Expressions.parameter(int.class, "i");
+ final ParameterExpression c = Expressions.parameter(boolean.class, "c");
+ final ParameterExpression d = Expressions.parameter(boolean.class, "d");
+ final Expression x = b.append("x", Expressions.divide(ONE, i));
+ b.add(
+ Expressions.return_(null,
+ Expressions.condition(c,
+ Expressions.condition(d, TWO, x), ONE)));
+ assertThat(b.toBlock(),
+ hasToString("{\n"
+ + " final int x = 1 / i;\n"
+ + " return c ? (d ? 2 : x) : 1;\n"
+ + "}\n"));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7729";>[CALCITE-7729]
+ * Linq4j BlockBuilder.optimize can optimize away expressions that
+ * throw</a>.
+ *
+ * <p>Nested conditionals: "x" is read in the condition of an inner
+ * conditional, so it is evaluated only if the outer condition holds. */
+ @Test void testNestedConditionalCondition() {
+ final ParameterExpression i = Expressions.parameter(int.class, "i");
+ final ParameterExpression c = Expressions.parameter(boolean.class, "c");
+ final Expression x = b.append("x", Expressions.divide(ONE, i));
+ b.add(
+ Expressions.return_(null,
+ Expressions.condition(c,
+ Expressions.condition(
+ Expressions.greaterThan(x, ONE), TWO, ONE),
+ ONE)));
+ assertThat(b.toBlock(),
+ hasToString("{\n"
+ + " final int x = 1 / i;\n"
+ + " return c ? (x > 1 ? 2 : 1) : 1;\n"
+ + "}\n"));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7729";>[CALCITE-7729]
+ * Linq4j BlockBuilder.optimize can optimize away expressions that
+ * throw</a>. */
+ @Test void testDeclarationUsedOnlyInWhileBody() {
+ final ParameterExpression i = Expressions.parameter(int.class, "i");
+ final ParameterExpression c = Expressions.parameter(boolean.class, "c");
+ final ParameterExpression y = Expressions.parameter(int.class, "y");
+ final Expression x = b.append("x", Expressions.divide(ONE, i));
+ b.add(Expressions.declare(0, y, ONE));
+ b.add(
+ Expressions.while_(c,
+ Expressions.statement(Expressions.assign(y, x))));
+ assertThat(b.toBlock(),
+ hasToString("{\n"
+ + " final int x = 1 / i;\n"
+ + " int y = 1;\n"
+ + " while (c) {\n"
+ + " y = x;\n"
+ + " }\n"
+ + "}\n"));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7729";>[CALCITE-7729]
+ * Linq4j BlockBuilder.optimize can optimize away expressions that
+ * throw</a>. */
+ @Test void testPureDeclarationIsInlinedIntoBranch() {
+ // Test with expression that does not throw
+ final ParameterExpression i = Expressions.parameter(int.class, "i");
+ final ParameterExpression c = Expressions.parameter(boolean.class, "c");
+ final Expression x = b.append("x", Expressions.add(ONE, i));
+ b.add(Expressions.return_(null, Expressions.condition(c, TWO, x)));
+ assertThat(b.toBlock(), hasToString("{\n return c ? 2 : 1 + i;\n}\n"));
+ }
+
/** Test case for
* <a
href="https://issues.apache.org/jira/browse/CALCITE-7728";>[CALCITE-7728]
* Linq4j can simplify expressions without regards for 'safety'</a>.
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 c75c2a2139..3482e2b867 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
@@ -1496,8 +1496,8 @@ public void checkBlockBuilder(boolean optimizing, String
expected) {
assertThat(Expressions.toString(builder.toBlock()),
is("{\n"
+ " final Short v = (Short) ((Object[]) p)[4];\n"
- + " return (Number) v == null ? null : ("
- + "(Number) v).intValue() == 1997;\n"
+ + " final int v5 = ((Number) v).intValue();\n"
+ + " return (Number) v == null ? null : v5 == 1997;\n"
+ "}\n"));
}
diff --git
a/linq4j/src/test/java/org/apache/calcite/linq4j/test/OptimizerTest.java
b/linq4j/src/test/java/org/apache/calcite/linq4j/test/OptimizerTest.java
index 8fb0191ec2..ce1e429c6a 100644
--- a/linq4j/src/test/java/org/apache/calcite/linq4j/test/OptimizerTest.java
+++ b/linq4j/src/test/java/org/apache/calcite/linq4j/test/OptimizerTest.java
@@ -868,8 +868,9 @@ class OptimizerTest {
x_)))),
equalTo("{\n"
+ " long x = 0L;\n"
+ + " final long y = System.currentTimeMillis();\n"
+ " if (System.nanoTime() > 0L) {\n"
- + " x = System.currentTimeMillis();\n"
+ + " x = y;\n"
+ " }\n"
+ " System.out.println(x);\n"
+ "}\n"));
@@ -897,8 +898,9 @@ class OptimizerTest {
Expressions.statement(Expressions.assign(x_, y_))))),
equalTo("{\n"
+ " long x = 0L;\n"
+ + " final long y = System.currentTimeMillis();\n"
+ " if (System.currentTimeMillis() > 0L) {\n"
- + " x = System.currentTimeMillis();\n"
+ + " x = y;\n"
+ " }\n"
+ "}\n"));
}
