github-actions[bot] commented on code in PR #67805:
URL: https://github.com/apache/doris/pull/67805#discussion_r3988658096
##########
be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h:
##########
@@ -44,7 +45,15 @@ struct QuantileReservoirSampler {
}
void merge(const QuantileReservoirSampler& rhs) {
- level = rhs.level;
+ if (rhs.data.empty()) {
Review Comment:
[P2] Preserve finite configuration for all-NaN reservoir states
`add()` records `input_level` before `ReservoirSampler::insert()` drops NaN,
and serialization still writes that finite level, so an all-NaN state is
configured even though `data.empty()` is true. This return silently ignores an
incompatible configured RHS; in the reverse order the empty-destination branch
overwrites its level. That contradicts the configured-state contract already
applied to exact percentile and eventless funnel states, and it affects direct
plus serialized merge/union. Please track configuration separately from
retained samples, compare levels before the payload-empty fast paths, and cover
valid finite levels with all-NaN samples in both operand orders.
##########
be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h:
##########
@@ -232,9 +248,13 @@ struct WindowFunnelStateV2 {
read_var_int(tmp, in);
sorted = (tmp != 0);
+ // Legacy states use 0/1 and retain their configuration even without
events.
+ initialized = tmp == 2 || window != WINDOW_UNSET ||
Review Comment:
[P1] Preserve identity semantics for legacy reset states
Before this change, V2 `reset()` cleared only `events_list`/`sorted`, so an
old BE can serialize a reset identity while stale window/mode values remain in
its 0/1 header. This is production-reachable with
`window_funnel_v2_union(stored_state) OVER (... ROWS ...)`: the analytic path
resets before an empty frame and the non-null `_union` result serializes the
nested state. This inference marks those old reset bytes initialized, so a new
BE can spuriously reject a different valid configuration. Legacy tag-0/1 empty
states had identity semantics under the old merge logic and are
indistinguishable from old all-false states; preserve that legacy meaning
(using tag 2 only for new configured-empty states), and add an old
add-then-reset writer/new-reader test.
##########
be/src/exprs/aggregate/aggregate_function_topn.h:
##########
@@ -85,8 +86,15 @@ struct AggregateFunctionTopNData {
return;
}
- top_num = rhs.top_num;
- capacity = rhs.capacity;
+ if (!top_num) {
Review Comment:
[P1] Define compatibility for legacy reset TopN states
Old `reset()` cleared only `counter_map`, so an old-BE analytic
`topn*_union` empty frame can serialize a reset identity with stale nonzero
N/capacity. This new check treats those bytes as configured and can reject a
different valid state in either order. Payload emptiness is not enough to
repair it: accepted inputs such as `topn_array_state(1, 1, 0)` establish
N=1/capacity=0 but serialize zero retained elements too. Please add old
add-then-reset writer/new-reader tests for all TopN variants and introduce a
versioned/tagged initialization policy (or another explicit rolling-upgrade
rule) that distinguishes reset identities from legitimately configured empty
states.
##########
be/src/exprs/aggregate/aggregate_function_histogram.h:
##########
@@ -78,7 +81,13 @@ struct AggregateFunctionHistogramData {
return;
}
- max_num_buckets = rhs.max_num_buckets;
+ if (!max_num_buckets) {
Review Comment:
[P1] Treat legacy reset histograms as identities
Before this PR, `reset()` cleared only `ordered_map`, so an old BE can
serialize a reset state as a nonzero bucket count followed by zero elements.
This is reachable through `histogram_union(stored_state) OVER (... ROWS ...)`:
the analytic path resets before an empty frame and serializes the non-null
`_union` result. A new BE then treats the stale count as configuration here and
throws against a different valid Histogram state in either operand order,
although the reset state has no buckets. Since every configured non-null
Histogram add inserts an entry, recognize the legacy nonzero-count/zero-element
encoding as reset identity before comparing parameters, and add old
add-then-reset writer/new-reader coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
