From 6af2d2e38c59a2feee9bad9ff7ffbb7a657728ba Mon Sep 17 00:00:00 2001 From: Frank Chen Date: Tue, 22 Sep 2026 18:23:21 +0800 Subject: [PATCH 1/2] [CALCITE-7809] Avoid copying unchanged SQL and Rex operands Allocate operand copies only when a shuttle changes an operand, while preserving immutable Rex lists and SqlShuttle's alwaysCopy behavior. Add focused tests for unchanged inputs, changed boundaries, mutable and partial-view lists, and forced SQL call copies. --- .../org/apache/calcite/rex/RexShuttle.java | 23 +++- .../apache/calcite/sql/util/SqlShuttle.java | 33 +++-- .../calcite/sql/util/SqlShuttleTest.java | 111 ++++++++++++++++ .../apache/calcite/test/RexShuttleTest.java | 120 ++++++++++++++++++ 4 files changed, 270 insertions(+), 17 deletions(-) create mode 100644 core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java diff --git a/core/src/main/java/org/apache/calcite/rex/RexShuttle.java b/core/src/main/java/org/apache/calcite/rex/RexShuttle.java index d99ae12630fa..15c312da3e7b 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexShuttle.java +++ b/core/src/main/java/org/apache/calcite/rex/RexShuttle.java @@ -159,19 +159,30 @@ protected RexNode[] visitArray(RexNode[] exprs, boolean @Nullable [] update) { * @param exprs List of expressions * @param update If not null, sets this to true if any of the expressions * was modified - * @return Array of visited expressions + * @return Immutable list of visited expressions; may be {@code exprs} + * itself if no expression was modified */ protected List visitList( List exprs, boolean @Nullable [] update) { - ImmutableList.Builder clonedOperands = ImmutableList.builder(); + ImmutableList.@Nullable Builder newOperands = null; + int i = 0; for (RexNode operand : exprs) { RexNode clonedOperand = operand.accept(this); - if ((clonedOperand != operand) && (update != null)) { - update[0] = true; + if (clonedOperand != operand && newOperands == null) { + newOperands = ImmutableList.builder(); + newOperands.addAll(exprs.subList(0, i)); + if (update != null) { + update[0] = true; + } } - clonedOperands.add(clonedOperand); + if (newOperands != null) { + newOperands.add(clonedOperand); + } + i++; } - return clonedOperands.build(); + return newOperands != null + ? newOperands.build() + : ImmutableList.copyOf(exprs); } /** diff --git a/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java b/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java index 6ca2000642e4..339f7e6449e5 100644 --- a/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java +++ b/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java @@ -96,31 +96,32 @@ public class SqlShuttle extends SqlBasicVisitor<@Nullable SqlNode> { /** * Implementation of * {@link org.apache.calcite.sql.util.SqlBasicVisitor.ArgHandler} - * that deep-copies {@link SqlCall}s and their operands. + * that copies a {@link SqlCall} when an operand changes or when + * {@code alwaysCopy} is true. */ protected class CallCopyingArgHandler implements ArgHandler<@Nullable SqlNode> { boolean update; - final @Nullable SqlNode[] clonedOperands; + @Nullable SqlNode @Nullable [] clonedOperands; private final SqlCall call; private final boolean alwaysCopy; public CallCopyingArgHandler(SqlCall call, boolean alwaysCopy) { this.call = call; this.update = false; - final List<@Nullable SqlNode> operands = (List<@Nullable SqlNode>) call.getOperandList(); - this.clonedOperands = operands.toArray(new SqlNode[0]); this.alwaysCopy = alwaysCopy; + this.clonedOperands = null; } @Override public SqlNode result() { - if (update || alwaysCopy) { - return call.getOperator().createCall( - call.getFunctionQuantifier(), - call.getParserPosition(), - clonedOperands); - } else { + if (!update && !alwaysCopy) { return call; } + final @Nullable SqlNode[] operands = + clonedOperands != null ? clonedOperands : copyOperands(); + return call.getOperator().createCall( + call.getFunctionQuantifier(), + call.getParserPosition(), + operands); } @Override public @Nullable SqlNode visitChild( @@ -134,9 +135,19 @@ public CallCopyingArgHandler(SqlCall call, boolean alwaysCopy) { SqlNode newOperand = operand.accept(SqlShuttle.this); if (newOperand != operand) { update = true; + if (clonedOperands == null) { + clonedOperands = copyOperands(); + } + } + if (clonedOperands != null) { + clonedOperands[i] = newOperand; } - clonedOperands[i] = newOperand; return newOperand; } + + private @Nullable SqlNode[] copyOperands() { + final List<@Nullable SqlNode> operands = call.getOperandList(); + return operands.toArray(SqlNode.EMPTY_ARRAY); + } } } diff --git a/core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java b/core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java new file mode 100644 index 000000000000..18ed5d372416 --- /dev/null +++ b/core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java @@ -0,0 +1,111 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.calcite.sql.util; + +import org.apache.calcite.sql.SqlCall; +import org.apache.calcite.sql.SqlLiteral; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.parser.SqlParserPos; + +import org.junit.jupiter.api.Test; + +import static java.util.Objects.requireNonNull; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; + +/** Unit tests for {@link SqlShuttle}. */ +class SqlShuttleTest { + + /** Test case for + * [CALCITE-7809] + * Reduce temporary object allocation in expression shuttles. */ + @Test void testUnchangedCallIsReused() { + final SqlLiteral[] operands = createLiterals(); + final SqlCall call = createCall(operands); + final SqlLiteral absent = SqlLiteral.createExactNumeric("4", SqlParserPos.ZERO); + + final SqlNode result = + requireNonNull(call.accept(new ReplacingSqlShuttle(absent, absent))); + + assertSame(call, result); + } + + @Test void testChangedOperandCopiesCall() { + final SqlLiteral[] operands = createLiterals(); + final SqlCall call = createCall(operands); + final SqlLiteral replacement = + SqlLiteral.createExactNumeric("4", SqlParserPos.ZERO); + + final SqlCall result = + (SqlCall) requireNonNull(call.accept( + new ReplacingSqlShuttle(operands[1], replacement))); + + assertNotSame(call, result); + assertEquals(operands.length, result.operandCount()); + assertSame(operands[0], result.operand(0)); + assertSame(replacement, result.operand(1)); + assertSame(operands[2], result.operand(2)); + } + + @Test void testAlwaysCopyCopiesUnchangedCall() { + final SqlLiteral[] operands = createLiterals(); + final SqlCall call = createCall(operands); + final SqlShuttle shuttle = new SqlShuttle(); + final SqlShuttle.CallCopyingArgHandler argHandler = + shuttle.new CallCopyingArgHandler(call, true); + + call.getOperator().acceptCall(shuttle, call, false, argHandler); + final SqlCall result = (SqlCall) argHandler.result(); + + assertNotSame(call, result); + assertEquals(operands.length, result.operandCount()); + assertSame(operands[0], result.operand(0)); + assertSame(operands[1], result.operand(1)); + assertSame(operands[2], result.operand(2)); + } + + private static SqlLiteral[] createLiterals() { + return new SqlLiteral[] { + SqlLiteral.createExactNumeric("1", SqlParserPos.ZERO), + SqlLiteral.createExactNumeric("2", SqlParserPos.ZERO), + SqlLiteral.createExactNumeric("3", SqlParserPos.ZERO) + }; + } + + private static SqlCall createCall(SqlNode... operands) { + return SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR.createCall( + SqlParserPos.ZERO, operands); + } + + /** Shuttle that replaces one target node. */ + private static class ReplacingSqlShuttle extends SqlShuttle { + private final SqlNode target; + private final SqlNode replacement; + + ReplacingSqlShuttle(SqlNode target, SqlNode replacement) { + this.target = target; + this.replacement = replacement; + } + + @Override public SqlNode visit(SqlLiteral literal) { + return literal == target ? replacement : literal; + } + } +} diff --git a/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java b/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java index aed5304a157c..fe88a6db873e 100644 --- a/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java +++ b/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java @@ -16,6 +16,8 @@ */ package org.apache.calcite.test; +import com.google.common.collect.ImmutableList; + import org.apache.calcite.plan.hep.HepPlanner; import org.apache.calcite.plan.hep.HepProgram; import org.apache.calcite.plan.hep.HepProgramBuilder; @@ -33,15 +35,133 @@ import org.apache.calcite.tools.RelBuilder; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.util.ArrayList; +import java.util.List; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; /** * Unit tests for {@link RexShuttle}. */ class RexShuttleTest { + /** Test case for + * [CALCITE-7809] + * Reduce temporary object allocation in expression shuttles. */ + @Test void testVisitListReusesUnchangedImmutableList() { + final RelDataType type = createIntegerType(); + final ImmutableList operands = createInputRefs(type, 3); + final RexNode absent = new RexInputRef(3, type); + final boolean[] update = {false}; + + final List result = + new ListVisitingShuttle(absent, absent) + .visitListForTest(operands, update); + + assertSame(operands, result); + assertFalse(update[0]); + } + + @ParameterizedTest + @ValueSource(ints = {0, 1, 2}) + void testVisitListCopiesOnFirstChange(int changedIndex) { + final RelDataType type = createIntegerType(); + final ImmutableList operands = createInputRefs(type, 3); + final RexNode replacement = new RexInputRef(3, type); + final boolean[] update = {false}; + + final List result = + new ListVisitingShuttle(operands.get(changedIndex), replacement) + .visitListForTest(operands, update); + + assertNotSame(operands, result); + assertEquals(operands.size(), result.size()); + assertTrue(update[0]); + assertSame(replacement, result.get(changedIndex)); + for (int i = 0; i < operands.size(); i++) { + if (i != changedIndex) { + assertSame(operands.get(i), result.get(i)); + } + } + } + + @Test void testVisitListCopiesUnchangedMutableInput() { + final RelDataType type = createIntegerType(); + final List operands = new ArrayList<>(createInputRefs(type, 3)); + final RexNode absent = new RexInputRef(3, type); + final boolean[] update = {false}; + + final List result = + new ListVisitingShuttle(absent, absent) + .visitListForTest(operands, update); + + assertNotSame(operands, result); + assertEquals(operands, result); + assertInstanceOf(ImmutableList.class, result); + assertFalse(update[0]); + } + + @Test void testVisitListCopiesImmutableListPartialView() { + final RelDataType type = createIntegerType(); + final ImmutableList backingList = createInputRefs(type, 5); + final List operands = backingList.subList(1, 4); + final RexNode absent = new RexInputRef(5, type); + final boolean[] update = {false}; + + final List result = + new ListVisitingShuttle(absent, absent) + .visitListForTest(operands, update); + + assertNotSame(operands, result); + assertEquals(operands, result); + assertInstanceOf(ImmutableList.class, result); + assertFalse(update[0]); + } + + private static RelDataType createIntegerType() { + return RelBuilder.create(RelBuilderTest.config().build()) + .getTypeFactory().createSqlType(SqlTypeName.INTEGER); + } + + private static ImmutableList createInputRefs( + RelDataType type, int count) { + final ImmutableList.Builder builder = ImmutableList.builder(); + for (int i = 0; i < count; i++) { + builder.add(new RexInputRef(i, type)); + } + return builder.build(); + } + + /** Shuttle that exposes {@link #visitList} for testing. */ + private static class ListVisitingShuttle extends RexShuttle { + private final RexNode target; + private final RexNode replacement; + + ListVisitingShuttle(RexNode target, RexNode replacement) { + this.target = target; + this.replacement = replacement; + } + + @Override public RexNode visitInputRef(RexInputRef inputRef) { + return inputRef == target ? replacement : inputRef; + } + + List visitListForTest( + List exprs, boolean[] update) { + return visitList(exprs, update); + } + } + /** Test case for * [CALCITE-3165] * Project#accept(RexShuttle shuttle) does not update rowType. */ From ef3fe85ede1a575d1ed1dc02e7ef2acb3656fe8f Mon Sep 17 00:00:00 2001 From: Frank Chen Date: Sun, 27 Sep 2026 19:11:40 +0800 Subject: [PATCH 2/2] [CALCITE-7809] Add benchmarks for shuttle copy-on-write paths --- .../apache/calcite/sql/util/SqlShuttle.java | 5 +- .../calcite/sql/util/SqlShuttleTest.java | 7 +- .../apache/calcite/test/RexShuttleTest.java | 4 +- .../RelNodeConversionBenchmark.java | 100 +++++++++++++++++- 4 files changed, 108 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java b/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java index 339f7e6449e5..5dc5b140db45 100644 --- a/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java +++ b/core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java @@ -146,8 +146,9 @@ public CallCopyingArgHandler(SqlCall call, boolean alwaysCopy) { } private @Nullable SqlNode[] copyOperands() { - final List<@Nullable SqlNode> operands = call.getOperandList(); - return operands.toArray(SqlNode.EMPTY_ARRAY); + final List<@Nullable SqlNode> operands = + (List<@Nullable SqlNode>) call.getOperandList(); + return operands.toArray(new SqlNode[0]); } } } diff --git a/core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java b/core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java index 18ed5d372416..e32785c868b1 100644 --- a/core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java +++ b/core/src/test/java/org/apache/calcite/sql/util/SqlShuttleTest.java @@ -24,12 +24,12 @@ import org.junit.jupiter.api.Test; -import static java.util.Objects.requireNonNull; - import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; +import static java.util.Objects.requireNonNull; + /** Unit tests for {@link SqlShuttle}. */ class SqlShuttleTest { @@ -54,7 +54,8 @@ class SqlShuttleTest { SqlLiteral.createExactNumeric("4", SqlParserPos.ZERO); final SqlCall result = - (SqlCall) requireNonNull(call.accept( + (SqlCall) requireNonNull( + call.accept( new ReplacingSqlShuttle(operands[1], replacement))); assertNotSame(call, result); diff --git a/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java b/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java index fe88a6db873e..fceb8ea85974 100644 --- a/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java +++ b/core/src/test/java/org/apache/calcite/test/RexShuttleTest.java @@ -16,8 +16,6 @@ */ package org.apache.calcite.test; -import com.google.common.collect.ImmutableList; - import org.apache.calcite.plan.hep.HepPlanner; import org.apache.calcite.plan.hep.HepProgram; import org.apache.calcite.plan.hep.HepProgramBuilder; @@ -34,6 +32,8 @@ import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.tools.RelBuilder; +import com.google.common.collect.ImmutableList; + import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; diff --git a/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/RelNodeConversionBenchmark.java b/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/RelNodeConversionBenchmark.java index cbabec73c1fb..e8d257a4af05 100644 --- a/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/RelNodeConversionBenchmark.java +++ b/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/RelNodeConversionBenchmark.java @@ -27,9 +27,14 @@ import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.schema.SchemaPlus; import org.apache.calcite.schema.impl.AbstractTable; +import org.apache.calcite.sql.SqlCall; +import org.apache.calcite.sql.SqlLiteral; import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.parser.SqlParserPos; import org.apache.calcite.sql.type.SqlTypeName; +import org.apache.calcite.sql.util.SqlShuttle; import org.apache.calcite.tools.FrameworkConfig; import org.apache.calcite.tools.Frameworks; import org.apache.calcite.tools.Planner; @@ -81,6 +86,11 @@ public abstract static class RelNodeConversionBenchmarkState { Planner p; public void setup(int length, int columnLength) { + setup(length, columnLength, SqlTypeName.INTEGER); + } + + public void setup(int length, int columnLength, + SqlTypeName columnType) { // Create Sql StringBuilder sb = new StringBuilder(); sb.append("select 1 "); @@ -111,7 +121,7 @@ public void setup(int length, int columnLength) { @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) { RelDataTypeFactory.Builder builder = typeFactory.builder(); for (int i = 0; i < columnLength; i++) { - builder.add(String.format(Locale.ROOT, "c%d", i), SqlTypeName.INTEGER); + builder.add(String.format(Locale.ROOT, "c%d", i), columnType); } return builder.build(); } @@ -161,6 +171,94 @@ public RelNode parse(SqlToRelNodeBenchmarkState state) throws Exception { return state.parse(); } + /** + * State for converting SQL that compares a VARCHAR to a large string array. + * + *

