[Opt](func) Improve AggFunc Percentile performance#62520
[Opt](func) Improve AggFunc Percentile performance#62520linrrzqqq wants to merge 1 commit intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
0744d49 to
809d8bc
Compare
|
run buildall |
809d8bc to
a1a7e42
Compare
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Requesting changes because this PR introduces a blocking compatibility regression in the serialized state format for percentile / percentile_array.
Critical checkpoint conclusions:
- Goal of current task: Improve exact percentile aggregate performance. The new code likely improves the hot path by appending raw values and reusing ordered quantile selection, but the change is not safe to merge as-is because it breaks previously serialized aggregate states. The added unit tests cover only the new helper and do not prove compatibility or end-to-end correctness for persisted / exchanged agg states.
- Scope and focus: The implementation is reasonably focused on the percentile aggregate, but it also changes the on-wire / on-disk representation of aggregate states by replacing
Countsserialization withPercentileLevels + raw valuesserialization. - Concurrency: I did not find a new thread-safety issue here. This code runs on per-aggregate state and does not add shared mutable state or locking.
- Lifecycle / static initialization: No special lifecycle or static-init issue found.
- Configuration: No new configuration added.
- Compatibility / storage format: Blocking issue.
PercentileState::write/readnow use a different binary layout, but there is nobe_exec_versiongate, fallback reader, or other compatibility handling.AggStatecolumns persist aggregate bytes and later reconstruct them viadeserialize(), so previously storedpercentilestates can no longer be read correctly. Mixed-version exchange/merge of serialized states is also at risk for the same reason. - Parallel code paths: The exact percentile path was updated consistently for both scalar and array forms; no separate unmodified exact-percentile path stood out.
- Special conditional checks: Quantile validation and null checks are still present and understandable.
- Test coverage: Insufficient for this change. The removed
Countstest is not replaced with tests that exerciseAggregateFunctionPercentileserialization/deserialization, agg-state persistence, or compatibility with old serialized bytes. - Test result changes: No result files involved.
- Observability: No additional observability appears necessary for this perf optimization.
- Transaction / persistence impact: Yes. This affects persisted agg-state bytes used by storage readers and block readers, and the compatibility path is missing.
- Data writes / modifications: No table-write atomicity issue found, but persisted aggregate-state data correctness is affected by the format change above.
- FE/BE variable passing: Not applicable.
- Performance: The intended optimization is reasonable, but compatibility must be preserved.
- Other issues: None beyond the blocking serialization compatibility gap.
Because this is a format-affecting change without backward-compatibility handling or tests, I cannot approve it in the current form.
| counts.serialize(buf); | ||
|
|
||
| levels.write(buf); | ||
| size_t size = values.size(); |
There was a problem hiding this comment.
This changes the serialized layout of PercentileState in a backward-incompatible way.
Before this PR, the state was encoded as:
inited_flag + quantile list + serialized Counts objects
After this PR, it becomes:
inited_flag + quantile list + raw value count + raw values
percentile / percentile_array states are not just transient in-memory objects. They are also persisted and reloaded through AggState (DataTypeAggState -> storage reader -> deserialize()), and can be exchanged between BEs as serialized aggregate states. With the new read() implementation, any previously stored state bytes will interpret the old Counts payload as the new size_t size field and then read the wrong number of bytes into values.
I don't see any be_exec_version bump/gate, dual-format reader, or other compatibility path here, so this looks like a storage / wire-format break rather than a pure internal refactor. Please either preserve the old format, or add explicit versioned compatibility handling plus tests that cover reading old serialized percentile states.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Key Optimizations
values + PercentileLevels + inited_flaglayout.PercentileLevelsmetadata structure.percentile levelsonly once.percentile_array) by sorting quantiles through permutation once, then reusing incrementalnth_elementselection.Performance
before:
now
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)