fix: widen decimal SUM accumulation

[daviszhen]

What type of PR is this?

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

Which issue(s) this PR fixes:

Fixes #28585

What this PR does / why we need it:

• Derives DECIMAL SUM precision with aggregate headroom instead of pinning Decimal128 inputs to precision 38.
• Selects a Decimal256 accumulator when the derived SUM result exceeds Decimal128.
• Keeps AVG physical state aligned with its existing executor.
• Versions the widened DECIMAL SUM partial-state layout with MORPC v64.
• Uses the pre-v64 [Decimal128 sum, int64 count] layout on both remote partial and coordinator MergeGroup while a cluster has mixed protocol versions; a new coordinator converts the compatible legacy state to its planned Decimal256 result.
• Covers old-partial/new-merge and new-partial/old-merge wire compatibility for wide Decimal64 and Decimal128, along with overflow, cancellation/order independence, DISTINCT, and return-type behavior.
• Merges the latest main.

Testing

• go test ./pkg/sql/colexec/aggexec ./pkg/sql/colexec/group ./pkg/sql/colexec/window ./pkg/sql/plan/substrait -count=1
• make build

[qodo-code-review]

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 →

[daviszhen]

Updated to current upstream/main (cd8e8d9) and fixed the Ubuntu/x86 UT failure in 9972f41.\n\nRoot cause: the explicitly Decimal128 fast SUM executor could advertise/allocate a Decimal256 state after the public SUM metadata was widened. Flush physically reads Decimal128 values, so the checked CI path panicked with "type mismatch: casting DECIMAL256 vector to []types.Decimal128". The fast executor now keeps its physical state and direct fast-path result consistently Decimal128; production SUM/AVG results requiring Decimal256 continue to be dispatched to newSumAvgDecExec.\n\nVerification:\n- make build\n- go test ./pkg/sql/colexec/aggexec ./pkg/sql/colexec/window ./pkg/sql/plan/substrait -count=1\n- go test -race -tags matrixone_test ./pkg/sql/colexec/aggexec -run '^TestSumDecimal128Fast_Basic$' -count=1\n- golangci-lint v2.6.2 on affected packages: 0 issues\n\nNew CI: https://github.com/matrixorigin/matrixone/actions/runs/34557988105

[XuPeng-SH]

REQUEST_CHANGES at 9972f41 against base cd8e8d9: one concrete mixed-version partial-aggregation compatibility blocker, detailed inline.

Reviewed the full current diff/comments and the widened type dispatcher, Decimal128 fast-state correction, Decimal256 merge/flush/distinct/window paths, and aggregate state transport consumer. Same-version accumulator/result alignment is improved, and keeping explicit Decimal128 fast executors internally Decimal128 resolves the reported test mismatch. The existing generic wider accumulator remains bounded per group; DISTINCT storage and checked final result behavior reuse existing machinery. Substrait fallback for >38-digit sums is explicit in the changed tests, so this is an intentional eligibility/performance tradeoff rather than an unnoticed export success claim.

The blocking issue is the unversioned change from two partial-state vectors to one for an unchanged SUM signature. Source tracing reaches the actual remote MergeGroup decoder; this is not merely a request for optional supplementary testing.

Validation/limits: exact-head source/control-flow review, full change map and focused test-oracle review; no native UT/BVT, mixed-CN deployment, or benchmarks were run by me, and CI was not awaited. Author-reported owning-package/race/SQL checks remain separate evidence. Head/base were rechecked unchanged before submission.

[aptend]

Reviewed exact head 9972f41001f5369b5c0eb8b285456d49964bbc4d against base cd8e8d91343798f565f8ad0f8473cb995b8b361b. I read the complete diff, PR and issue discussion, all reviews and inline threads, and traced SUM result metadata through Decimal128/Decimal256 dispatch, fast and generic accumulation, window/DISTINCT/flush behavior, Substrait fallback, and remote aggregate-state serialization/decoding. Same-version widening closes the reported order-dependent Decimal128 overflow, and the final fast-state correction keeps the remaining fast executors physically consistent. One rolling-upgrade blocker remains.

Validation on the exact head: git diff --check; focused CGo-aware SUM return/overflow, sliding-window, window operator, and Substrait tests passed. Exact-head required CI, Ubuntu UT, coverage, SCA, and standalone/compose BVT checks are terminal-success and were reused. A temporary bidirectional state-wire counterexample using value 1 reproduced both mixed-version failures and was removed afterward.

[aunjgr]

[P1] Version the widened SUM partial-state contract before changing the dispatcher.

At pkg/sql/colexec/aggexec/sumavg2.go:1939-1947, SUM of Decimal64 widths above 16 and Decimal128 now switches from the Decimal128 fast executor to sumAvgDecExec with a Decimal256 state. For the unchanged aggregate ID/input type, the old fast executor serializes [Decimal128 sum, int64 count], while the new generic SUM serializes only [Decimal256 sum] (newSumAvgDecExec:2025-2026). No capability gate or new aggregate identity accompanies this change.

This reaches the actual remote consumer: group/mergeGroup.go constructs local aggregate executors and calls UnmarshalFromReader on received partials; aggState.readStateWithAllocation:775-798 reads as many vectors as the local stateTypes requires. An old sender/new receiver leaves the count vector where the new decoder expects its trailer; the reverse direction attempts to read a nonexistent second vector. Even a single value 1 in SUM(DECIMAL(38,0)) can fail during a rolling upgrade, independent of overflow.

Preserve the old representation below a negotiated boundary, or introduce and fence a new state/aggregate contract before dispatch. Add bidirectional old/new partial-state tests with ordinary value 1 and include grouped/window consumers where applicable.

Reviewed all eight changed files at 9972f41 against cd8e8d9 and traced dispatcher, state construction and remote decoder. Same-version widening and the fast-executor state correction are sound; existing bounded generic/DISTINCT machinery is reused. The changed Substrait test explicitly makes 12 TPC-H queries ineligible due to >38-digit SUM results—an important offload/performance tradeoff, not a claim that all 22 remain exportable.

Reused current-head green CI and aptend's exact-head bidirectional state-wire counterexample; no new native tests or mixed-CN deployment run. This independently confirms the existing compatibility finding.

[daviszhen]

Addressed both rolling-upgrade review comments in f4f46ab304 (fix commit 904726efdc):

• introduced MORPC v64 for the widened DECIMAL SUM partial-state layout;
• kept pre-v64 Group and coordinator MergeGroup on the old [Decimal128 sum, int64 count] wire representation;
• retained new-plan Decimal256 result metadata by widening the compatible legacy accumulator only at final Flush;
• added bidirectional old/new serde tests for wide Decimal64 and Decimal128;
• merged the latest main.

Local verification passed:

• go test ./pkg/sql/colexec/aggexec ./pkg/sql/colexec/group ./pkg/sql/colexec/window ./pkg/sql/plan/substrait -count=1
• make build

Both review threads have been replied to and resolved. CI for the new head is running.