github-actions[bot] commented on code in PR #67805:
URL: https://github.com/apache/doris/pull/67805#discussion_r3978916500
##########
be/src/exprs/aggregate/aggregate_function_collect.h:
##########
@@ -444,6 +445,18 @@ class AggregateFunctionCollect final
Arena& arena) const override {
auto& data = this->data(place);
const auto& rhs_data = this->data(rhs);
+ if constexpr (HasLimit) {
+ if (rhs_data.max_size == -1) {
Review Comment:
[P1] Preserve populated collect states with an explicit `-1` limit
`max_size == -1` is not proof that this source is empty. Neither FE legality
nor `add()` rejects an explicit `-1`: `add()` stores it, the signed/unsigned
size comparison still permits rows, and serialization preserves those rows with
`max_size == -1`. This return therefore drops a populated
`collect_list`/`collect_set` state, and a `-1`/positive pair also bypasses the
intended mismatch rejection depending on direction. Track initialization or
semantic emptiness separately, or reject this value before building a state,
and cover both merge orders plus the serialized path.
##########
be/src/exprs/aggregate/aggregate_function_collect.h:
##########
@@ -444,6 +445,18 @@ class AggregateFunctionCollect final
Arena& arena) const override {
auto& data = this->data(place);
const auto& rhs_data = this->data(rhs);
+ if constexpr (HasLimit) {
+ if (rhs_data.max_size == -1) {
+ return;
+ }
+ if (data.max_size != -1) {
+ if (UNLIKELY(data.max_size != rhs_data.max_size)) {
Review Comment:
[P1] Enforce one `collect_set` limit per state
This comparison assumes each partial already has one well-defined limit, but
FE `CollectSet` does not require argument 2 to be constant. `add()` captures
`max_size` only from the first row and silently ignores later limit-column
values, so `collect_set_combine(value, limit_column)` and ordinary distributed
aggregation can create partials whose behavior depends on row/partition order;
this check sees only the sampled values and may either throw or silently
accept. Add the same constant contract as `CollectList`, or validate every add
before relying on merge-time equality.
##########
be/src/exprs/aggregate/aggregate_function_ema.h:
##########
@@ -86,12 +87,17 @@ struct ExponentialMovingAverageData {
}
void merge(const ExponentialMovingAverageData& rhs) {
- double hd = half_decay != 0.0 ? half_decay : rhs.half_decay;
- if (hd == 0.0) {
+ if (rhs.half_decay == 0.0) {
return;
}
- half_decay = hd;
- merge_point(rhs, hd);
+ if (half_decay == 0.0) {
+ half_decay = rhs.half_decay;
+ } else if (UNLIKELY(half_decay != rhs.half_decay)) {
Review Comment:
[P2] Define equality for every accepted half-decay value
FE accepts any constant numeric half decay, including a NaN double. The
first NaN-configured source initializes the destination; merging a second
identically configured source reaches this branch and throws because `NaN !=
NaN`. Either reject non-finite values at both FE and BE construction boundaries
or use equality semantics that are reflexive for every accepted configuration,
with direct and serialized identical-NaN coverage.
##########
be/src/exprs/aggregate/aggregate_function_group_concat.h:
##########
@@ -73,6 +74,10 @@ struct AggregateFunctionGroupConcatData {
separator = rhs.separator;
data.assign(rhs.data);
} else {
+ if (UNLIKELY(separator != rhs.separator)) {
Review Comment:
[P1] Cover the multi-distinct AggState wrapper
This validation runs only when the nested `group_concat` state is merged.
`multi_distinct_group_concat_state` remains supported, but
`AggregateFunctionDistinct::merge()` unions only its outer argument set and
finalization later calls nested `add()`; it never invokes this method. States
with `','` and `';'` therefore merge silently and whichever tuple is iterated
first selects the separator for all values. Either make that wrapper
unsupported for AggState or preserve and validate its separator before
unioning, with both operand orders in regression coverage.
##########
be/src/exprs/aggregate/aggregate_function_window_funnel.h:
##########
@@ -295,6 +295,15 @@ struct WindowFunnelState {
if (other.events_list.empty()) {
return;
}
+
+ if (events_list.empty()) {
+ window = other.window;
+ window_funnel_mode = other.window_funnel_mode;
+ } else if (UNLIKELY(window != other.window ||
Review Comment:
[P1] Validate funnel parameters while building each state
FE checks only the types of `window` and `mode`; both V1 and V2 `add()`
overwrite them on every row while retaining events gathered under prior values.
Because `_combine` delegates raw adds, one serialized state can already mix
configurations, and ordinary distributed partials may throw or silently merge
here according to their final row rather than the full input. Require these
parameters to be constant or compare them against the first observed values on
every add; apply the same invariant to V2 and cover `_combine` and distributed
varying-parameter inputs.
##########
be/src/exprs/aggregate/aggregate_function_percentile.h:
##########
@@ -736,8 +739,9 @@ struct PercentileExactState {
if (!inited_flag) {
levels = rhs.levels;
inited_flag = true;
- } else {
- levels.merge(rhs.levels);
+ } else if (UNLIKELY(levels.quantiles != rhs.levels.quantiles)) {
Review Comment:
[P2] Treat an all-NaN exact-percentile state as identity
`add_single_range()`/`add_many_range()` set `inited_flag` and the quantiles
before `_append()`, but `_append()` discards every floating NaN. Thus
`percentile_state(NaN, 0.25)` serializes as initialized with zero retained
values. Merging it with a contributing `0.75` state now throws here in either
order even though the all-NaN side cannot affect the result; the reservoir path
already uses retained-sample emptiness for this case. Check `values.empty()`
symmetrically before comparing/adopting levels and add both direct and
serialized merge orders.
##########
be/src/exprs/aggregate/aggregate_function_linear_histogram.h:
##########
@@ -90,8 +91,14 @@ struct AggregateFunctionLinearHistogramData {
return;
}
- interval = rhs.interval;
- offset = rhs.offset;
+ if (interval == 0) {
+ interval = rhs.interval;
+ offset = rhs.offset;
+ } else if (UNLIKELY(interval != rhs.interval || offset != rhs.offset))
{
Review Comment:
[P1] Establish a stable, finite histogram configuration before merge
FE requires neither constant nor finite interval/offset values, while
`add()` overwrites both for every row. A `linear_histogram_combine(value,
interval_column, offset_column)` state can therefore contain buckets computed
under several configurations but serialize only the last one; distributed
partials then throw or pass here based on their last rows. NaN also passes the
existing range predicates, and two identically configured NaN partials reject
each other at this raw comparison. Enforce constant/finite parameters or
validate them on every add, and cover `_combine` plus distributed
varying-parameter cases.
##########
be/src/exprs/aggregate/aggregate_function_ema.h:
##########
@@ -86,12 +87,17 @@ struct ExponentialMovingAverageData {
}
void merge(const ExponentialMovingAverageData& rhs) {
- double hd = half_decay != 0.0 ? half_decay : rhs.half_decay;
- if (hd == 0.0) {
+ if (rhs.half_decay == 0.0) {
Review Comment:
[P1] Do not use a valid zero decay as the empty-state marker
A half decay of `0.0` is accepted and explicitly handled by this aggregate,
and `add()` can populate such a state. This return drops that populated source;
in the reverse mismatch direction, the following `half_decay == 0.0` branch
adopts the nonzero configuration instead of rejecting it. The nullable wrapper
already tracks whether the aggregate received a non-null row, so use an
explicit initialization/emptiness signal rather than a legal parameter value,
and add zero/nonzero tests in both directions.
##########
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()) {
+ return;
+ }
+ if (data.empty()) {
+ level = rhs.level;
+ } else if (UNLIKELY(level != rhs.level)) {
Review Comment:
[P2] Reject or consistently compare non-finite reservoir levels
A binary prepared DOUBLE parameter can supply NaN as a `DoubleLiteral`; the
FE check `value < 0 || value > 1` admits it, and BE stores the level with the
sample. After the first state initializes the destination, an identically
configured second state throws here because `NaN != NaN`. Add finite-value
validation at FE and BE boundaries, or define equality for every accepted
level, and cover the prepared/direct plus serialized paths.
##########
be/src/exprs/aggregate/aggregate_function_sequence_match.h:
##########
@@ -119,6 +120,12 @@ struct AggregateFunctionSequenceMatchData final {
void merge(const AggregateFunctionSequenceMatchData& other) {
if (other.events_list.empty()) return;
+ if (!init_flag) {
Review Comment:
[P1] Treat eventless sequence states symmetrically
`AggregateFunctionSequenceBase::add()` initializes the pattern before
`data.add()` discards an all-false event row. Such a state has `init_flag ==
true` but an empty `events_list`: as RHS it is ignored above, while as LHS this
branch refuses to adopt a differently configured contributing RHS and the next
branch throws. Merge success therefore depends on operand order. If the
destination has no stored events, adopt the contributing source's
pattern/parser state just as an empty RHS is ignored, and test both
`sequence_match` and `sequence_count` orders.
--
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]
