Skip to content

feat: support prepared and sortable discrete percentiles - #28540

Open
daviszhen wants to merge 28 commits into
matrixorigin:mainfrom
daviszhen:codex/issue-25110-p1
Open

feat: support prepared and sortable discrete percentiles#28540
daviszhen wants to merge 28 commits into
matrixorigin:mainfrom
daviszhen:codex/issue-25110-p1

Conversation

@daviszhen

@daviszhen daviszhen commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

Follow-up for issue #25110.

What this PR does / why we need it:

This completes the two highest-priority ordered-set percentile follow-ups discussed in #25110:

  • allows a prepared-statement parameter for p in APPROX_PERCENTILE, PERCENTILE_CONT, and PERCENTILE_DISC; the parameter is fixed per EXECUTE, while column/per-row percentile expressions remain rejected;
  • prevents stale percentile reuse by rebuilding the physical compile for parameterized percentile aggregates on every execution;
  • extends PERCENTILE_DISC from numeric-only input to engine SQL-sortable types, including VARCHAR, DATE, UUID, and DECIMAL256, while retaining the numeric interpolation restrictions of PERCENTILE_CONT;
  • retains variable-width discrete inputs and string provenance in the accounted aggregate argument arena and supports unambiguous intermediate-state serialization and merge;
  • adds planner, compiler, executor, frontend cache, and distributed protocol regression coverage.

Ordered-set window functions remain outside this PR.

Compatibility and merge dependency

  • Extended discrete-percentile remote execution is admitted at MORPC v66.
  • Current main owns v63 for INSERT IGNORE CHECK warning diagnostics and v64 for typed BIN/CONV execution contracts.
  • PR fix: widen decimal SUM accumulation #28624 owns v65 for widened DECIMAL SUM partial state; its full implementation is integrated in this head.
  • This PR depends on fix: widen decimal SUM accumulation #28624 and must merge after it. This head includes fix: widen decimal SUM accumulation #28624 through commit 44c60bd, so a v66 binary implements the complete monotonic protocol prefix.
  • Exact design revision for approval: e743b6dcdd, in docs/design/20260910-prepared-and-discrete-percentiles.md.

Validation

  • make build
  • go test ./pkg/frontend ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/colexec/aggexec ./pkg/sql/colexec/group ./pkg/sql/compile -count=1
  • local MatrixOne SQL validation for two executions of the same prepared percentile statement with different p values, prepared PERCENTILE_CONT/APPROX_PERCENTILE, and VARCHAR/DATE/DECIMAL256 PERCENTILE_DISC

@qodo-code-review

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor Author

Addressed the remaining design-first review gate and rebased onto current main.

Exact design revision: 30d79f9463

The new stable, date-versioned design covers the complete PR contract:

  • prepared-percentile invariants and the PREPARE/EXECUTE cache state machine, including repeated values and invalid-then-valid execution;
  • PERCENTILE_DISC supported type families, per-family ordering, rank, NULL/tie, return-type, and selected string-provenance semantics;
  • orderedPercentileDiscreteExec ownership, allocation accounting, group spill behavior, and per-group O(N) flush bounds;
  • exact aggregate config and retained-argument wire layouts, metadata envelope, merge invariants, and fallback/error behavior;
  • mixed-version rollout, downgrade, and rollback procedures;
  • alternatives, decision record, and a contract-to-test validation map.

The original APPROX_PERCENTILE design now links to this extension and is explicitly scoped to PR #27864.

Rebase compatibility fix: current main already assigns MORPCVersion60 to row-dependent expression defaults, so this PR now uses MORPCVersion61. The protocol test verifies v60 rejection, v61 admission, and the unchanged v17 numeric boundary.

Additional regression coverage executes one prepared statement with p=0.25, p=0.75, invalid p=1.5, and then valid p=0.5 to prove clean invalid-to-valid reuse.

Verification on rebased head:

  • make build
  • go test ./pkg/sql/colexec/aggexec ./pkg/sql/plan/function ./pkg/sql/plan ./pkg/sql/compile ./pkg/frontend ./pkg/queryservice/client -count=1
  • git diff --check upstream/main...HEAD

All passed locally. Please review and record design approval against exact revision 30d79f9 before implementation approval proceeds.

@aptend aptend 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.

Deep re-review on exact head 30d79f9 against merge base 9a07655. I read the complete review/comment history and author follow-ups, checked the increment from my prior reviewed head 18cdf7c, and reviewed the new exact design revision.

The three earlier implementation blockers have source-level fixes, but the proposed design is not yet approvable because its retained-string encoding is ambiguous.

P1 — untagged string data can be decoded as the metadata header and silently corrupted.

Locations: docs/design/20260910-prepared-and-discrete-percentiles.md:218-234 and pkg/sql/colexec/aggexec/aggState.go:1194-1217.

The design reserves f1 53 4d only for rows with non-default metadata while storing default-metadata rows as raw, unescaped bytes. Therefore the decoder cannot distinguish an ordinary VARCHAR/VARBINARY value beginning with that prefix from an envelope. If bytes 3-5 happen to be version 1 and valid domain/source enum values, it strips the first six user bytes; other combinations can turn valid user data into an execution error.

