vlsi commented on code in PR #5283:
URL: https://github.com/apache/calcite/pull/5283#discussion_r4348587931
##########
core/src/test/java/org/apache/calcite/test/RexShuttleTest.java:
##########
@@ -32,16 +32,167 @@
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 com.google.common.base.Preconditions.checkArgument;
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]
+ * Avoid copying unchanged SQL and Rex operands</a>. */
+ @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};
Review Comment:
Every new test (lines 67, 83, 105, 125, 142) starts from `update = {false}`,
so nothing checks that `visitList` leaves an `update[0]` that is already `true`
alone. `RexShuttle.visitWindow` depends on that: it passes one `update` array
to `visitFieldCollations(window.orderKeys, update)` and then to
`visitList(window.partitionKeys, update)`. If `visitList` ended with `update[0]
= newOperands != null`, a window whose order key changed and whose partition
keys did not would come back as the original window, and all tests in this file
would still pass. Could you add a case that visits an unchanged list with
`update = {true}` and asserts the flag is still `true`?
##########
core/src/test/java/org/apache/calcite/test/RexShuttleTest.java:
##########
@@ -32,16 +32,167 @@
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 com.google.common.base.Preconditions.checkArgument;
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]
+ * Avoid copying unchanged SQL and Rex operands</a>. */
+ @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 ReplacingListShuttle(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<RexNode> operands = createInputRefs(type, 3);
+ final RexNode replacement = new RexInputRef(3, type);
+ final boolean[] update = {false};
+
+ final List<RexNode> result =
+ new ReplacingListShuttle(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++) {
Review Comment:
The loop and the `if` compute which elements to check, and each `assertSame`
stops at the first mismatch, so a failure prints one element (or only
`expected: <3> but was: <4>` from the size check) instead of the list. Building
the expected list in the test replaces the size check, the loop, and the
condition, and prints both lists on failure:
```java
final List<RexNode> expected = new ArrayList<>(operands);
expected.set(changedIndex, replacement);
assertThat(result, is(expected));
```
Every node here has a distinct index, so equality is enough to catch a
misplaced element.
##########
ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/RelNodeConversionBenchmark.java:
##########
@@ -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.
+ *
+ * <p>This benchmark targets allocation from traversing a large operand list.
Review Comment:
`parseLargeVarcharSomeArray` measures parsing, validating, and converting a
query with 100000 literals, and the copies this PR removes are 1.28% (6.27
MB/op) of its allocation. So "targets allocation from traversing a large
operand list" describes the two `visitLargeVarcharArray*` benchmarks rather
than this one. Suggestion:
```java
* <p>Measures the allocation of parsing, validating, and converting the
* query, which includes the operand copies made by shuttles. Run with
* {@code -prof gc} and compare {@code gc.alloc.rate.norm} rather than
* latency.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
