perf(plan): avoid lossy string aggregate range shuffle - #28646
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? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed current head 2a484f1 against main 44b7f90. No concrete merge-blocking issue found. COMMENT only because this is a self-authored PR.
The guard is a conservative planner-policy change: CHAR/VARCHAR/TEXT aggregate distribution keys retain the reset hash strategy instead of deriving range bounds from an eight-byte prefix. Range inheritance is rejected without retaining stale Reuse/range metadata; full-key hash reuse remains eligible. The existing aggregate cardinality/cost decision may still choose no shuffle. I traced selected GroupBy-column identity, direct-column versus expression eligibility, multi-key/grouping-set selection, DISTINCT topology exclusions, strategy reset/reuse, and the compiler's group-shuffle argument construction. Numeric and join range selection are not disabled, and fresh string range statistics remain available to non-aggregate consumers.
Importantly, prefix loss here demonstrates possible severe partition skew, not by itself a wrong-result counterexample: prefix collisions need not split equal complete keys. This change does not introduce a new string comparator, aggregate state format, persistent metadata, or wire representation. NULL/empty/long/multibyte keys continue through the existing hash/no-shuffle execution paths; aggregate expressions and their explicit ordering semantics are unchanged.
The versioned design update explains the restriction and the future full-key-sketch prerequisite. The focused planner tests exercise fresh string-range rejection and inherited-range rejection; the composition regression protects the non-aggregate metadata contract. Optional follow-up: table-drive CHAR/TEXT and a retained hash-reuse control alongside VARCHAR, and measure a well-distributed short-string workload to quantify the deliberate conservative tradeoff. These are not blockers.
Validation: inspected the complete four-file diff, PR discussion/review history, and relevant exact-head planner/compiler consumers; rechecked head/base before submission. The PR reports package tests, vet/build and a ClickBench comparison; I did not independently rerun native tests, distributed execution, or those performance measurements, and did not wait for CI.
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #28550. Completes the expired-task test follow-up documented in #28668. ## What this PR does / why we need it: HAKeeper state transitions need one driver per store. Two paths violated that ownership: 1. `TestTaskSchedulerCanReScheduleExpiredTasks` manually bootstrapped and scheduled tasks while its fixture also ran the production HAKeeper checker and task ticker. This caused the allocator race reported by [the #28646 UT job](https://github.com/matrixorigin/matrixone/actions/runs/34554939075/job/103145324437). 2. In production, stopping a local HAKeeper replica leaves the store-owned ticker alive, but starting that replica again admitted another ticker. A real NodeHost stop/resume probe against unchanged main production code observed **two worker entries instead of one**, through both voting and non-voting startup paths. Extra checkers share the allocator/bootstrap state and also start extra task schedulers. Both replica startup paths now share one store-lifetime startup guard. Concurrent callers observe the same admission result; shutdown rejection is terminal for that store. Replica stop/resume reuses the original ticker, while a new store gets a fresh guard. This adds synchronization only at worker startup, with no new locks, scans, or allocations in tick, task scheduling, ID allocation, or query execution. The manual task-service fixture now always disables background workers. Its shared bootstrap helper enforces that precondition, eliminating duplicated bootstrap code. The expired-task test advances the **real replicated HAKeeper clock** explicitly instead of sleeping for expiration. It retains the existing complete-and-truncate behavior for expired tasks and adds live-runner and exact-boundary controls. The explicit task-ticker test remains asynchronous and joins its worker before teardown. FD checks now inspect the initialized fixture filesystem. ### Test plan / QA reference | Contract | Verification | |---|---| | Original flaky case | `TestTaskSchedulerCanReScheduleExpiredTasks`: no eligible CN retains a Created task; exact expiry boundary retains the running task; the next tick removes only the expired runner's task; repeated scheduling preserves the live task; a returning CN accepts new work without resurrecting the old task. | | Other manual-bootstrap consumers | `TestTaskSchedulerCanScheduleTasksToCNs`, `TestTickerForTaskSchedule`, `TestAllocateIDByKeyWithRequestID`. All use the checked shared helper; the ticker case still proves asynchronous assignment. | | Production worker lifetime | `TestHAKeeperTickerSurvivesReplicaRestart`: concurrent startup admission; real voting and non-voting stop/resume; replicated tick progress before and after voting-replica restart; exactly one worker entry after joining all workers. | | Shutdown / disabled-worker admission | `TestHAKeeperTickerRejectsStartupAfterShutdown`: concurrent callers receive `stopper.ErrUnavailable`; disabled-worker fixtures create no worker. | | Counterexample strength | Temporary Go overlays changing CN expiry from `<` to `<=`, disabling expiry entirely, or re-enabling the manual fixture's background workers each fail the corresponding assertion. These deliberately incorrect variants are not committed. | Validation on Linux/amd64, Go 1.26.4, using source-matched native artifacts and `.agents/skills/mo-dev/scripts/mo-cgo-test`: - The four manual-bootstrap consumers above each passed `-race -count=100` in a single process. Their worker-disabled execution paths are unchanged by the subsequent production startup guard. - The restart test passed `-race -count=68`; shutdown admission passed `-race -count=100`. Counts follow a 30-second per-test stress budget (restart measured 0.44 seconds). - Final `pkg/logservice` normal suite passed (110.622 seconds); the full race suite passed (112.295 seconds), both with `-count=1 -timeout=600s`. - Scoped `golangci-lint --new-from-rev=<base> ./pkg/logservice`, package `molint`, and `git diff --check` passed. - Expiration-test body time under race decreased from roughly 1.3 seconds to 0.2 seconds locally. The new two-mode restart test takes roughly 0.44 seconds. No production throughput gain is claimed. The SQL surface and wire/disk formats do not change. The affected restart boundary is exercised with real in-memory NodeHosts; no full SQL cluster, retained-data upgrade, or 55-machine test is claimed. Rebased onto main `4a7bfac4f9` before delivery. The intervening commit changes only `pkg/embed`, outside the verified `pkg/logservice` test dependency graph; native inputs and all validated code/test paths are unchanged, so the completed evidence is reused. ### Ownership / unhappy-path review | Boundary | Closure | |---|---| | Startup | One `sync.Once` admission per store; both replica roles share it; no RPC is held under the guard. | | Replica stop/resume | The original ticker remains the owner and observes the current replica identity; no additional checker/task ticker accumulates. | | Store shutdown | Existing stopper cancellation joins its workers before NodeHost closure; manual ticker cleanup also joins before fixture teardown. | | Admission failure / fresh store | Shutdown rejection is published to every caller; a fresh store has independent startup state. | | Test failure | Defers release contexts, stores and task services; leak/FD checks run after the resources close. |
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep-reviewed the complete four-file diff and PR history at exact head 0ff2ff5f7ed7e8c653e73e727d8f9ba05db16deb (base/merge-base 7013fbbd6e152ba31da2d3dbba7012ac7f8b8462). No blocking issue found.
The planner now rejects both freshly selected and inherited lossy eight-byte string range distributions specifically for CHAR/VARCHAR/TEXT aggregate keys, while retaining reusable full-key hash distributions. I traced the selected group-key identity, reset/fallback path, aggregate-child and join-child reuse, multi-key/grouping-set eligibility, and all range-strategy writers; numeric aggregate ranges and join/scan range behavior remain outside the guard. The stats-composition test also preserves fresh string range metadata for non-aggregate consumers.
Validation at the exact head: git diff --check; CGo/third-party build; ./.agents/skills/mo-dev/scripts/mo-cgo-test ./pkg/sql/plan ./pkg/vm/engine/disttae -count=1; and focused -race runs for both new planner regressions. Required CI is green. The documented possibility that a conservative hash fallback may subsequently choose no shuffle for some well-distributed short-string workloads is a performance tradeoff to measure, not a correctness or merge blocker.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed current head 0ff2ff5 against main 7013fbb. No concrete blocker found. COMMENT only for this self-authored PR.
The complete four-file change remains identical to my prior reviewed head 2a484f1; intervening commits merge main. I re-read the full current diff, all discussion/reviews, and current planner/compiler consumers, including the newly merged standalone DISTINCT-aggregate rewrite interaction. Both fresh and inherited CHAR/VARCHAR/TEXT aggregate range choices are rejected, while reset clears range/reuse state before fallback and full-key hash reuse remains eligible. Selected group-key identity, grouping-set/DISTINCT topology, remap fallbacks, and compiler key/type construction remain consistent. Numeric aggregate ranges, join/scan range decisions and fresh string range metadata are not disabled.
This prevents known prefix-induced skew; eight-byte prefix collisions alone are not evidence of incorrect aggregate results. Equality, collation handling, NULL/empty/long-key representation, aggregate ordering/state, wire and persistent formats are unchanged. There is no added runtime resource owner or synchronization. The existing hash cost threshold can choose no shuffle, so some well-distributed short-string workloads can lose parallel range aggregation; that conservative performance tradeoff is documented and is not a new blocker. Prior suggestions for CHAR/TEXT/hash-reuse/numeric controls and a short-string benchmark remain optional, not mandatory evidence failures.
Evidence: full exact-head source/history inspection and prior applicable review reused. An external review records successful exact-head CGo package tests for plan/disttae and focused race runs of the two planner regressions; I reused that report rather than rerunning native tests. I did not independently execute SQL, UT/BVT, distributed performance or ClickBench measurements, and did not wait for CI. Head/base were rechecked immediately before submission.
4 participants
Problem
ClickBench Q34 on the 55 machine could select
shuffle: range(hits.url)for a high-cardinality stringGROUP BY.ShuffleRange.UpdateStringand the executor encode only the first eight bytes, while grouping equality uses the complete URL. The full URL had 1,853,441 distinct values but only 51 distinct first-eight-byte prefixes; one prefix owned 1.69M rows. The resulting range plan took about 15.1s on the same NVMe data, with roughly 358MiB spill and 215s cumulative scan wait. The no-group-range plan was about 1.75s.Change
CHAR/VARCHAR/TEXTaggregate keys when selecting a range strategy; the existing reset hash/no-shuffle cost decision remains in control.This is a conservative, reusable guard for any string aggregate with long common prefixes. A future full-key quantile sketch can explicitly re-enable range ownership.
Validation
./.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=600s ./pkg/sql/plan ./pkg/vm/engine/disttaego vet -mod=readonly ./pkg/sql/plan ./pkg/vm/engine/disttaego build -mod=readonly ./pkg/sql/plan ./pkg/vm/engine/disttaegit diff --checkandgofmt -d