[Opt](func) Improve AggFunc Percentile performance - #62520
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
6317cf8 to
71d44fa
Compare
|
/review |
There was a problem hiding this comment.
I found blocking issues.
Critical checkpoints:
- Goal of current task: the PR aims to speed up exact
percentile/percentile_arraywhile preserving compatibility through the new v1/v2 split. The current code does not fully achieve that. The default Nereids path no longer benefits fromMergePercentileToArray, explicitpercentile_v1can be rewritten into v2 semantics, and old percentileagg_statecompatibility is broken by FE canonicalization. The added tests do not cover those regressions. - Modification size/focus: the BE implementation change itself is reasonably focused, but the FE alias/canonical-name change was not propagated consistently to all parallel paths.
- Concurrency: I did not find new locking or thread-safety issues in the touched code.
- Lifecycle/static initialization: no special lifecycle or static-init issues found in the changed code.
- Configuration: no new configs.
- Incompatible changes / rolling upgrade: there are compatibility regressions. FE now serializes or normalizes percentile names to v2 in places where BE compatibility handling still depends on the old canonical names.
- Functionally parallel code paths: not all equivalent paths were updated.
MergePercentileToArraystill keys on the v1 class only, and the compatibility path only covers the old canonical BE names. - Special conditional checks: quantile validation and null checks look fine; I did not find a blocking issue there.
- Test coverage: BE unit tests cover the new utility helpers, but FE tests miss explicit
percentile_v1rewrite semantics andagg_statecompatibility. I could not run FE UT in this runner becausethirdparty/installed/bin/protocis missing. - Test result files: none changed.
- Observability: no additional observability change appears necessary here.
- Transaction/persistence: persistence compatibility is affected for percentile
agg_statecolumns and materialized views because the function name is part of the stored/read type contract. - Data writes/modifications: no direct table-write logic changed, but
agg_statedeserialize/merge compatibility is impacted. - FE/BE variable passing: no new FE-BE variables.
- Performance: the intended optimization is skipped for the new default
percentile/percentile_cont/medianpath. - Other issues: none beyond the inline comments.
|
|
||
| void insert_result_into(IColumn& to) const { | ||
| auto& column_data = assert_cast<ColumnFloat64&>(to).get_data(); | ||
| if (!inited_flag || levels.empty() || values.empty()) { |
There was a problem hiding this comment.
为什么上面的get返回的是0.0,这里是没有return呢
There was a problem hiding this comment.
行为不一致
Doris> SELECT percentile(num, 0.5) FROM test_percent;
+----------------------+
| percentile(num, 0.5) |
+----------------------+
| NULL |
+----------------------+
1 row in set (0.228 sec)
Doris> SELECT percentile_array(num, [0.1, 0.5]) FROM test_percent;
+-----------------------------------+
| percentile_array(num, [0.1, 0.5]) |
+-----------------------------------+
| [] |
+-----------------------------------+
1 row in set (0.127 sec)
|
/review |
There was a problem hiding this comment.
Findings
AggStateTypeno longer canonicalizes percentile aliases, but Nereids now binds bothpercentileandpercentile_conttoPercentileV2, whose internal name ispercentile_v2. That breaks agg-state identity forpercentile_state/percentile_merge/percentile_unionand inserts into existingagg_state<percentile(...)>/agg_state<percentile_cont(...)>columns, because the FE cast path only accepts equal agg-state function names and BE merge/union requirestype_function_name + suffixto match exactly.map_aggandwindow_funnelkept alias canonicalization precisely to avoid this mismatch.
Critical Checkpoints
- Goal: The PR does improve exact percentile execution and rewrite paths, but it does not fully preserve existing percentile agg-state behavior, so the functional goal is not yet complete. Current tests cover utility logic and rewrite shape, but not agg-state/combinator compatibility.
- Scope: The code changes are fairly focused, but they change user-visible function identities and agg-state metadata, so the compatibility surface is broader than the local diff suggests.
- Concurrency: No new concurrency-sensitive path found.
- Lifecycle / static init: No special lifecycle or static-initialization issue found.
- Config: No new configuration items.
- Compatibility: Blocking issue above. Function identity for percentile aliases changed without the corresponding agg-state canonicalization / compatibility handling.
- Parallel code paths: The main aggregate and rewrite paths were updated, but the agg-state/combinator path was missed.
- Special checks: Quantile validation is still present; no blocking issue found there.
- Tests: Missing regression coverage for
agg_state<percentile(...)>/agg_state<percentile_cont(...)>together withpercentile_state|merge|union, and for pre-existing metadata created before the rename. - Test results: Added unit tests do not cover the blocking compatibility path.
- Observability: No additional observability issue found.
- Transaction / persistence / data writes: Affected. Existing agg-state schemas and rollup queries can fail because the stored function name and generated combinator name diverge.
- Performance: The fast path looks directionally good, but the compatibility regression blocks approval.
Requesting changes until percentile agg-state/function-name compatibility is preserved.
| .put("any", "any_value") | ||
| .put("char_length", "character_length") | ||
| .put("stddev_pop", "stddev") | ||
| .put("percentile_cont", "percentile") |
There was a problem hiding this comment.
Binding percentile/percentile_cont to PercentileV2 while removing the agg-state canonicalization here breaks the *_state/*_merge/*_union path for existing agg_state<percentile(...)> and agg_state<percentile_cont(...)> schemas. StateCombinator / MergeCombinator derive the runtime function name from nested.getName() (percentile_v2 now), but LogicalPlanBuilder still parses those column types as percentile / percentile_cont, CheckCast.checkWithLooseAggState() only accepts equal function names, and BE merge/union checks type_function_name + suffix exactly in vectorized_agg_fn.cpp. A concrete failure is CREATE TABLE t (s agg_state<percentile(int,double)> generic); INSERT INTO t SELECT percentile_state(k, 0.5) ... or SELECT percentile_merge(s) FROM t, which now becomes percentile_v2_* on the query side and no longer matches the stored agg-state name. map_agg / window_funnel keep alias canonicalization for this reason, so percentile needs the same treatment.
8de4532 to
06486f8
Compare
| BeExecVersionManager::registe_restrict_function_compatibility("percentile_array"); | ||
| using creator = creator_with_type_list<TYPE_TINYINT, TYPE_SMALLINT, TYPE_INT, TYPE_BIGINT, | ||
| TYPE_LARGEINT, TYPE_FLOAT, TYPE_DOUBLE>; | ||
| factory.register_alternative_function("percentile", |
There was a problem hiding this comment.
use query_option based upgrade
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
removed
the
approved
d589246 to
689e2b6
Compare
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
689e2b6 to
27e2609
Compare
|
run buildall |
e03264d to
f0fb942
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Blocking findings:\n- still applies old compatibility before the new remap, so rolling-upgrade / downgraded-compatibility queries can still fail on public / spellings even though FE now always enables the new flag.\n- assumes the second argument is constant across the whole block, but only Nereids enforces that. Legacy FE still allows non-constant levels, and now enables globally, so legacy no- queries can silently compute wrong results.\n\nCritical checkpoint conclusions:\n- Goal: the perf optimization is clear, but the current code does not preserve rolling-upgrade compatibility or all FE parallel paths.\n- Scope: the change is focused, but the surrounding compatibility / planner assumptions were not updated consistently.\n- Concurrency: I did not find a new concurrency issue in the touched paths.\n- Compatibility: blocked by the factory ordering issue above.\n- Parallel code paths: blocked by the legacy-FE vs Nereids constant-argument mismatch for .\n- Tests: helper/factory unit tests were added, but there is still no coverage for downgraded or legacy non-constant queries.\n- Tests run in this review: none.
| name_str = function_alias[name]; | ||
| } | ||
|
|
||
| if (attr.new_version_percentile) { |
There was a problem hiding this comment.
still runs before this remap. That means a FE that temporarily lowers for rolling upgrade / compatibility mode will still hit the old / restriction before these public names can be rewritten to / . Since FE now always sets , the remap needs to happen before the old-version compatibility lookup (the scalar-function factory already uses that ordering for ).
| quantile_array.get_data()) | ||
| .get_nested_column(); | ||
| const auto& nested_column_data = | ||
| assert_cast<const ColumnFloat64&, TypeCheckOnRelease::DISABLE>(nested_column); |
There was a problem hiding this comment.
This batch fast path assumes every row in the block shares row 0's quantile array: it always feeds the nested quantiles/null-map starting at offset 0 and uses as the array length for the whole batch. That is only safe if FE guarantees a constant second argument. Nereids does (), but legacy FE still has no such check, and now enables globally. So a legacy no- query like will silently use the first row's levels for the whole block via . The old implementation did not override , so this is a new regression.
There was a problem hiding this comment.
Superseding the malformed bot review body above.
Blocking findings:
AggregateFunctionSimpleFactory::get()still applies oldbe_exec_versioncompatibility before thenew_version_percentileremap, so rolling-upgrade or downgraded-compatibility queries can still fail on publicpercentile(...)andpercentile_array(...)spellings even though FE now always enables the new flag.AggregateFunctionPercentileArrayV2::add_batch_single_place()assumes the second argument is constant across the whole block, but only Nereids enforces that. Legacy FE still allows non-constantpercentile_arraylevels, andSessionVariable.toThrift()now enablesnew_version_percentileglobally, so legacy no-GROUP BYqueries can silently compute wrong results.
Critical checkpoint conclusions:
- Goal: the perf optimization is clear, but the current code does not preserve rolling-upgrade compatibility or all FE parallel paths.
- Scope: the change is focused, but the surrounding compatibility and planner assumptions were not updated consistently.
- Concurrency: I did not find a new concurrency issue in the touched paths.
- Compatibility: blocked by the factory-ordering issue above.
- Parallel code paths: blocked by the legacy-FE vs Nereids constant-argument mismatch for
percentile_array. - Tests: helper and factory unit tests were added, but there is still no coverage for downgraded
be_exec_versionor legacy non-constantpercentile_arrayqueries. - Tests run in this review: none.
| name_str = function_alias[name]; | ||
| } | ||
|
|
||
| if (attr.new_version_percentile) { |
There was a problem hiding this comment.
temporary_function_update(be_version, name_str) still runs before this new_version_percentile remap. That means a FE that temporarily lowers be_exec_version for rolling upgrade or compatibility mode will still hit the old percentile and percentile_array restriction before these public names can be rewritten to percentile_v2 and percentile_array_v2. Since FE now always sets new_version_percentile=true, this remap needs to happen before the old-version compatibility lookup. The scalar-function factory already uses that ordering for the unix_timestamp upgrade path.
| const auto& nested_column_data = | ||
| assert_cast<const ColumnFloat64&, TypeCheckOnRelease::DISABLE>(nested_column); | ||
| DCHECK_EQ(sources.get_data().size(), batch_size); | ||
| AggregateFunctionPercentileArrayV2::data(place).add_many_range( |
There was a problem hiding this comment.
This batch fast path assumes every row in the block shares row 0 values for the quantile array: it always passes nested quantiles and null-map data starting at offset 0, and it uses offset_column_data[0] as the array length for the whole batch. That is only safe if FE guarantees a constant second argument. Nereids does in PercentileArray.checkLegalityBeforeTypeCoercion(), but legacy FE still has no such check, and SessionVariable.toThrift() now enables new_version_percentile=true globally. A legacy no-GROUP BY query like select percentile_array(v, arr_col) from t will therefore go through AggFnEvaluator::execute_single_add() and silently use the first row levels for the whole block. The old percentile_array implementation did not override add_batch_single_place(), so this is a new regression.
FE Regression Coverage ReportIncrement line coverage |
724a22b to
b3b3bdc
Compare
b3b3bdc to
ac8c962
Compare
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
I reviewed the current head ac8c962 against the existing review threads. I am not adding duplicate inline comments because the remaining blockers are already captured in those threads, but they are still valid on this revision:
be/src/exprs/aggregate/aggregate_function_percentile.h:PercentileExactStatestill changes the serialized percentile state format without any compatibility gate or dual-format reader. This remains a storage / wire-format break for persisted or exchangedagg_state<percentile...>bytes.be/src/exprs/aggregate/aggregate_function_simple_factory.h:temporary_function_update()still runs before thenew_version_percentileremap, so queries sent with an olderbe_exec_versioncan still fail during rolling upgrade instead of routing topercentile_v2/percentile_array_v2.be/src/exprs/aggregate/aggregate_function_percentile.h:percentile_array_v2batch fast paths still assume the second argument is constant across the whole block. Nereids enforces that, but legacy FE does not, so legacy no-GROUP BYqueries can still return results based on row 0's array.
Critical checkpoints:
- Goal: Partially met. The new fast path is implemented, but the PR is not safe to merge until the compatibility and legacy-FE correctness issues above are fixed.
- Scope/minimality: Moderate but acceptable; the risky part is the global FE query-option rollout plus the new BE serialization behavior.
- Concurrency: No new concurrency or lock-order issues found.
- Lifecycle/static init: No special lifecycle or static-init issues found.
- Config/query options:
new_version_percentilewas added and propagated on the main FE paths I checked, but its interaction withbe_exec_versionis still incorrect. - Compatibility: Blocking issues remain for rolling upgrade and serialized aggregate-state compatibility.
- Parallel paths: I checked the relevant FE/BE routes and did not find any additional distinct blocker beyond the existing threads.
- Conditions/invariants: The array fast path still depends on an unstated constant-argument invariant on legacy FE paths.
- Test coverage: Insufficient. Current tests cover factory routing and helper behavior, but not old-state deserialization, rolling-upgrade behavior, or legacy-FE non-constant
percentile_arrayexecution. - Test results: The added unit tests look fine for what they cover, but they do not cover the blocking scenarios above.
- Observability: No additional observability requirement identified.
- Persistence/transactions: Persistence compatibility is a blocker because percentile aggregate states are serialized and stored.
- FE/BE variable propagation: The new FE-to-BE query option exists, but the old-version compatibility path is still wrong.
- Performance: The optimization is real, but merging now would trade performance for unresolved correctness and compatibility regressions.
User focus: no additional user-provided focus points.
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
### Key Optimizations - Reworked exact percentile state into a simpler `values + PercentileLevels + inited_flag` layout. - Removed the old Counts-based exact percentile path and replaced it with a lightweight `PercentileLevels` metadata structure. - Reduced add-path overhead by directly appending values into a contiguous buffer and initializing `percentile levels` only once. - Optimized multi-quantile finalize(`percentile_array`) by sorting quantiles through permutation once, then reusing incremental `nth_element` selection. ### Performance before: ```text Doris> select percentile(FUniqID, 0.91) from hits_100m; +---------------------------+ | percentile(FUniqID, 0.91) | +---------------------------+ | 8.749975206154981e+18 | +---------------------------+ 1 row in set (6.149 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) from hits_100m; +------------------------------------------------------------------------------------------------+ | percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) | +------------------------------------------------------------------------------------------------+ | [0, 5.097819197233503e+18, 5.68906185719365e+18, 6.281280528073601e+18, 6.869583632113269e+18] | +------------------------------------------------------------------------------------------------+ 1 row in set (28.984 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56, 0.67, 0.78, 0.89, 0.91]) from hits_100m; ERROR 1105 (HY000): errCode = 2, detailMessage = (127.0.0.1)[E-3113]string column length is too large: total_length=7200000149, element_number=0, rows=0 ``` now ```text Doris> select percentile(FUniqID, 0.91) from hits_100m; +---------------------------+ | percentile(FUniqID, 0.91) | +---------------------------+ | 8.749975206154981e+18 | +---------------------------+ 1 row in set (2.107 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) from hits_100m; +------------------------------------------------------------------------------------------------+ | percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) | +------------------------------------------------------------------------------------------------+ | [0, 5.097819197233503e+18, 5.68906185719365e+18, 6.281280528073601e+18, 6.869583632113269e+18] | +------------------------------------------------------------------------------------------------+ 1 row in set (5.903 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56, 0.67, 0.78, 0.89, 0.91]) from hits_100m; +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56, 0.67, 0.78, 0.89, 0.91]) | +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | [0, 5.097819197233503e+18, 5.68906185719365e+18, 6.281280528073601e+18, 6.869583632113269e+18, 7.459044216605808e+18, 8.044902013692362e+18, 8.640523572825436e+18, 8.749975206154981e+18] | +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (8.866 sec) ```
- Reworked exact percentile state into a simpler `values + PercentileLevels + inited_flag` layout. - Removed the old Counts-based exact percentile path and replaced it with a lightweight `PercentileLevels` metadata structure. - Reduced add-path overhead by directly appending values into a contiguous buffer and initializing `percentile levels` only once. - Optimized multi-quantile finalize(`percentile_array`) by sorting quantiles through permutation once, then reusing incremental `nth_element` selection. before: ```text Doris> select percentile(FUniqID, 0.91) from hits_100m; +---------------------------+ | percentile(FUniqID, 0.91) | +---------------------------+ | 8.749975206154981e+18 | +---------------------------+ 1 row in set (6.149 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) from hits_100m; +------------------------------------------------------------------------------------------------+ | percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) | +------------------------------------------------------------------------------------------------+ | [0, 5.097819197233503e+18, 5.68906185719365e+18, 6.281280528073601e+18, 6.869583632113269e+18] | +------------------------------------------------------------------------------------------------+ 1 row in set (28.984 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56, 0.67, 0.78, 0.89, 0.91]) from hits_100m; ERROR 1105 (HY000): errCode = 2, detailMessage = (127.0.0.1)[E-3113]string column length is too large: total_length=7200000149, element_number=0, rows=0 ``` now ```text Doris> select percentile(FUniqID, 0.91) from hits_100m; +---------------------------+ | percentile(FUniqID, 0.91) | +---------------------------+ | 8.749975206154981e+18 | +---------------------------+ 1 row in set (2.107 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) from hits_100m; +------------------------------------------------------------------------------------------------+ | percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56]) | +------------------------------------------------------------------------------------------------+ | [0, 5.097819197233503e+18, 5.68906185719365e+18, 6.281280528073601e+18, 6.869583632113269e+18] | +------------------------------------------------------------------------------------------------+ 1 row in set (5.903 sec) Doris> select percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56, 0.67, 0.78, 0.89, 0.91]) from hits_100m; +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | percentile_array(FUniqID, [0.12, 0.23, 0.34, 0.45, 0.56, 0.67, 0.78, 0.89, 0.91]) | +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | [0, 5.097819197233503e+18, 5.68906185719365e+18, 6.281280528073601e+18, 6.869583632113269e+18, 7.459044216605808e+18, 8.044902013692362e+18, 8.640523572825436e+18, 8.749975206154981e+18] | +--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (8.866 sec) ```
6 participants
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)