[Feature](func) Support AggFn percentile_approx_array - #65847
Merged
Conversation
linrrzqqq
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 21, 2026 08:12
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
/review |
Collaborator
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: request changes for one performance-path issue. I did not find a data-correctness defect in the new percentile calculation, nullable execution, or distributed state handling.
Critical checkpoint conclusions:
- Goal and proof: the FE/BE implementation adds
percentile_approx_arraywith optional compression and returns multiple requested levels from one TDigest. The regression and BE tests cover coercion, unsorted/duplicate/boundary levels, empty/all-null inputs, grouped results, serialization/merge, invalid levels, and compression normalization; test execution was not independently run in this review environment. - Scope: the change is focused on function registration, expression binding, TDigest batch lookup, aggregate state, and directly related tests.
- Concurrency: no shared mutable state or cross-thread ownership is introduced; aggregate states are query/pipeline-owned and phase-sequenced.
- Lifecycle: state vectors and the digest use RAII, and reset/read/merge preserve the uninitialized, initialized-empty, and initialized-populated invariants.
- Configuration: no configuration item is added or changed.
- Compatibility: no existing storage or wire format is changed; this is a new function/state format, and FE/BE arities, physical types, and result nullability agree.
- Parallel and conditional paths: row-wise, grouped, no-key batch, range, streaming, nullable-wrapper, empty/all-null, state/combinator, and merge paths were traced. Constant materialization and array offsets remain row-aligned; incompatible partial states fail before mutation.
- Test results: the checked-in regression output is consistent with the TDigest implementation and preserves requested level order. Negative cases use the required error-test form, and grouped output is ordered.
- Observability: no additional logs or metrics are needed for this pure aggregate function; existing analysis/runtime errors identify invalid quantiles and incompatible states.
- Transactions, persistence, and data writes: not applicable; the patch changes query-time expression and aggregate execution only.
- FE-BE propagation: no new thrift variable is introduced; registration, signatures, explicit coercion, outer non-nullability, nullable array elements, and empty-input behavior match across FE and BE.
- Performance: the one accepted inline issue asks that release-mode checks be removed from the per-quantile loop in accordance with the repository hot-path rule. The single-digest design and monotonic cumulative cursor otherwise implement the advertised optimization.
- Other risks: no additional substantiated issue remained after the full changed-file and upstream/downstream scans.
User focus: no additional user-provided focus was supplied; the full PR was reviewed.
Validation: static review of the authoritative PR diff and related code only. Per the runner review instructions, no local build or test was attempted.
Contributor
TPC-H: Total hot run time: 29599 ms |
Contributor
TPC-DS: Total hot run time: 176303 ms |
Contributor
ClickBench: Total hot run time: 25.03 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
linrrzqqq
force-pushed
the
percentile-approx-array
branch
from
July 22, 2026 03:09
9a8a351 to
4877373
Compare
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review conclusion
Changes requested. The core two- and three-argument function is wired end to end, but three parallel/correctness paths remain broken:
- batched TDigest left-tail rounding can differ from equivalent scalar calls;
- automatic
_foreachdiscovery exposes an unusable callable form; _merge/_unionfail after an aggregate state crosses a slot boundary.
Critical checkpoint conclusions
- Goal and scope: FE registration/signatures, visitor dispatch, BE factory/state, and the regression/BE tests implement the intended single-digest array API. The patch is otherwise focused on that feature.
- Correctness and lifecycle: I traced row, grouped, selected, no-key batch, range/window, nullable/all-null, empty-level, reset, serialization/deserialization/merge, output offsets, and nested null maps. Ownership and state lifecycles are sound; the three inline issues are the remaining correctness failures.
- Concurrency: No new shared concurrency, lock-ordering, or static-lifecycle concern is introduced; state is aggregate-instance local.
- Configuration: No configuration item is added or changed.
- Compatibility and parallel paths: FE/BE physical and result types agree, and the new serialized state is self-contained. No existing persisted or wire format is changed. The dynamically synthesized
_foreachand projected/stored_merge/_unionpaths need the inline fixes. - Errors and tests: Finite/range validation and incompatible-state failures are explicit. The added deterministic regression and BE unit tests cover the main, negative, empty, and serialize/merge paths, but need the three focused cases called out inline.
- Observability: Existing analysis/runtime errors are sufficient for this pure aggregate function; no new logging or metrics are required.
- Transactions, persistence, and data writes: Not applicable; this patch changes query-time expression and aggregate execution only.
- FE-BE propagation: No new thrift/session variable is introduced. Function arities, coercion, result/item nullability, and empty-input behavior otherwise align across FE and BE.
- Performance: One digest plus one sorted forward scan is the intended optimization, and the earlier release-check hot-loop concern is fixed on the current head. The left-tail branch still needs to preserve scalar rounding.
- User focus: No additional user-provided focus was supplied; the full PR was reviewed.
Validation was static per the review-runner contract; no local build or test was run.
Collaborator
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29211 ms |
Contributor
TPC-DS: Total hot run time: 177985 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
Contributor
There was a problem hiding this comment.
Review conclusion
Changes requested. The core two- and three-argument percentile_approx_array path is wired through FE, BE, scalar-equivalence unit tests, and regression output, but the synthesized _state path has one additional P2 correctness gap: a null value lets an invalid quantile bypass both FE post-rewrite validation and BE state initialization, producing a serialized empty state. The inline comment gives a concrete trigger and fix direction.
Critical checkpoints
- Goal, correctness, and scope: The change is focused and the direct aggregate substantially achieves its goal. FE/BE signatures, array nullability, empty/all-null results, registration, visitor dispatch, serialization, reset, result insertion, and both merge orders were traced. The
_statelegality bypass prevents the feature from being complete. - Concurrency and lifecycle: No new shared-thread, lock, static-initialization, or concurrently owned lifecycle is introduced. Aggregate state ownership is query/group-local; no additional lifetime or resource defect was found.
- Configuration, compatibility, and cross-module contracts: No configuration, Thrift field, or other transmitted variable is added. FE and BE types agree, and the new state format was checked for uninitialized, empty, populated, and merged states. No separate mixed-version defect was substantiated.
- Parallel paths and conditions: Direct, constant-folded, DISTINCT, window,
_state,_merge,_union, and_foreachexposures were reviewed. Existing threads already cover_foreach, projected/stored_merge/_union, and batched left-tail precision; they are not duplicated here. DISTINCT analytic syntax is parser-rejected for this function, and direct DISTINCT uses planner deduplication. - Tests and results: The added BE and regression coverage exercises direct, grouped, null, empty, invalid, non-finite, serialization, and scalar-equivalence cases, and the checked-in results are consistent with those queries. It lacks the accepted
_statenull-plus-invalid-quantile case. Per the review contract, no local build or test was run. Current CI shows compile, CheckStyle, clang-format, macOS BE UT, vault regression, and performance passing; Linux BE UT, FE UT, and several regression suites remain pending. - Observability, transactions, persistence, and writes: No new logging/metrics need, transaction protocol, catalog persistence, or storage-write path is introduced.
- Performance: The batched sorted traversal avoids repeated scalar searches, and no new CPU/memory issue was found beyond the already-known left-tail precision thread. The prior hot-loop check issue is fixed at the current head.
- User focus: No additional user-provided focus was specified; the complete PR was reviewed.
Two normal full-review passes and a separate risk-focused pass converged on this frozen comment set with no further valuable findings.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Mryange
previously approved these changes
Jul 29, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
HappenLee
reviewed
Jul 31, 2026
HappenLee
reviewed
Jul 31, 2026
HappenLee
reviewed
Jul 31, 2026
github-actions
Bot
removed
the
approved
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29148 ms |
Contributor
TPC-DS: Total hot run time: 169751 ms |
Contributor
ClickBench: Total hot run time: 23.88 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
924060929
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release note
doc: apache/doris-website#4016
Support function
PERCENTILE_APPROX_ARRAYperformance: