HappenLee commented on code in PR #67805:
URL: https://github.com/apache/doris/pull/67805#discussion_r3995346112
##########
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:
The NULL path reported above is fixed in
7e1a126ca022f001a57292fb4e6866917c71fa9a, now included in this PR.
Constant folding changed CAST('7' AS DOUBLE) into a non-null literal,
causing the producer to rebuild a non-null AggState layout while the UNION
consumer still expected the analyzed nullable layout. The consumer then skipped
the nested merge. Both _state and _combine now preserve the analyzed state
signature; translation aligns raw combine inputs with that layout, while
explicit AggState casts still retarget it. Layout differences also participate
in expression equality.
The original populated 0.25/0.75 query now raises the incompatible-parameter
error, and matching 0.25 inputs return 7.25 without non_nullable wrappers. The
earlier configured-empty policy is superseded by
822f212f89ec6da1f13f6abe13f4df6a47f77b2b: all-NaN reservoir states ignore their
levels. Both empty states return NaN, and merging an empty state with sample 7
returns 7 in either order, including through _union.
Validation: FE build and Checkstyle passed; all 43 related FE unit tests and
all 13 AggState regression suites passed. The new nullable-rewrite suite covers
32 generated result sets and 8 expected errors, including true NULL input,
mixed UNION branches, both aggregate phases and stored casts.
##########
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:
Updated in 822f212f89ec6da1f13f6abe13f4df6a47f77b2b. Negative-limit collect
states retain their values and existing unlimited-collection behavior.
Parameter checks now apply only when both states contain elements; zero-element
states are skipped, and an empty destination adopts the contributing source's
limit. This supersedes the earlier claim that every configured empty state must
constrain merging.
The BE direct/serialized tests cover both operand orders and numeric, string
and complex-list paths; the SQL parameter suite also passes.
##########
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:
The intended rule has been corrected in
822f212f89ec6da1f13f6abe13f4df6a47f77b2b: a state with no retained values
contributes nothing and does not constrain the quantiles. Exact-percentile V2
now checks retained-value emptiness before comparing parameters, symmetrically
in both merge orders and after serialization. This supersedes my earlier
configured-empty replies.
Direct and serialized tests cover all-NaN/contributing and all-NaN/all-NaN
pairs. Legacy exact-percentile paths that actually retain NaN values continue
to treat those values as data.
##########
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:
Updated in 822f212f89ec6da1f13f6abe13f4df6a47f77b2b to use event emptiness
symmetrically. An eventless RHS is ignored; an eventless destination adopts the
contributing source's pattern/parser configuration. Pattern and argument-count
checks apply when both states retain events. This supersedes my earlier reply
requiring checks for all-false inputs.
Both sequence_match and sequence_count are covered by direct/serialized
tests and SQL merge/union cases in both operand orders.
##########
be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h:
##########
@@ -184,9 +184,16 @@ struct WindowFunnelStateV2 {
}
if (events_list.empty()) {
+ window = other.window;
Review Comment:
The agreed merge rule has been corrected in
822f212f89ec6da1f13f6abe13f4df6a47f77b2b. Eventless V2 funnel states do not
affect the result, so their window/mode must not constrain merging. Empty
inputs are handled before parameter checks in both directions; an empty
destination adopts the contributing source. Both contributing states must still
agree.
Removed the extra initialization flag and configured-empty sorted tag; the
sorted field is again a boolean. Direct/serialized tests and SQL coverage
include all-false states, both orders, reset identities and the default
window_funnel alias. This supersedes the configured-empty behavior described in
my previous reply.
##########
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:
The latest implementation in 822f212f89ec6da1f13f6abe13f4df6a47f77b2b no
longer infers initialization from stored window/mode values. It skips eventless
states before comparing parameters, so residual parameters in an empty state
cannot establish a merge constraint. The extra initialization flag and sorted
tag have been removed.
Historical trial-state compatibility remains outside this PR's scope.
Current-writer reset and eventless behavior is covered by direct/serialized and
SQL tests; the earlier statement that all-false states must constrain merging
is superseded.
--
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]