This benchmark targets allocation from traversing a large operand list. + * Run with {@code -prof gc} and compare {@code gc.alloc.rate.norm}; latency is + * not its primary metric. + */ + @State(Scope.Thread) + public static class LargeVarcharSomeArraySqlToRelNodeBenchmarkState + extends RelNodeConversionBenchmarkState { + @Param({"100000"}) + int length; + + @Setup(Level.Iteration) + public void setUp() { + super.setup(0, 1, SqlTypeName.VARCHAR); + + final StringBuilder builder = new StringBuilder(length * 8); + builder.append("select c0 from test1 where c0 = some (array["); + for (int i = 0; i < length; i++) { + if (i > 0) { + builder.append(", "); + } + builder.append('\'').append(i).append('\''); + } + sql = builder.append("])").toString(); + } + + public RelNode parse() throws Exception { + SqlNode node = p.parse(sql); + node = p.validate(node); + RelNode rel = p.rel(node).project(); + p.close(); + p.reset(); + return rel; + } + } + + @Benchmark + public RelNode parseLargeVarcharSomeArray( + LargeVarcharSomeArraySqlToRelNodeBenchmarkState state) throws Exception { + return state.parse(); + } + + /** State for traversing a large string ARRAY with {@link SqlShuttle}. */ + @State(Scope.Thread) + public static class LargeVarcharArraySqlShuttleBenchmarkState { + @Param({"100000"}) + int length; + + SqlCall arrayCall; + SqlShuttle unchangedShuttle; + SqlShuttle changedShuttle; + + @Setup(Level.Trial) + public void setUp() { + final SqlNode[] operands = new SqlNode[length]; + for (int i = 0; i < length; i++) { + operands[i] = + SqlLiteral.createCharString(Integer.toString(i), SqlParserPos.ZERO); + } + arrayCall = + SqlStdOperatorTable.ARRAY_VALUE_CONSTRUCTOR.createCall(SqlParserPos.ZERO, operands); + + final SqlNode target = operands[length / 2]; + final SqlLiteral replacement = + SqlLiteral.createCharString("replacement", SqlParserPos.ZERO); + unchangedShuttle = new SqlShuttle(); + changedShuttle = new SqlShuttle() { + @Override public SqlNode visit(SqlLiteral literal) { + return literal == target ? replacement : literal; + } + }; + } + } + + @Benchmark + public SqlNode visitLargeVarcharArrayUnchanged( + LargeVarcharArraySqlShuttleBenchmarkState state) { + return state.arrayCall.accept(state.unchangedShuttle); + } + + @Benchmark + public SqlNode visitLargeVarcharArrayChanged( + LargeVarcharArraySqlShuttleBenchmarkState state) { + return state.arrayCall.accept(state.changedShuttle); + } + /** * A state holding information needed to convert To Rel. */