Skip to content

feat(aggexec): add approx_percentile aggregate function for p95/p99#25881

Open
VioletQwQ-0 wants to merge 30 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/takeover-24667-approx-percentile
Open

feat(aggexec): add approx_percentile aggregate function for p95/p99#25881
VioletQwQ-0 wants to merge 30 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/takeover-24667-approx-percentile

Conversation

@VioletQwQ-0

@VioletQwQ-0 VioletQwQ-0 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

What type of PR is this?

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

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:

  • a bounded, mergeable KLL-style sketch instead of retaining every input row;
  • bounded serialized partial state for distributed aggregation;
  • exact rational/integer DECIMAL64 and DECIMAL128 interpolation without float64 conversion;
  • exact min/max results for percentile 0 and 1;
  • planner and executor validation for non-null constant percentiles in [0,1];
  • current main merged with function IDs reconciled.

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

  • go test -count=1 -timeout 180s ./pkg/sql/colexec/aggexec ./pkg/sql/compile ./pkg/sql/plan ./pkg/sql/plan/function
  • go test -race -count=10 -run TestApproxPercentileExec_... ./pkg/sql/colexec/aggexec
  • go vet ./pkg/sql/colexec/aggexec ./pkg/sql/compile ./pkg/sql/plan ./pkg/sql/plan/function
  • go build ./pkg/sql/colexec/aggexec ./pkg/sql/compile ./pkg/sql/plan ./pkg/sql/plan/function
  • git diff upstream/main --check

ULookup and others added 17 commits June 16, 2026 23:15
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
@mergify

mergify Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

⚠️ The sha of the head commit of this PR conflicts with #24667. Mergify cannot evaluate rules on this PR. Once #24667 is merged or closed, Mergify will resume processing this PR. ⚠️

@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Jul 20, 2026
@VioletQwQ-0
VioletQwQ-0 requested review from fengttt July 20, 2026 18:35

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pkg/sql/colexec/aggexec/aggState.go

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

@fengttt The aggregate/window registration layer was removed in f935280.
register.go, all RegisterXXX functions, specialAgg, mutable runtime IDs, and planner init callbacks are gone. Aggregate construction now dispatches directly from fixed catalog IDs, with tests covering the full ID mapping.
The remaining proto.RegisterType is generated protobuf code and unrelated to this registration layer. Please re-review the current head.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Fixed the direct DECIMAL128 helper inconsistency in 12c0bcdd5.

PercentileDecimal128 now reads the input precision from the vector type and combines it with the existing argScale argument, then uses the same ApproxPercentileReturnType rule as the production executor. It no longer unconditionally adds one scale digit when the input is already at precision 38.

Added direct-helper regressions for:

  • DECIMAL(38,0) maximum positive/negative endpoints at p=0 and p=1;
  • DECIMAL(38,38);
  • retained-scale midpoint rounding;
  • DECIMAL width 37 continuing to gain one result scale digit.

Validation on the complete final diff:

  • related package tests for aggexec, timewin, compile, plan, plan/function, group, and window;
  • full aggexec race test plus focused decimal boundary tests under -race -count=10;
  • vet and build for all seven related packages;
  • git diff --check;
  • full preflight manifest and mo-self-review: exact-snapshot PASS, no remaining blocker or pending probe.

@gouhongshen @fengttt please re-review the current head.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Addressed the current-head partial-merge panic in 21026c8a0.

  • quantileSketch.Merge now creates the destination level/parity slot before copying values, including an empty compacted level whose parity bit still carries state.
  • Added a regression that compacts exactly 2*k source rows, verifies level 0 is empty with parity set, and merges into a destination with no local values.

Validation:

  • go test ./pkg/sql/colexec/aggexec -count=1
  • focused go test -race ... -count=10
  • go vet ./pkg/sql/colexec/aggexec
  • git diff --check
  • exact-head preflight PASS on 21026c8a0cb9d5552a9dc77e75a3252c78c9f809

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants