[CALCITE-7809] Avoid copying unchanged SQL and Rex operands

[FrankChen021]

Fixes CALCITE-7809.

Why

RexShuttle and SqlShuttle copy operand collections before knowing whether traversal will replace an operand. Read-only traversals therefore allocate and discard copies, adding memory pressure for large expressions. In Druid's string-IN planning benchmark, this change reduced allocation by 2.72% at 100,000 literals and 1.55% at 1,000,000 literals. Timing confidence intervals overlapped, so no latency improvement is claimed.

What

Make call-operand traversal copy-on-write: reuse unchanged immutable Rex lists and SQL calls, and allocate only when an operand changes or alwaysCopy requires a new call.

Verification

Focused tests cover unchanged identity, first/middle/last replacements, mutable and partial-view Rex lists, changed SQL operands, and alwaysCopy.

Calcite JMH with 100,000 VARCHAR ARRAY operands and -prof gc:

Path	Baseline	This PR	Baseline allocation	This PR allocation
Unchanged	0.138 ms/op	0.068 ms/op	400,016 B/op	0.029 B/op
One changed operand	0.281 ms/op	0.280 ms/op	800,080 B/op	800,112 B/op

The end-to-end VARCHAR = SOME(ARRAY[...]) benchmark reduced allocation by 6.27 MB/op (1.28%).

Scope

This PR covers RexShuttle.visitList and SqlShuttle call operands. Plain IN literals inside SqlNodeList and other eager-copy visitor paths are unchanged.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[julianhyde]

This is slop. Do not merge.

[caicancai]

This PR description looks very AI slop.

[vlsi]

Thanks for the change. I ran SqlValidatorTest, SqlToRelConverterTest, RelOptRulesTest, RexProgramTest, and RexShuttleTest on the PR head: 2444 tests, 0 failures. The approach is sound and the risk is low. A few things before merge:

1. Nothing tests the property the PR adds. If visitList goes back to always copying, every existing test still passes, so the optimization can regress without anyone noticing. Could you add tests along these lines?
   • RexShuttle.visitList on an unchanged ImmutableList returns the same instance and leaves update[0] false.
   • A shuttle that replaces a middle operand returns a new list with the prefix and suffix intact and sets update[0]. This catches an off-by-one in subList(0, i). Replacing the first and the last operand covers the boundaries.
   • An unchanged input that is not an ImmutableList still comes back as an ImmutableList with equal elements.
   • SqlShuttle returns the same SqlCall when no operand changes, returns a new call with the other operands kept as the same instances when one operand changes, and returns a new instance when alwaysCopy is true and nothing changes.
2. The measurement comes from Druid's benchmark, which nobody can rerun from this repository. ubenchmark already has RelNodeConversionBenchmark. A case with a large ARRAY[...] or IN list, run with -prof gc, would show the effect in Calcite itself and catch a regression later.
3. Scope: a plain x IN ('1', ..., 'N') in Calcite (no Druid rewrite) goes through SqlShuttle.visit(SqlNodeList), which still allocates a full ArrayList on every traversal. RexShuttle.visitArray, visitFieldCollations, and the visitList overrides in ProjectFilterTransposeRule and DateRangeRules still copy eagerly too. Either cover visit(SqlNodeList) here or say in the description that this PR targets only the call-operand paths.
4. The commit subject says [CALCITE-7805], but this PR is CALCITE-7809. CALCITE-7805 is the umbrella allocation issue.

Minor, for the description: the "based directly on main at 38413ec" sentence only matters while the PR is open, and the arrow diagrams could each be one sentence.

[vlsi]

ImmutableList.copyOf already returns its argument when that argument is a full ImmutableList, so the instanceof branch and the unchecked cast aren't needed:

return clonedOperands != null ? clonedOperands.build() : ImmutableList.copyOf(exprs);

It also handles a case the current branch gets wrong: ImmutableList.subList(...) is an instanceof ImmutableList view that keeps the whole backing array reachable. copyOf compacts such a view, and this branch returns it as is. I checked both cases on Guava 33.4.8.

[FrankChen021]

Updated to use ImmutableList.copyOf(exprs). It reuses a full ImmutableList and compacts partial views; a partial-view test was added.

[vlsi]

Nit: ImmutableList.builderWithExpectedSize(exprs.size()) avoids growing the builder on the path where an operand changed.

[FrankChen021]

Calcite supports Guava 21, where builderWithExpectedSize is unavailable, so I retained ImmutableList.builder().

[vlsi]

The method now can return exprs itself, and subclasses will start relying on that, so the Javadoc of this method should say it. Its @return is also wrong: the method returns a list, not an array. Suggestion:

   * @return List of visited expressions; {@code exprs} itself if it is an
   *         {@link ImmutableList} and no expression was modified

[FrankChen021]

Updated the Javadoc to say it returns a list and may return exprs itself when unchanged and immutable.

[vlsi]

The class Javadoc says the handler "deep-copies SqlCalls and their operands". That wasn't accurate before this PR and is less accurate now: the handler creates a new call only when an operand changes or when alwaysCopy is true. Since the PR touches this class, could you fix the sentence?

[FrankChen021]

Updated the class Javadoc: a new call is created only when an operand changes or alwaysCopy is true.

[vlsi]

Optional simplification: if result() copies the operands when clonedOperands is still null, the constructor needs no branch and alwaysCopy gets lazy copying too. When nothing changed, copying at result() gives the same array as copying here.

@Override public SqlNode result() {
  if (!update && !alwaysCopy) {
    return call;
  }
  final @Nullable SqlNode[] operands =
      clonedOperands != null ? clonedOperands : copyOperands(call);
  return call.getOperator().createCall(
      call.getFunctionQuantifier(), call.getParserPosition(), operands);
}

[FrankChen021]

Applied. alwaysCopy now copies operands lazily in result() when a new call is required.

[vlsi]

Nit: stray blank line before the closing brace.

[FrankChen021]

Removed the stray blank line.

[FrankChen021]

This PR description looks very AI slop.

@julianhyde @caicancai I don't accept the word 'slop' here.

I deliberately guided the AI to generate the description, to include:

• The problem description
• Code execution path and new code path for illustration for people to better understand the changes before and after
• Benchmark data as evidence to show the outcome

It may be long, but it's clear.

I wrote an email in the dev mailing list, we can discuss about this(non-implementation details) in that thread.

[FrankChen021]

@vlsi Thank you very much for the feedback. I will address these problems later this week.

[vlsi]

It may be long, but it's clear.

From my point of view, it is hard to answer "why do we need the change at all?" question.
As a reviewer and Calcite maintainer, I don't want to dig into details until I understand why is that needed at all.

Frank, I wonder if you could try this skill: https://github.com/Netcracker/qubership-ai-packages/tree/main/agent-packages/change-description-authoring

See https://github.com/Netcracker/qubership-ai-packages/blob/main/agent-packages/change-description-authoring/.apm/skills/change-description-authoring/SKILL.md#pull-request-title

From my point of view, it produces decent commit messages: #5278, #5230, #5213

[FrankChen021]

It may be long, but it's clear.

From my point of view, it is hard to answer "why do we need the change at all?" question. As a reviewer and Calcite maintainer, I don't want to dig into details until I understand why is that needed at all.

Frank, I wonder if you could try this skill: https://github.com/Netcracker/qubership-ai-packages/tree/main/agent-packages/change-description-authoring

See https://github.com/Netcracker/qubership-ai-packages/blob/main/agent-packages/change-description-authoring/.apm/skills/change-description-authoring/SKILL.md#pull-request-title

From my point of view, it produces decent commit messages: #5278, #5230, #5213

That's a good suggestion. Let's talk about this case here, the "why" question I think is stated in the JIRA as:

https://issues.apache.org/jira/browse/CALCITE-7809

RexShuttle and SqlShuttle visit every child and return either the original child or a replacement. They currently copy operand collections before knowing whether any child changes, then discard those copies when the traversal is read-only.

Make these operand collections copy-on-write. Allocate only when the first changed child is encountered, while preserving RexShuttle's immutable-result contract and SqlShuttle's explicit alwaysCopy behavior.

The isolated Druid benchmark reduced allocation by 2.72% at 100,000 string literals and 1.55% at 1,000,000 string literals.

Do you think the description is clear to answer the why ?
If not, then I can improve the description, if yes, I think the problem here is that we have PR and JIRA separated, next time I can attach these description in PR, and to focus on why first, I can fold the details by default in the description. does it sound good to you?

[FrankChen021]

@vlsi I updated the description, does it sound good to you?

[vlsi]

The updated description is better:

• The first sentence in "Why" clarifies everything. From my point of view it makes a huge difference.
• I think "what" section is excessively large. There's no point in those ascii-charts. A mere "reuse input operand list and create a new only when at least one operand changes" would probably do.
• I'm not sure listing "I ran tests, I ran autostyle" adds value. We have CI for that. I would prefer "verification" to include "targeted" information for the PR. For instance, it could be "here's a test I added, here are Calcite benchmark results.

PS. The PR still misses calcite-level tests, and calcite-level benchmarks. If the PR had those tests, the description could be way better.

[FrankChen021]

PS. The PR still misses calcite-level tests, and calcite-level benchmarks. If the PR had those tests, the description could be way better.

Correct. I have not done this yet, it takes time. Thanks.

[FrankChen021]

@vlsi Thanks for the detailed review. I have addressed your above comments. Could you please take another look?

The benchmark now includes VARCHAR = SOME(ARRAY[...]). It shows a 1.28% end-to-end allocation reduction, smaller than the Druid result because the Druid planning pipeline exercises the optimized traversal more extensively. The unchanged traversal improves substantially, while the changed-operand path shows no material regression.

@julianhyde You mentioned this PR in the mailing-list discussion. The change is limited to making operand copying lazy, with tests covering unchanged operands, changed operands, and alwaysCopy. If you have additional concrete scenarios or benchmarks you would like covered, please let me know.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud