Repository navigation
[CALCITE-7809] Avoid copying unchanged SQL and Rex operands #5283
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,31 +96,32 @@ | |
| /** | ||
| * 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> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The class Javadoc says the handler "deep-copies
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the class Javadoc: a new call is created only when an operand changes or |
||
| 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,20 @@ | |
| 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; | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: stray blank line before the closing brace.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the stray blank line. |
||
| private @Nullable SqlNode[] copyOperands() { | ||
| final List<@Nullable SqlNode> operands = | ||
| (List<@Nullable SqlNode>) call.getOperandList(); | ||
|
Check warning on line 150 in core/src/main/java/org/apache/calcite/sql/util/SqlShuttle.java
|
||
| return operands.toArray(new SqlNode[0]); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,112 @@ | ||||||
| /* | ||||||
| * 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 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 { | ||||||
|
|
||||||
| /** Test case for | ||||||
| * <a href="https://issues.apache.org/jira/browse/CALCITE-7809">[CALCITE-7809] | ||||||
| * Reduce temporary object allocation in expression shuttles</a>. */ | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The JIRA summary is now "Avoid copying unchanged SQL and Rex operands", and this line is also indented one space short.
Suggested change
|
||||||
| @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() { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Each test here changes at most one operand, so nothing checks that |
||||||
| 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; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -32,16 +32,136 @@ | |||||
| 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; | ||||||
|
|
||||||
| 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 | ||||||
| * <a href="https://issues.apache.org/jira/browse/CALCITE-7809">[CALCITE-7809] | ||||||
| * Reduce temporary object allocation in expression shuttles</a>. */ | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The JIRA summary is now "Avoid copying unchanged SQL and Rex operands".
Suggested change
|
||||||
| @Test void testVisitListReusesUnchangedImmutableList() { | ||||||
| final RelDataType type = createIntegerType(); | ||||||
| final ImmutableList<RexNode> operands = createInputRefs(type, 3); | ||||||
| final RexNode absent = new RexInputRef(3, type); | ||||||
| final boolean[] update = {false}; | ||||||
|
|
||||||
| final List<RexNode> result = | ||||||
| new ListVisitingShuttle(absent, absent) | ||||||
| .visitListForTest(operands, update); | ||||||
|
|
||||||
| assertSame(operands, result); | ||||||
| assertFalse(update[0]); | ||||||
| } | ||||||
|
|
||||||
| @ParameterizedTest | ||||||
| @ValueSource(ints = {0, 1, 2}) | ||||||
| void testVisitListCopiesOnFirstChange(int changedIndex) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every case changes exactly one operand, so nothing checks that a second change keeps the first replacement. Could you add a case that replaces the first and the last operand, keeps the middle one, and asserts all three positions of the result? |
||||||
| final RelDataType type = createIntegerType(); | ||||||
| final ImmutableList<RexNode> operands = createInputRefs(type, 3); | ||||||
| final RexNode replacement = new RexInputRef(3, type); | ||||||
| final boolean[] update = {false}; | ||||||
|
|
||||||
| final List<RexNode> 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<RexNode> operands = new ArrayList<>(createInputRefs(type, 3)); | ||||||
| final RexNode absent = new RexInputRef(3, type); | ||||||
| final boolean[] update = {false}; | ||||||
|
|
||||||
| final List<RexNode> 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<RexNode> backingList = createInputRefs(type, 5); | ||||||
| final List<RexNode> operands = backingList.subList(1, 4); | ||||||
| final RexNode absent = new RexInputRef(5, type); | ||||||
| final boolean[] update = {false}; | ||||||
|
|
||||||
| final List<RexNode> 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<RexNode> createInputRefs( | ||||||
| RelDataType type, int count) { | ||||||
| final ImmutableList.Builder<RexNode> 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<RexNode> visitListForTest( | ||||||
| List<? extends RexNode> exprs, boolean[] update) { | ||||||
| return visitList(exprs, update); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** Test case for | ||||||
| * <a href="https://issues.apache.org/jira/browse/CALCITE-3165">[CALCITE-3165] | ||||||
| * Project#accept(RexShuttle shuttle) does not update rowType</a>. */ | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method now can return
exprsitself, and subclasses will start relying on that, so the Javadoc of this method should say it. Its@returnis also wrong: the method returns a list, not an array. Suggestion:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the Javadoc to say it returns a list and may return
exprsitself when unchanged and immutable.