HappenLee commented on code in PR #67805:
URL: https://github.com/apache/doris/pull/67805#discussion_r3992867964
##########
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:
This follow-up supersedes the earlier negative-limit exception. The unified
contract now preserves negative-limit collect states and their existing
unlimited-collection behavior. Every Int32 limit, including -1, -2 and zero, is
a real configuration; only a distinct fresh/reset marker is ignored.
Configurations must match before merging payloads.
Implemented in 1ba9b391e9f2490df06e63f0768e8cebf8dc0bca. Direct/serialized
tests cover both merge orders, compatible negative limits, Int32 endpoints,
fresh adoption and reset across numeric, string and complex-list paths. The
final SQL regression also verifies that merging compatible -1 states retains
both values.
##########
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:
Fixed in 1ba9b391e9f2490df06e63f0768e8cebf8dc0bca. This supersedes the
earlier zero-half-decay exception: zero still returns 0, but now establishes a
configuration and must match the other initialized state. EMA tracks
initialization independently; fresh/reset states remain identities, including
after serialization. The three-double layout is retained, with NaN reserved as
the fresh/reset wire marker because configured NaN half-decays cannot be
serialized.
BE and SQL tests cover 0 versus 1 in both orders, the all-zero state,
compatible zero configurations and empty analytic frames followed by a
different configuration.
##########
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:
The agreed rejection policy is now implemented and validated: a configured
NaN half-decay raises INVALID_ARGUMENT at serialization or finalization, so
newly produced NaN-configured AggState inputs cannot reach subsequent
serialized merging. Ordinary, _state and _combine expected-error SQL cases
pass, as do BE output-boundary tests. This deliberately keeps validation at
output boundaries rather than adding a per-row check; previously serialized
trial NaN states are outside scope.
1ba9b391e9f2490df06e63f0768e8cebf8dc0bca also distinguishes initialized zero
half-decay from fresh/reset states. Resolving under this documented policy.
##########
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:
Confirmed as intended behavior under the unified contract, now applied
consistently to Reservoir and EMA/collect as well. A non-null all-NaN input
establishes its quantile even when exact percentile V2 retains no values.
Different configured quantiles must be rejected in either merge order; only
truly fresh/reset states are identities.
1ba9b391e9f2490df06e63f0768e8cebf8dc0bca adds compatible and incompatible
all-NaN direct/serialized tests for scalar and array exact percentile V2. These
tests pass. Resolving this thread as an intentional parameter-compatibility
rule.
##########
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:
The scoped FE fix is included in this PR: the valid-interval predicate
rejects NaN and both infinities while accepting endpoints 0 and 1.
StateCombinator and CombineCombinator delegate to that legality check. The
previously run FE parameter tests passed, and the final SQL parameter suite
passes.
No BE migration/validation of historical trial NaN-configured payloads is
added. This resolves the accepted-configuration equality issue for states
produced through the supported FE path.
##########
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:
Fixed: sequence_match and sequence_count preserve initialization, pattern
and argument count independently of retained events. Merge validates two
configured states before event-empty fast paths, and serialization preserves
the configuration. All-false inputs with different patterns now reject the
mismatch in both operand orders rather than accepting one direction.
The final BE ASAN and SQL parameter suites pass, including
eventless/eventless and eventless/contributing pairs, both merge/union orders,
compatible states, fresh adoption and reset.
--
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]