Deterministic counterexample on this exact head: a single VARBINARY input {f1 53 4d 01 00 00 78} with inherited domain/expression source, PERCENTILE_DISC(0.5). A temporary executor test expected the same seven bytes but Flush returned only {78}. This affects local execution before any wire round trip and can change both sort order and the selected result.

Please make every retained string payload unambiguous (for example, explicitly tag both raw and metadata-bearing variants, or use a framing/escaping scheme outside the raw-value namespace), update the exact design revision and compatibility contract, and add regressions for valid and invalid magic-prefix raw values through local selection, merge, and intermediate serialization. Because this protocol-bearing feature is above the design gate and the exact design still has this correctness blocker, implementation approval must wait for a corrected, traceably approved revision.

Validation performed: make thirdparties && make -C cgo; the review-only TestOrderedPercentileDiscreteUntaggedMagicPrefix reproduced the corruption and was removed afterward; git diff --check passed.

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

Copy link
Copy Markdown
Contributor Author

已修复本轮 multi-CN BVT 失败,提交:62b014470f

原因:prepared percentile 的运行时参数非法时,错误在 aggregate operator 构造阶段通过 panic 返回;这会把 Go stack 拼进 SQL 错误,并且 child scopes 已先构造,导致后续出现 missing free for type: *compile.Scope 和 CN 重启/级联失败。

修复:

  • 在编译 aggregate child scopes 前预检包含 ParamRef 的 percentile_cont/disc 配置;
  • 非法参数通过普通 error 返回,不再携带 panic stack,也不会留下部分构造的 scopes;
  • 提取并复用 percentile config 构造逻辑,保持实际 operator 配置行为一致;
  • 补充单元测试和设计文档。

验证:

  • go test ./pkg/sql/compile -run Test(ConstructAggregateConfig|PreflightOrderedPercentile) -count=1 -v:通过
  • go test ./pkg/sql/compile -count=1:通过
  • make build:通过
  • 隔离 Docker Compose 多 CN 环境 + 最新 mo-tester 334d3e124b,执行 func_aggr_ordered_set.test:38/38 通过
  • 日志中无 panic in compile、无 missing free for type,CN restart count 均为 0

新的 CI 已触发:MatrixOne ALL CI run 34483764964。

@daviszhen

Copy link
Copy Markdown
Contributor Author

Updated the branch in c4d7313:\n\n- Merged current upstream/main (82e3a53), which removes the PR conflict.\n- Resolved the MORPC version collision: main now owns v61 for index-metadata provenance, so extended PERCENTILE_DISC remote admission is moved to v62. The boundary test now verifies v61 rejection and v62 admission, and the rollout/design documentation is aligned.\n- Preserved main's stable query-service protocol tests.\n\nThe prior Ubuntu/x86 unit-test failure in run 34483764964 was the job-wide timeout (exit 124); both engine shards had completed successfully and no test assertion failed.\n\nLocal verification:\n- make build\n- go test ./pkg/sql/colexec/aggexec ./pkg/sql/plan/function ./pkg/sql/plan ./pkg/sql/compile ./pkg/frontend ./pkg/queryservice/client -count=1\n- golangci-lint v2.6.2 on the conflict-affected packages: 0 issues\n\nNew CI run: https://github.com/matrixorigin/matrixone/actions/runs/34502230697

@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.

Design/compatibility review at c4d7313 against base 82e3a53.

[P1] v62 no longer uniquely identifies extended PERCENTILE_DISC support. Current upstream main already assigns MORPCVersion62 to VARCHAR OCT overloads. This PR's new aggregate gate (pkg/sql/compile/remoterun.go:1884 onward) admits extended types at >=62, but an OCT-only v62 worker cannot instantiate the new VARCHAR/DATE/UUID/Decimal256 executor or decode its retained string payload. Merge current main and choose a fresh capability coordinated with other pending protocol changes; update code, boundary tests, and rollout design together.

The previous ambiguous magic-prefix encoding has been corrected in the design and source: every retained string now has an envelope and missing/invalid envelopes are rejected. I am not repeating that resolved finding.

The exact design still needs traceable approval: docs/design/20260910-prepared-and-discrete-percentiles.md is proposed, the latest design review rejected the older encoding, and I found no subsequent approval of the corrected revision. The new collision also invalidates the document's current v62 rollout contract. Update and review the exact revision before implementation approval proceeds.

Scope/evidence: read the complete current 336-line extension design and full approval/comment history; inspected retained framing and remote admission to confirm the fixes and current blocker. This is the design-first gate, not a full implementation sign-off. Green exact-head CI and author validation do not prove compatibility with an already released different v62 capability. No new build or cluster run.

@daviszhen

Copy link
Copy Markdown
Contributor Author

Updated to current upstream/main (cd8e8d9) in 98cd36c.\n\nThe merge introduced another MORPC version collision because main now owns v62 for VARCHAR OCT overload identities. Extended PERCENTILE_DISC remote admission is therefore moved to v63, with the rejection/admission test and rollout documentation updated consistently.\n\nVerification:\n- make build\n- targeted tests: aggexec, plan/function, plan, compile, frontend, queryservice/client\n- golangci-lint v2.6.2 on the conflict-affected packages: 0 issues\n\nThe previous CI run was fully green. New CI: https://github.com/matrixorigin/matrixone/actions/runs/34557984253

@daviszhen

Copy link
Copy Markdown
Contributor Author

Addressed the latest design/compatibility review at exact head c51394f8f8549959af9eb9f6f4b18250eea293c2:

  • merged current upstream main;
  • preserved main's MORPC v63 assignment for INSERT IGNORE CHECK warning diagnostics;
  • coordinated with pending fix: widen decimal SUM accumulation #28624, which owns v64 for widened DECIMAL SUM partial state;
  • moved extended PERCENTILE_DISC admission to v65;
  • updated the fail-closed boundary test to reject v64 and admit v65;
  • updated the exact design's compatibility, rollout, downgrade, rollback, and validation map.

This PR now explicitly depends on #28624 and must merge after it. After #28624 lands, I will merge/rebase the resulting main here before merge so the v65 binary contains the full monotonic capability prefix.

Local validation passed:

  • make build
  • go test ./pkg/sql/colexec/aggexec ./pkg/sql/plan/function ./pkg/sql/plan ./pkg/sql/compile ./pkg/frontend ./pkg/queryservice/client -count=1

Please re-review and provide traceable approval of the exact design revision above if the compatibility contract is acceptable.

@daviszhen

Copy link
Copy Markdown
Contributor Author

Follow-up: upstream main advanced again while validation was running, so I merged the additional four commits through db4edb49d2 and reran the full local validation. The current exact head/design revision is now 857d218d6e6a16edbf5bd7272f6f526c316c8836.

Both the targeted package tests and make build pass on this final head. The v63/v64/v65 coordination and the dependency on #28624 are unchanged.

@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-review at exact head 857d218 against main db4edb4. REQUEST_CHANGES: the documented protocol-prefix dependency is not yet incorporated, and prepared APPROX_PERCENTILE still reaches the panic/partial-scope failure path that was fixed for CONT/DISC (inline findings).

Prior findings: the separate runtime-specialization cache is now excluded/cleared for percentile-marker plans; selected string domain/source survives retained framing, restoration and UnionOne publication; every retained MySQL string has an envelope, so raw magic-prefix values are no longer ambiguous. Extended-type admission is now distinct from existing v62/v63 capabilities. I am not repeating those resolved implementation findings or treating optional extra tests as blockers.

Design-first assessment: reviewed the complete versioned design at this exact revision, including rank/type semantics, per-execution cache transitions, unambiguous retained state, memory ownership, configured-version rollout and alternatives. Its explicit requirement that v65 contain #28624's v64 implementation is sound, but remains unsatisfied by this head; therefore the feature cannot yet receive unconditional design/implementation acceptance as delivered. This is an acknowledged dependency/composition gate, not a claim that #28624 must be reimplemented here. After merging the dependency, recheck the affected protocol/state integration. The final decision-record sentence still says protocol 63 while the normative rollout says 65; align that stale sentence as part of recording final design approval.

Other reviewed closures: exact rational rank is reused for p=0/1 and ASC/DESC; NULL values are excluded and empty groups return NULL; extended inputs retain their complete type and use the existing shared sorter rather than a new comparison relation. Saved values are copied into accounted storage, merge copies ownership, restore/result errors free temporary vectors/selectors, and metadata-bearing state stays query-local. Numeric executor/state paths remain separate. Per-execution compilation and per-group O(N) restoration plus sorting are deliberate costs; no unrelated benchmark claim is inferred.

Evidence/limits: read the full current change, prior reviews/author fixes, exact design, current compile/cache/aggregate/codec consumers, and the live #28624 dependency state. Reused the author's exact-head build/package-test report and prior applicable wire/provenance evidence. The new APPROX counterexample is source-derived, not a claimed executed SQL run. I did not run native tests, a live mixed-version cluster, allocation-failure or cancellation workloads, and did not wait for CI. Rechecked head/base before submission.

Comment thread pkg/defines/const.go Outdated
Comment thread pkg/sql/compile/operator.go

@aptend aptend 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.

Deep re-review of the complete diff, design, review/thread history, author replies, and the increment since my prior reviewed head 30d79f9, at exact head 857d218. The retained-string framing blocker is closed: every saved MySQL string is now enveloped and the magic-prefix local/wire cases are covered. Runtime-specialization exclusion, selected-row provenance, extended-type admission, rank/type/ownership paths, and current green CI were also checked. Two delivery blockers remain inline. The exact design cannot yet be approved as delivered: it requires the v64 dependency that this head does not contain, and its decision record still says protocol 63 while the normative boundary/code say 65.

Comment thread pkg/defines/const.go Outdated
Comment thread pkg/sql/compile/operator.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement kind/feature kind/test-ci size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants