Skip to content

perf(sql): reduce SQL planning allocations for large literal arrays - #20367

Open
FrankChen021 wants to merge 5 commits into
apache:masterfrom
FrankChen021:codex/bypass-string-array-reparsing
Open

FrankChen021 wants to merge 5 commits into
apache:masterfrom
FrankChen021:codex/bypass-string-array-reparsing

Conversation

@FrankChen021

Copy link
Copy Markdown
Member

Related to #20326.

Description

Large literal arrays already contain constant Calcite values, but the existing planning path converts them into Druid expressions, formats them as text, parses that text, evaluates it, and rebuilds Calcite literals:

DruidRexExecutor.reduce()
  Expressions.toDruidExpression(...)
    ArrayConstructorOperatorConversion
      DirectOperatorConversion
        Convert each literal into a child DruidExpression
        DruidExpression.ofFunctionCall(..., "array", operands)
          Creates the text-generator callback

  druidExpression.getExpression()
    Formats "array('a','b',...)"

  Parser.parse(...)
    Builds a new Druid expression tree

  expr.eval(...)
    Produces the array value

  rexBuilder.makeLiteral(...)
    Rebuilds the Calcite array and its elements

This round trip allocates per-element expression objects, strings, parser objects, evaluated values, and replacement Calcite nodes.

This change skips that work for already-normalized string and integer literal arrays. Expressions requiring conversion or evaluation retain the existing path.

Benchmark results

Type Elements Before MB/op After MB/op Diff MB/op Reduction Before ms/op After ms/op
LONG 100,000 355.87 226.23 129.63 36.43% 215.95 153.58
LONG 1,000,000 3535.11 2216.22 1318.89 37.31% 2899.52 2324.95
STRING 100,000 1541.08 1231.01 310.06 20.12% 788.72 580.71
STRING 1,000,000 15561.88 12307.44 3254.44 20.91% 8106.12 6150.46

MB is decimal; allocation means total bytes allocated per planning operation, not peak or retained heap.

Both variants use Calcite 1.42.0 with only the CALCITE-7782 fix. The benchmark isolates this Druid change using identical dependencies and harness settings.

JDK 25.0.4.1; two forks; two warmup and three measurement iterations; GC profiler. Queries are planned without execution or EXPLAIN serialization. Timing results are preliminary.

Tests

Tests cover identity reuse, NULLs, integer boundaries, type mismatches, charset/collation differences, computed operands, and SQL IN/NOT IN semantics.

  • 261 tests passed with the declared Calcite dependency.
  • 186 targeted tests passed with patched Calcite. The broader patched run encounters an unrelated exact-version assertion.

Release note

Reduced SQL planning allocations for large literal string and integer arrays, including arrays generated from large IN lists.

Copilot AI lite review requested due to automatic review settings September 17, 2026 03:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The optimization is type-gated, preserves fallback evaluation, and includes focused correctness and benchmark coverage.

Pull request overview

Reduces SQL planning allocations for large literal string and integer arrays, especially arrays generated from large IN lists.

Changes:

  • Adds a type-safe literal-array reuse fast path.
  • Adds planner and SQL semantic regression tests.
  • Adds string and long plan-only benchmark variants.
File summaries
File Description
DruidRexExecutor.java Reuses compatible literal array calls.
DruidRexExecutorTest.java Tests type matching and evaluator fallbacks.
CalciteQueryTest.java Verifies IN/NOT IN semantics.
InPlanningBenchmark.java Measures planning-only performance.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@FrankChen021 FrankChen021 changed the title Reduce SQL planning allocations for large literal arrays perf(sql): reduce SQL planning allocations for large literal arrays Sep 17, 2026
@FrankChen021
FrankChen021 requested a review from gianm September 17, 2026 03:19

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Static review of the current head found no actionable PR-caused issues. The literal-array fast path is restricted to ARRAY_VALUE_CONSTRUCTOR calls whose component type is CHAR/VARCHAR or TINYINT/SMALLINT/INTEGER/BIGINT and whose operands are RexLiteral values with matching types ignoring nullability; casts, computed operands, mixed or mismatched types, unsupported types, and required normalization continue through the existing Druid evaluator path.

Reviewed 4 of 4 changed files:

  • benchmarks/src/test/java/org/apache/druid/benchmark/query/InPlanningBenchmark.java
  • sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidRexExecutor.java
  • sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
  • sql/src/test/java/org/apache/druid/sql/calcite/planner/DruidRexExecutorTest.java

I also inspected the executor registration and callers, scalar-IN and array conversions, Calcite array type/coercion behavior, relevant test data, and the benchmark lifecycle. Validation run: git diff --check against the PR merge base; it passed. No builds or broad tests were run.


This is an automated review by Codex GPT-5.6-Luna(max)

@FrankChen021 FrankChen021 added this to the 39.0.0 milestone Sep 19, 2026

@GWphua GWphua left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with comment

///
/// Returns null for mismatched element types, unsupported types, or non-literal operands (including CAST calls).
/// These retain the existing Druid expression evaluation path, including any required type normalization.
///

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// Seems like a wrong JavaDoc format

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is markdown format Java doc from JDK 23

@FrankChen021

Copy link
Copy Markdown
Member Author

I will keep this open until druid 39 is going to be cut off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants