Repository navigation
ci: use bounded cluster admission for coverage UT - #468
Conversation
## What type of PR is this? - [ ] API-change - [ ] BUG - [x] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #31037. ## What this PR does / why we need it: `GROUP BY x,x-1,x-2,x-3` carries redundant exact integer keys through aggregation. Remove derived keys only when the existing range proof establishes a total function of explicitly retained, identically typed grouping columns, and the proposed retained inputs and complete aggregate list are within the shared fixed-input/grouped-only/ordinary COUNT cost domain. Reconstruct consumers from original bound trees, preserving resolved overloads and complete result types. Run at the existing all-consumers-bound boundary, before aggregate layout is fixed. Reuse source substitution, registry compaction and the atomic bound-slot remapper shared with affine SUM rewriting. Preflight all consumers; unsupported arithmetic, diagnostics, provenance, query references, stale types/slots, grouping extensions and excessive work retain the existing plan. Generic Group/MergeGroup, memory admission and spill remain the runtime owners. Keep aggregate distribution at its existing owner. Its bounded pressure estimate recognizes known fixed group/COUNT state that is expected to spill per partial owner but fit with complete partition ownership. Reuse runtime hash capacity estimates, DOP and spill-threshold resolution, with topology, selected-NDV and raw-transport guards. Share this same eligibility/payload calculation with pruning admission; do not copy executor state layouts or infer state from result width. The two review regressions are addressed together: COUNT retains the common-owner pressure correction; SUM, AVG, MIN/MAX, DISTINCT, variable-state/mixed aggregates, unsupported retained keys and wide/computed aggregate inputs now skip pruning atomically. Their original keys and distribution decisions remain. The handwritten single-key SUM retains its pre-existing policy and can still spill. This deliberately narrows optimization coverage instead of adding aggregate-specific estimators or forcing shuffle. The versioned [design](docs/design/20261024-derived-group-by-pruning.md), including the cost-domain amendments, was reviewed by **gpt-6.1-sol / xhigh** before implementation. The same selected model completed the final overall review with **PASS / no unresolved material findings** on the exact delivered source fingerprint. Delivered head: `4be001a3fac34a80bdc8b3ed454feee7e3025796`. Validation on rebased main `501a569224d2586effd7460df5bf94790128b8cf`, Go 1.27.1 / macOS arm64, matching verified native libraries: - Full planner normal suite: **4,157 root tests pass**. New/direct pruning, affine and shuffle tests: **14 roots / 138 nodes pass once with race**. Formatting, vet and configured incremental golangci pass (**0 issues**). Molint retains five advisories at unchanged sites. New atomic tests check consumers, aggregate lists, keys, flags and all registries; an exact old-pruner overlay fails the intended SUM plan and atomic assertions. - Existing metadata-comparing BVT: **23/23 statements pass twice on one instance**, including NULL/boundaries, diagnostics, prepared reuse and teardown. Fresh base/fix/fix/base public comparisons match **22 records** of rows, metadata, errors/warnings and prepared execution. Invalid integer CAST follows rebased main's zero + warning 1292 contract; invalid DECIMAL input still errors. - Moderate-cardinality regression: one million INT rows / 100,000 groups, DOP4, 4 MiB. In A/B/B/A blocks, each with two warmups and four measured samples, the derived SUM query has base/fixed medians **18.10/17.55 ms**. Both retain four keys, range shuffle and **zero spill**, with about 13.4 MiB reported aggregate memory. The prior head independently reproduced 10.90 MiB spill locally; its older physical dataset is not used to claim a matched speedup ratio. - Derived COUNT at 4 MiB has base/fixed medians **14.66/6.17 ms**; fixed derived COUNT, single-key COUNT and grouped-only use complete ownership without spill. Each block also checks all **100,000 SUM result groups** against an independent formula. - Nineteen public performance/control shapes cover auto/16 MiB budgets, DOP1/2/8, single SUM/COUNT, mixed aggregates, wide and computed-wide inputs, low NDV, selective/skewed data and actual forced spill. Raw samples are retained; overlapping control ranges do not support attributing small timing differences. Single-key SUM still spills at 4 MiB, as on base. At moderate cardinality with auto/16 MiB resident budgets, COUNT still uses partial/merge ownership: aggregate memory can rise from about 13.45 MiB to 26.16 MiB while output shrinks from 3.43 MiB to 1.14 MiB and timing ranges overlap. That existing resident-policy tradeoff is not fixed here. No universal latency or memory reduction is claimed. - Historical 10M/50M INT+COUNT evidence below remains applicable only to its unchanged high-NDV pruning/ordinary-shuffle path; those timings were not rerun. Aggregate memory figures are operator diagnostics, not process RSS. Controlled synthetic Q36 input, unchanged SQL, four workers, same persisted data and build settings; A/B/B/A blocks each use two warmups and four measured runs (eight measured samples per variant): | Input | Baseline median | Candidate median | Wall reduction | | --- | ---: | ---: | ---: | | 10 million rows | 0.250s | 0.142s | 43.3% | | 50 million rows | 1.542s | 0.904s | 41.4% | Every timed result is checked. The 50M plan reduces four grouping keys to one; reported aggregate memory falls from 1.84 GiB to 799.34 MiB and aggregate output from 858.21 MiB to 286.07 MiB, with no spill. These are operator measurements, not process RSS. The original single-key, retained composite-key and low-cardinality resident controls have overlapping sample ranges; the additional moderate-cardinality budget controls above address distribution changes. This uses synthetic data, not the official ClickBench distribution, and does not establish closure of the remaining single-key gap to DuckDB. Remote CI for the updated head is not yet verified. The prior head's latest UT job failed during Java TLS download, and coverage failed during the five-minute source checkout; neither reached test execution. The earlier, separate embedded-cluster admission incident is tracked in [matrixorigin/CI#468](matrixorigin/CI#468); full owning-runner validation remains separate.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Recommendation: REQUEST_CHANGES — one outstanding runtime acceptance gate.
Reviewed 11441a1d4f7f7f57aa640420abb78383c8652013 against base/merge-base 49251b9bc656fdad6fb16aaa1fb78e7eba35c7d5, including the existing admission/lifecycle owners on MatrixOne main 4769417cdce5.
The three-line implementation is appropriately scoped: it reuses the existing process pool, preserves same-process exclusion, and adds no scheduler, duplicate state, compatibility branch, test-selection change or weaker deadline/failure gate. I found no demonstrated implementation defect. Original diagnostics independently confirm 326.6s and 305.2s admission waits; the change therefore has a valid rationale.
Validation checked:
- Exact-head workflow validation is green. Parsed YAML differs only by this step-local environment setting.
- Latest-main admission package normal and race tests pass (10 roots / 25 nodes each).
- Independent multiprocess challenge passes: two owners, third-owner cancellation, pool/exclusive exclusion, SIGKILL slot recovery, surviving-owner exclusion and subsequent reuse; all nine subprocesses reaped.
- The real coverage wrapper propagates the setting and preserves exit 0/passed versus exit 7/failed. Author's existing focused coverage and Linux wrapper evidence was inspected and reused where semantically unchanged.
The remaining merge blocker is the complete owning-runner acceptance already required by the PR description and its design/review record. A focused upgrade pass does not test the contention or resource envelope this PR changes. See the inline finding for the concrete required evidence. No existing review comments or questions were present to resolve.
GitHub does not permit an author to submit APPROVE/REQUEST_CHANGES on their own PR; the authenticated account and author are both XuPeng-SH, so this is published as COMMENT with the explicit recommendation above.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Updated recommendation: APPROVE — no unresolved blockers. This supersedes my earlier REQUEST_CHANGES recommendation: the owning-runner full-coverage acceptance has now passed and the inline blocker is resolved.
Real canary run verified the exact candidate workflow 11441a1, MatrixOne f197f8e (main 4769417 plus only a temporary caller), effective pool=2, -p8 and the existing Guangzhou 16GiB runner. 505 packages, producer exit0/passed, zero failure events; pure tests 1525.9s, complete coverage step30m08s, entire job31m53s. All upgrade-recovery scenarios passed (root71.40s). The uploaded mode=set coverage profile was downloaded and parsed:411385 blocks,35090048 bytes. No test/assertion/deadline relaxation was introduced.
Peak RSS/OOM counters and detailed raw admission-wait events are not retained by the successful path. Therefore this is evidence of one successful complete representative run, not a quantified memory margin, matched performance comparison or universal no-OOM guarantee. No additional scheduler/state/compatibility machinery is justified by the evidence.
The authenticated reviewer is also the PR author, so GitHub's self-review restriction requires COMMENT rather than formal APPROVE.
Coverage UT launches eight packages while embedded clusters default to one runner-wide admission owner. In the failing MatrixOne run, two successful embedded tests spent 631.8 seconds waiting for admission. The final upgrade-recovery scenarios then exhausted the package's existing 20-minute deadline.
Enable the existing two-process cluster-admission pool only in the coverage step. Same-process exclusion, package parallelism, timeout limits, runner class, coverage instrumentation and failure reporting remain unchanged. This reuses MatrixOne's existing slot/gate owner without adding scheduling logic. Related: matrixorigin/matrixone#31040.
Validation:
TestClusterAdmissionAtStartand all fourTestCrossTargetUpgradeRecoveryscenarios pass locally (42.2 seconds, macOS arm64).killpgEPERM on macOS; Linux validation covers the workflow platform.Live acceptance passed on the owning Guangzhou 16GiB runner: canary run 40268403568, from temporary MatrixOne PR #31189. Logs verify this exact workflow SHA
11441a1d4f7f7f57aa640420abb78383c8652013, MatrixOne checkoutf197f8e0bb3e9c92ee09193afd86a39e063f48b6(main4769417cdce5531c9771644eca49f1e71d8f654cplus only the temporary caller), pool=2 and package parallelism=8. All 505 selected packages completed, producer exit0/passed, zero failure events. Pure tests took 1525.9s; the coverage step took30m08s, the job31m53s. Upgrade recovery passed in71.40s. The downloaded mode=set profile contains411385 blocks and35090048 bytes; upload and final producer status succeeded.Peak RSS/OOM counters and detailed raw admission waits were not retained on this successful path. This proves one complete representative run, not a quantified memory margin or matched performance speedup. No OOM failure occurred in the run; no universal no-OOM claim is made. The full-run review blocker is resolved.