feat(aggexec): add approx_percentile aggregate function for p95/p99#25881
feat(aggexec): add approx_percentile aggregate function for p95/p99#25881VioletQwQ-0 wants to merge 30 commits into
Conversation
The expected result had column `a` typed as a[8,54,0] (int64), but the CI environment maps INT to a[4,32,0] (int32). Regenerated the .result to match the actual CI output.
Resolve conflicts in function_id.go and function_id_test.go: - Keep all new function IDs from main (GET_LOCK, S2, H3, ST_POINT, CAST_STRICT) - Place APPROX_PERCENTILE=542 before FUNCTION_END_NUMBER=543
…centile-quantile-agg # Conflicts: # pkg/sql/plan/function/function_id_test.go
…centile-quantile-agg # Conflicts: # pkg/sql/plan/function/function_id_test.go
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No actionable regressions found. Prior blockers are resolved at the current head; aggregate/window ID dispatch, bounded mpool-backed sketch lifecycle, merge/serialization, numeric and DECIMAL interpolation, constant-percentile validation, and GROUP/WINDOW/TIME_WINDOW integration are coherent. Current-head unit, BVT, multi-CN, SCA, and coverage CI checks pass.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed the current head independently. The sketch state is bounded and its retained value buffers are mpool-backed, with free/reset/unmarshal error ownership closed. Integer interpolation preserves endpoint precision with exact big-integer arithmetic, decimal return scale is capped at precision 38 and interpolation applies the actual scale delta, and aggregate configuration is propagated through group/window/time-window execution. I found no remaining merge-blocking correctness or lifecycle issue.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 6bbd7d7 after the main merge. No blocking findings.
The prior blockers are closed: retained KLL sample and scratch buffers are mpool-backed and released across grow, merge, decode failure, restore, spill, and final cleanup; integer and DECIMAL interpolation preserve endpoint precision and the DECIMAL(38,s) return contract; all aggregate/window RegisterXXX runtime registration has been removed and the immutable catalog-to-executor ID map is pinned by tests. The only semantic merge conflict was in TIME_WINDOW construction, and its resolution correctly combines the current layout/AggIdx logic with the shared percentile configuration path. GROUP, WINDOW, TIME_WINDOW, and remote pipeline config propagation remain coherent.
Fresh validation on this head:
- full tests for aggexec, group, window, timewin, compile, plan, and plan/function: PASS
- focused lifecycle, partial-merge, decimal, TIME_WINDOW, binder, and ID tests under race, count 10: PASS
- build and vet for all seven packages: PASS
- merge-tree against current origin/main and diff-check: clean
- required CI: green
XuPeng-SH
left a comment
There was a problem hiding this comment.
One lifecycle correctness blocker remains in the generic aggregate-state unmarshal path. The approx_percentile implementation, registry cleanup, numeric/DECIMAL behavior, configuration propagation, bounded state, and current CI otherwise look good. Local related-package tests, race tests, build, and vet pass; the focused reuse regression below fails on this head.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 8cbe5feb459639af642b31abb29c617d9ad10b70. The generic aggregate-state reuse blocker is fixed: UnmarshalFromReader now drops the freed outer chunk metadata before decoding, so an empty replacement has zero groups and multi-chunk replacement cannot append through stale freed state.
Independent lifecycle validation covered the PR regression 20x; real freeable approx_percentile sketch reuse from populated -> empty, populated -> two chunks with live mobs across the chunk boundary, and malformed replacement -> clean empty state/mpool zero 10x; the combined matrix under -race 10x; full aggexec tests and full-package race; related batch/timewin/compile/plan/function tests and vet; git diff --check; and a conflict-free merge tree against current origin/main. The previously reviewed bounded-state/mpool accounting, config propagation, numeric/DECIMAL correctness, and registration cleanup remain intact.
Current-head GitHub CI has no failures at review time, though several jobs are still in progress; this approval is based on the exact-head code and independent validation above.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No blocking regression found. The author’s latest lifecycle explanation is supported by the exact-head code, and current-head CI is green. One helper-only DECIMAL inconsistency remains non-blocking because the production SQL/executor path does not call it.
P2 - Apply the capped decimal scale rule to the direct helper (pkg/sql/colexec/aggexec/approx_percentile.go:1034)
The exported PercentileDecimal128 helper still unconditionally passes scaleDelta=1 and ignores argScale/the vector’s precision. For a DECIMAL(38,0) vector containing 99999999999999999999999999999999999999, even p=0 multiplies the unscaled endpoint by 10 and returns decimal interpolation overflow—the same max-precision defect already fixed in the executor path. There are no production callers in this repository, so this is non-blocking, but the direct-caller contract and regression coverage should be aligned with ApproxPercentileReturnType.
|
@fengttt The aggregate/window registration layer was removed in f935280. |
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Reviewed exact head 8cbe5fe and refreshed all PR discussion. The author’s latest registration-cleanup statement is supported: runtime RegisterXXX machinery is gone and all 38 fixed executor IDs are pinned against catalog IDs. The aggregate-state reuse fix is correct, current-head CI is green, and no new actionable regression remains. Local focused tests could not start because the worktree lacks prebuilt cgo/libmo.dylib; the worktree remains clean.
|
Fixed the direct DECIMAL128 helper inconsistency in
Added direct-helper regressions for:
Validation on the complete final diff:
@gouhongshen @fengttt please re-review the current head. |
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Reviewed exact head 12c0bcd against origin/main after loading all existing reviews, issue comments, inline discussions, and the latest author reply. The new DECIMAL128 helper fix is consistent with aggregate return-type handling and has targeted boundary tests. Previously reported correctness, memory-accounting, registration, and state-reuse issues are resolved. Exact-head CI is green. Focused local tests could not start because the required prebuilt cgo/libmo.dylib is absent; no dependencies were modified.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
One blocking partial-aggregation merge panic remains; the previously reviewed lifecycle and DECIMAL issues are addressed.
P1 - Avoid indexing a missing parity level during partial sketch merge (pkg/sql/colexec/aggexec/approx_percentile.go:115)
A 400-row source sketch compacts level 0 to an empty slice and toggles other.parity[0]. When it is merged into a newly created destination group, appendValues(0, empty) returns before ensureLevel (lines 148-152), so s.parity is still empty and line 116 panics. This occurs in distributed/partial aggregation whenever the merge-side group has no local values. The merge tests populate the destination before merging, so they do not cover this shape.
|
Addressed the current-head partial-merge panic in
Validation:
|
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The upstream merge preserves the prior correctness fixes and resolves the new function-ID collision consistently. No blocking regression remains; one non-blocking memory-efficiency issue persists.
P2 - Allocate sketches lazily for groups containing values (pkg/sql/colexec/aggexec/approx_percentile.go:570)
GroupGrow eagerly creates a quantileSketch for every group, including high-cardinality groups whose aggregate argument is always NULL. Fill, BulkFill, BatchFill, and merge already allocate lazily, while Flush explicitly handles nil state, so these empty sketches are unnecessary. Their Go-managed objects and slice headers are also excluded from the group spill calculation, which only reads ctr.mp.CurrNB(); consequently an all-NULL high-cardinality aggregation can accumulate substantial untracked heap memory. Remove this loop and retain ensureSketch at the existing value/merge sites.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Prior blocking review findings are resolved at fa762ce. One non-blocking memory-efficiency issue remains.
P2 - Allocate sketches lazily for NULL-only groups (pkg/sql/colexec/aggexec/approx_percentile.go:570)
GroupGrow eagerly creates a quantileSketch for every group. Fill/BatchFill skip NULL inputs before ensureSketch, merge skips nil source state, and Flush already emits NULL for a nil sketch, so groups whose value is always NULL never need one. These per-group Go heap objects and slice headers are outside mpool accounting; high-cardinality NULL-only aggregation therefore adds avoidable untracked heap pressure. Remove this eager loop and retain the existing lazy creation at value/merge sites.
…7-approx-percentile # Conflicts: # pkg/sql/plan/function/function_id.go # pkg/sql/plan/function/function_id_test.go
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Prior blockers for mpool lifecycle, configuration propagation, numeric/DECIMAL precision, registration removal, state replacement, and partial merges are fixed. The prior lazy NULL-only sketch allocation remains unchanged and non-blocking. A non-finite float endpoint failure is blocking.
P1 - Return extrema for infinite float inputs (pkg/sql/colexec/aggexec/approx_percentile.go:925)
Author response: non-finite inputs were intentionally left with the prior IEEE-754 propagation behavior. Why this remains blocking: non-finite float values are not excluded—CAST('Inf' AS DOUBLE) is accepted by the float cast path—and this aggregate accepts float64. For approx_percentile(CAST('Inf' AS DOUBLE), 1), Quantile returns lo=hi=+Inf with zero fraction, but interpolateFloat64 checks for Inf before the zero-fraction case and evaluates +Inf + (+Inf - +Inf) * 0, producing NaN. Thus p=1 fails to return the maximum; p=0 and singleton inputs have the same defect. Move the zero-fraction/equal-endpoint handling before the non-finite branch, and add executor/SQL coverage for +/-Inf endpoints and singleton inputs.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24550
What this PR does / why we need it:
This PR continues the unfinished work from #24667 while preserving its original commits and authorship.
It adds approx_percentile(value, percentile) with:
Review-blocker regressions cover 10K rows, duplicate values, NULLs, four-way partial merge, intermediate-state round trips, replacement of reused executors with empty and multi-chunk state, state-size bounds, mismatched merge configuration, and DECIMAL64/128 values above 2^53.
Validation