Skip to content

[fix](be) Keep all samples when merging percentile states - #68318

Draft
mrhhsg wants to merge 1 commit into
apache:masterfrom
mrhhsg:fix/percentile-state-merge-lost-samples
Draft

mrhhsg wants to merge 1 commit into
apache:masterfrom
mrhhsg:fix/percentile-state-merge-lost-samples

Conversation

@mrhhsg

@mrhhsg mrhhsg commented Sep 21, 2026

Copy link
Copy Markdown
Member

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

https://claude.ai/code/session_01Afs6xcnnuT7cUpnkiLEJ11

### 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
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@mrhhsg

mrhhsg commented Sep 21, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions github-actions Bot 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.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants