Skip to content

perf: bound repeated work in analytic queries - #28752

Open
aptend wants to merge 15 commits into
matrixorigin:mainfrom
aptend:perf/tpcds-timeout-heads-20260911
Open

perf: bound repeated work in analytic queries#28752
aptend wants to merge 15 commits into
matrixorigin:mainfrom
aptend:perf/tpcds-timeout-heads-20260911

Conversation

@aptend

@aptend aptend commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Related to #26768.

What this PR does / why we need it:

Large analytic plans can repeat the same CTE/grouping work, miss safe early pruning, project unique hash matches row by row, and retain a full partition for a small RANK bound. This PR addresses those mechanisms generically:

  • restores CTE and grouping-set sharing only with deterministic full-drain proofs, conservative materialization estimates, statement spill budgets, and legacy fallback;
  • keeps grouping-set aggregates on the raw input stream, while ignoring inactive rollup keys during group-cardinality estimation;
  • exposes safe common keys from mixed DNF predicates and delivers uncorrelated scalar predicates through joins only when the physical runtime-filter topology is provably local;
  • batches projection for unique residual-free inner hash joins;
  • extends bounded Partition Top-N to exact RANK <= N semantics by retaining boundary ties, with chunked output, accounted state, and an MORPC v67 compatibility gate.

No query ID is encoded in these rules. Statistics participate only in conservative cost/admission decisions, never in semantic legality. Unsupported, uncertain, mixed-version, or over-budget shapes keep the previous plan. The design and counterexample matrix are recorded in the stats-independent analytic rewrite RFC v10 (exact revision).

Measured on the isolated 1T TPC-DS lab with 128 MiB operator spill limits, merge disabled, a 20 GiB no-swap cgroup, and memory/OOM monitoring:

  • Q2 completed in 914.102s;
  • Q14a completed in 1059.257s; Q14b changed from the 1800s timeout to 994.934s.

The latest revision removes decimal SUM(SUM(v)) prefix reuse. Checked fixed-width addition is not associative in error semantics: a finer partial sequence can overflow even when the raw coarser input succeeds through cancellation. Dynamic grouping-set expansion remains available because each grouping set consumes raw rows in their original order.

Exact-head validation includes the complete pkg/sql/plan test package, go vet ./pkg/sql/plan, a clean build, and an executed public-SQL differential between shared and legacy grouping-set plans. The regression covers Decimal(38,0) near-bound cancellation in the failing row order, NULL aggregate input, runtime-empty input, and four nested rollup prefixes. CI on the updated head is pending.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@matrix-meow matrix-meow added the size/XXL Denotes a PR that changes 2000+ lines label Sep 12, 2026
@aptend
aptend force-pushed the perf/tpcds-timeout-heads-20260911 branch from 5222ef2 to 231ec9a Compare September 12, 2026 12:56
@aptend

aptend commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the P1 on exact head 58ec680c6de96fc8db0e07a22f0716973a65bb95.

I removed decimal aggregate-prefix reuse entirely, including its separate 32 GiB admission path. The remaining grouping-set rewrite aggregates each set from raw input, so it does not introduce SUM(SUM(v)) reassociation. RFC v9 records the checked-overflow counterexample and makes raw-input aggregate order part of the contract.

Added two complementary regressions: a planner-shape test that requires decimal SUM to use raw-input dynamic grouping, and an executed public SQL differential against the legacy plan covering the exact Decimal(38,0) cancellation order, NULL input, runtime-empty input, and four rollup prefixes. The shared and legacy executions returned identical results locally; pkg/sql/plan, go vet ./pkg/sql/plan, and a clean build pass. The revised implementation removes state rather than introducing ownership, so the existing shared-source spill/reset contract is unchanged.

@aptend
aptend requested a review from XuPeng-SH September 13, 2026 09:26
@aptend aptend added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/XXL Denotes a PR that changes 2000+ lines labels Sep 13, 2026

@XuPeng-SH XuPeng-SH 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.

GPT-6 medium deep review at exact head 58ec680c6de96fc8db0e07a22f0716973a65bb95:

Source review found no verified correctness blocker in the current planner/executor changes. The prior Decimal SUM reassociation issue is closed: finalized-prefix reuse is gone, grouping sets consume raw input, and the added plan-shape plus SQL differential cases cover cancellation, NULL, empty input, and nested prefixes. I also reviewed the CTE/full-drain and scalar-filter proofs, DNF/semi-containment changes, unique hash-join projection, peer-aware RANK Top-N, and v67 compatibility/fallback paths.

P2 — required implementation-acceptance evidence is incomplete. RFC v9 makes these measurements approval gates and requires exact-revision raw artifacts. The PR body reports only three TPC-DS elapsed times; I could not find the fixed TPC-H control results, base-versus-candidate planner p50/p95 time and allocation data, or per-target rows/bytes scanned, peak query memory, and spill bytes. Please attach the exact-revision report (or mark unavailable scale runs as open under the RFC's exception, with its required plan/result/resource artifacts). The exact-head run 34749464335 also still has UT, SCA, coverage, and BVT checks pending, so the mandatory validation set is not terminal yet.

The RFC states that one exact-head APPROVE accepts the design, implementation, and evidence together. I am therefore requesting changes for the missing acceptance evidence, not claiming an unresolved code defect.

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

Labels

kind/enhancement size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants