Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: `Counts::merge` (the sample container behind `percentile`,
`percentile_array` and `PERCENTILE_STATE/UNION/MERGE`) only moved the raw
`_nums` of the source state into `_sorted_nums_vec`. Two in-memory merges
were therefore wrong:
1. The source state was itself a merged state: its samples live only in
`_sorted_nums_vec`, `_nums` is empty, so every sample was dropped.
2. The destination still held raw samples in `_nums`: `terminate` with a
single sorted run overwrote `_nums` with that run, the multi-run path
ignored `_nums`, `serialize` ignored `_sorted_nums_vec` when `_nums` was
non-empty, and unsorted raw samples could be pushed as a "sorted" run.
Bucketed hash aggregation merges per-instance hash-table states directly in
the source operator, which hits both cases. For example, with rows
`(shard, v) = (0,0),(1,0),(1,10)`, `agg_phase=1`,
`enable_bucketed_hash_agg=true` and `parallel_pipeline_task_num=4`,
SELECT PERCENTILE_MERGE(s) FROM (
SELECT shard, PERCENTILE_UNION(PERCENTILE_STATE(v, 0.625)) s
FROM t GROUP BY shard) q;
returned 0 or 6.25 non-deterministically instead of 2.5. Plain
`percentile(v, q) ... GROUP BY` under bucketed aggregation lost samples too.
Fix: `merge` now moves the raw samples of both sides (sorted if needed) into
`_sorted_nums_vec` and takes over all sorted runs of the source.
`serialize` and `terminate` fold any remaining raw samples into the sorted
runs before merging them. The serialized format is unchanged.
### Release note
Fix wrong and non-deterministic results of percentile / percentile_array /
PERCENTILE_UNION when states are merged by bucketed hash aggregation.
### Check List (For Author)
- Test:
- Unit Test: PercentileUtilTest (3 new cases fail before the fix, all 16 pass after)
- Regression test: query_p0/aggregate/percentile_bucketed_agg_merge (fails
3/3 runs on an unfixed BE, passes 3/3 with the fix; results match the
non-bucketed controls)
- Manual test: the reproduction above returns 2.5 in 8/8 runs
- Behavior changed: Yes (percentile results under bucketed hash aggregation are now correct)
- Does this need documentation: No
Claude-Session: https://claude.ai/code/session_01Afs6xcnnuT7cUpnkiLEJ11
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated static review opinion: no blocking issues found.
- Goal and correctness: The change fixes both loss modes in
Counts::merge: a source already represented by sorted runs and a destination that still has raw samples. All reachable raw/run/mixed operation orders preserve every sample and the exact-percentile interpolation result. - Scope and design: The four-path change is focused on the state container and direct unit/regression coverage; no unrelated production behavior was changed.
- Concurrency and lifecycle: Bucketed aggregation publishes finished sink states with release/acquire synchronization and serializes each bucket with its CAS lock. Merged RHS states are distinct, consumed, and destroyed; generic paths use fresh temporary states. No alias, reuse, race, or deadlock path was found.
- Compatibility and parallel paths: The serialized layout remains a size followed by contiguous sorted values. Scalar/array percentile, aggregate-state union/merge, bucketed direct merge, nullable/combinator delegation, and ordinary serialized merge paths were traced.
- Tests: The added unit cases cover merged, raw, empty-source, boundary, and mixed serialization shapes. The regression forces and asserts bucketed aggregation, covers both state and raw percentile paths, validates the expected values, and compares non-bucketed controls.
- Performance, memory, and errors: Sorting raw runs before k-way merge is required for correctness and does not worsen the exact-percentile asymptotic cost. Transfers use moves; no new large untracked allocation, ignored status, or silent error path was introduced.
- Other checkpoints: No configuration, persistence/transaction, FE-BE protocol, storage-format, security-boundary, observability, static-initialization, or glibc-baseline change applies.
- User focus: No additional user-provided focus was specified.
Per the review-runner contract, this was a static review only: I did not build or run tests. The author-reported unit, regression, and manual results were inspected but not independently executed.
Reviewed commit: 10915037d22862bbe6c867c6146ea1765bade531.
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
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.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Counts::merge(the sample container behindpercentile,percentile_arrayandPERCENTILE_STATE/UNION/MERGE) only moved the raw_numsof the source state into_sorted_nums_vec. Two in-memory mergeswere therefore wrong:
_sorted_nums_vec,_numsis empty, so every sample was dropped._nums:terminatewith asingle sorted run overwrote
_numswith that run, the multi-run pathignored
_nums,serializeignored_sorted_nums_vecwhen_numswasnon-empty, and unsorted raw samples could be pushed as a "sorted" run.
Bucketed hash aggregation merges per-instance hash-table states directly in
the source operator, which hits both cases. For example, with rows
(shard, v) = (0,0),(1,0),(1,10),agg_phase=1,enable_bucketed_hash_agg=trueandparallel_pipeline_task_num=4,returned 0 or 6.25 non-deterministically instead of 2.5. Plain
percentile(v, q) ... GROUP BYunder bucketed aggregation lost samples too.Fix:
mergenow moves the raw samples of both sides (sorted if needed) into_sorted_nums_vecand takes over all sorted runs of the source.serializeandterminatefold any remaining raw samples into the sortedruns before merging them. The serialized format is unchanged.
Release note
Fix wrong and non-deterministic results of percentile / percentile_array /
PERCENTILE_UNION when states are merged by bucketed hash aggregation.
Check List (For Author)
3/3 runs on an unfixed BE, passes 3/3 with the fix; results match the
non-bucketed controls)
https://claude.ai/code/session_01Afs6xcnnuT7cUpnkiLEJ11