HappenLee commented on code in PR #67805:
URL: https://github.com/apache/doris/pull/67805#discussion_r3992869489
##########
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:
1ba9b391e9f2490df06e63f0768e8cebf8dc0bca supersedes my earlier Reservoir
exception. The nested state now retains the quantile for all-NaN samples and
rejects mismatches in both direct and serialized merges. Fresh/reset states use
an out-of-range marker. BE tests pass; SQL tests with explicitly non-null
sample types also pass.
However, end-to-end validation exposed a separate AggState nullability/type
path that can bypass the nested merge altogether. For example:
```sql
SELECT percentile_reservoir_merge(s) FROM (
SELECT percentile_reservoir_state(CAST('7' AS DOUBLE), 0.25) s
UNION ALL
SELECT percentile_reservoir_state(CAST('8' AS DOUBLE), 0.75) s
) t;
```
This returns NULL instead of a mismatch error; wrapping both casts in
non_nullable produces the expected error. The all-NaN form has the same
problem. Observed payload headers and the UNION's nullable subtype point to
inconsistent producer/consumer nullability after rewriting; the exact FE
rewrite remains to be isolated.
I am reopening this thread to discuss that remaining path. The
parameter-check fix is included, but this is not yet a complete fix for every
SQL construction path.
##########
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:
Closing under the compatibility scope discussed above. Historical reset
bytes from the trial AggState implementation are not a supported input for this
change; no claim of preserving their identity semantics is made. The PR
description and docs now state this explicitly.
Fresh/reset states produced by the updated implementation remain identities,
and configured all-false states retain parameters. Current-writer
reset/serialization and eventless tests pass.
##########
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:
The stale bucket count in historical reset bytes is a valid observation if
those old trial AggState payloads must be supported. As agreed for this PR,
that historical-state scenario is outside compatibility scope, so no old-writer
conversion is added.
Current histogram reset clears both data and bucket count. The final tests
cover fresh/reset identities, serialized reset and an actual histogram_union
analytic empty frame subsequently merged with a different bucket count. These
pass. Resolving on that explicit scope and current-writer behavior.
##########
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:
Historical trial AggState reset bytes are outside the compatibility scope,
consistently with the V2 funnel and histogram decisions. The PR description now
makes that limit explicit; retaining field layouts is not a general
mixed-version compatibility promise.
For states produced by the updated implementation, TopN reset clears
N/capacity, while a zero-capacity input keeps its established configuration and
rejects a different one. 1ba9b391e9f2490df06e63f0768e8cebf8dc0bca also
validates parameters before handling empty counter maps, preventing UINT64_MAX
count adjustment when capacity is zero. Tests cover all three variants, both
merge orders, serialized configured-empty states, unchanged compatible counters
and actual analytic empty-frame resets. The final BE and SQL suites pass.
--
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]
