perf: reuse shared computation in analytic plans - #27914
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Fixed the two actionable CI failures in bbd54f7: GROUPING(args) now preserves SQL bit order (rightmost argument is the least-significant bit), with direct 3-argument mask coverage; the two SCA findings were also removed. pkg/sql/plan tests and go vet pass locally. |
bbd54f7 to
92d2057
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed on exact head e4552e4071ce256e45af97310339c28d327e0880, including the full latest hardening delta and the previous INNER/CROSS drain, wire-fence, Parquet threshold, materialization-capacity, cancellation, spill, and reuse findings.
The four exact-e33fef... blockers are materially addressed on this branch: complete-drain witnesses now fail closed across unknown/CROSS/probe paths and pin an admitted equality-hash witness to the physical build side; grouping metadata is rejected recursively at both remote encode/decode below its capability; Parquet remains independently gated at v45; and RFC v7 plus planner/runtime ledgers account for row width, fanout scans, spill framing, cumulative memory/disk/FD, oversized-batch splitting, and allocation-account cleanup. I found no additional independent implementation defect in that hardening delta.
[P1][rolling-upgrade correctness] The exact head is nevertheless no longer integration-safe: it assigns grouping-set metadata to MORPCVersion46, while current main already owns cumulative v46 for subscription-aware information-schema metadata table functions. The PR is now CONFLICTING; the merge conflict in pkg/defines/const.go exposes the protocol collision directly. A binary from either branch can advertise v46 while lacking the other branch's payload/receiver contract, so resolving the text conflict by keeping one v46 definition would create a silent mixed-version wrong-result/compatibility path.
Rebase the latest main and serialize this change after the actual current protocol owner, allocate the next unique cumulative version, and update both fresh-plan fallback and sender/receiver cached/prepared rollback tests against the real N-1 predecessor. Because other open PRs also propose the next version, do not reserve a colliding number independently; the final merge order must determine it. Request a fresh exact-head review after the conflict/version integration is complete.
Resource/liveness audit of the branch-local implementation: Q1 — producer, readers, retained batches, spill file, disk/FD reservations, transient buffers, and allocation-account charges have explicit owners and terminal cleanup; Q2 — canceled/never-started lazy UNION readers release independently and cannot backpressure the producer, while compile cleanup closes partial generations before sealing the allocation account; Q3 — retained memory (64 MiB/source), in-memory batch count, spill record size, planner cumulative spill ceiling, and statement/CN memory/disk/FD admission are bounded. The blocker is the current-main wire integration, not these local lifecycle fixes.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed on new exact head a2bd0199eca6a2570531b339b3f30cbfce287970, including the new “no partial pipeline” empty-grouping-set fix and the full prior hardening/compatibility context.
The new branch-local fix is directionally and mechanically sound: local Group creates only key rows with empty aggregate states for an all-rolled legacy set; MergeGroup synthesizes declared empty dynamic set IDs only when no partial metadata/state arrived; multiple local empty partials merge idempotently; malformed/mixed metadata fails closed; allocation-account tests cover terminal release; and the new fields are included in round-trip plus sender/receiver validation. This closes the topology where an empty distributed scan creates no partial pipeline, while preserving COUNT=0, SUM=NULL, GROUPING bits, and duplicate set identities. I found no additional independent correctness/liveness blocker in this commit.
[P1][rolling-upgrade correctness] The blocker from the immediately preceding review is still unresolved and is now wider. This head remains based on old main, remains CONFLICTING, and still assigns grouping execution to MORPCVersion46; current main already owns v46 for subscription-aware information-schema metadata functions. The new protobuf fields Group.empty_grouping_set_ids=14 and empty_grouping_set=15 are also accepted whenever the process advertises v46. A current-main v46 peer does not implement those fields or the corresponding MergeGroup synthesis, so the same-version misclassification can silently drop the required empty row—the exact wrong-result path this commit is intended to fix.
Rebase current main, integrate after the actual current protocol owner, allocate the next unique cumulative version according to real merge order, and move every grouping field (including the two new empty-set fields) plus fresh-plan/cached/prepared sender/receiver rollback tests to that gate. Other open next-version proposals mean this must be serialized; picking a colliding v47 without final merge order is not closure.
Lifecycle audit of the new path: Q1 — synthetic key vectors and aggregate states use the existing Group/MergeGroup allocation owner and Free path; Q2 — synthesis occurs once at EOF after cancellation check and adds no wait/goroutine; Q3 — rows are bounded by the number of declared all-rolled grouping sets and do not depend on input cardinality. The request remains solely the unresolved current-main protocol integration, not CI; I did not wait for running checks.
|
已按最新
验证:
请基于 exact head |
|
最新冲突已解决并推送,exact head 为 本次主线新增 #28269 的 attempt-level materialized source cleanup。合并时保留主线的 验证通过:
请基于 |
|
Rebased onto latest Validated on
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review completed on exact head 01994d1806f211d6163091d806ee3d7d9b5866d6 against base 061a343d71ad40299c5a5e4abcbbda1d4a6711fe.
APPROVE — no remaining P0/P1/P2/P3 findings.
I reviewed the complete effective diff and rechecked the prior blockers from design, correctness, performance, rolling compatibility, and unhappy-path/lifecycle perspectives:
- The design now shares only deterministic, bounded work. CTE reuse preserves row/column error-evaluation domains and requires a complete-consumer witness; join-dependent witnesses are pinned to an exact physical hash-build input. Unproved APPLY/CROSS/probe-sensitive, LIMIT, correlation, nondeterministic, unknown-width, and marginal-cost shapes retain the legacy plan.
- Grouping-set sharing evaluates the common input once, aggregates by grouping keys plus set id, and materializes only reduced aggregate output. Duplicate sets, grouping NULL versus SQL NULL, GROUPING bit order, runtime-empty global aggregates, multiple empty partials, and the distributed no-partial topology are preserved. Malformed grouping metadata fails closed.
- The materialized source has explicit ownership and finite bounds: 64 MiB / 4096 resident batches, 64 MiB spill records with row-range splitting, statement-scoped disk/FD/transient-memory admission, allocation-accounted retained and decoded vectors, independent readers, and terminal cleanup for producer error, cancellation, early lazy-UNION stop, start failure, panic, and prepared reuse. I found no unbounded queue or new goroutine/wait cycle.
- Current
mainowns MORPC v48; this head correctly serializes grouping expansion at unique cumulative v49. Fresh planning falls back below v49, and recursive sender/receiver validation rejects cached/prepared v49 payloads after rollback to v48. The append-only protobuf fields round-trip, while independent v45 Parquet and v48 CDC boundaries remain intact. - Performance admission includes declared row widths, producer savings, branch read/write traffic, spill framing, cumulative memory/spill limits, and a conservative safety factor. The supplied pinned planner measurements remain within the RFC budgets; non-admitted and rollback-controlled paths keep the established plan.
Exact-head CI is terminal green for build, UT, SCA, coverage, multi-CN Compose BVT, standalone multi-CN BVT, and summary. git diff --check is clean.
Non-blocking: please refresh the PR body before merge. It still names grouping protocol v46 / predecessor v45 and validation head e33fefbaa4; the current integration is v49 / predecessor v48 at 01994d1806.
What type of PR is this?
Which issue(s) this PR fixes:
Related to #26768
What this PR does / why we need it:
Implements the shared-computation closure of the analytic rewrite series. The accepted design is the in-progress RFC on main.
UNION ALLscope subtree in child-to-parent order. This prevents both the LIMIT/OFFSET wait cycle and resource retention on early stop/cancellation, including prepared-statement reuse.sharedComputation=1.Exact-head validation (
e33fefbaa4):go test ./pkg/sql/compile ./pkg/sql/plan ./pkg/sql/internal/materialized ./pkg/sql/colexec/merge ./pkg/sql/colexec/unionall: PASS.TestGroupingSetMaterializedFanoutWithLazyUnion: PASS. Its 10,001-row shared aggregate exceeds ordinary two-reader fanout capacity;LIMIT 1 OFFSET 1000000drains, and earlyLIMIT 1releases the unstarted reader before a follow-up statement.UNION ALLand materialized dependent-reader/cancellation/spill paths: PASS.golangci-lint run ./pkg/sql/compile ./pkg/sql/plan ./pkg/tests/issues: 0 issues;git diff --check: clean.