HappenLee commented on code in PR #67805:
URL: https://github.com/apache/doris/pull/67805#discussion_r3998582144
##########
be/src/exprs/aggregate/aggregate_function_percentile.h:
##########
@@ -406,15 +408,20 @@ struct PercentileApproxArrayState {
return;
}
- if (!init_flag) {
- levels.merge(rhs.levels);
+ // Preserve the result shape when every state is empty, but let
contributing
+ // samples replace parameters recorded by an empty destination.
+ if (!init_flag || levels.empty() || digest->total_size() == 0) {
Review Comment:
Fixed in bf483fc348a1ea28c511e24fc1719f4162e71ad1.
We made the empty-result contract explicit: percentile_approx_array now
returns [] whenever it retains no samples, including all-NaN input. Sample-free
states serialize using the existing fresh-state encoding, so their serialized
representation also no longer depends on quantile count, compression or merge
order.
merge() skips sample-free sources and lets a sample-free destination adopt a
contributing source. Quantiles and compression are compared only when both
states contain samples. This intentionally changes all-NaN results from an
array of NaNs to []; the documentation and release note have been updated.
Validation: 9 BE ASAN tests passed, including different quantile-array
lengths, both operand orders, direct/serialized merges, both association
orders, later contributors, canonical empty serialization and reset. All 14
AggState regression suites and the original percentile_approx_array suite
passed. The new suite covers 45 generated result sets and 16 expected errors
across ordinary aggregation and _state/_combine/_merge/_union, including
one-/two-phase aggregation. Results were generated with the repository runner
and passed a normal rerun.
##########
be/src/exprs/aggregate/aggregate_function_collect.h:
##########
@@ -45,15 +46,24 @@
#include "util/var_int.h"
namespace doris {
+struct AggregateFunctionCollectLimitData {
+ // Limits are Int32 inputs. Keep all of them, including negative limits,
distinct from
+ // the fresh/reset marker while retaining the existing Int64 serialized
field.
+ static constexpr Int64 UNINITIALIZED_MAX_SIZE =
+ static_cast<Int64>(std::numeric_limits<Int32>::min()) - 1;
+ Int64 max_size = UNINITIALIZED_MAX_SIZE;
Review Comment:
Fixed in c3da1036090188708720960b4f98afb9cffd1bef.
Restored the original max_size = -1 representation across collect
specializations. The serialized marker is again one ZigZag byte plus the
existing length byte, removing the four-byte increase from no-limit partial
states. Existing read/write fields and order are retained.
The merge validation still skips empty sources, adopts the source limit for
empty destinations, and rejects different limits before merging two populated
states, including distinct negative limits. FE requires a constant limit, so
restoring the original initialization checks preserves the documented unlimited
behavior for matching negative limits.
Added exact serialization-size and round-trip checks for fresh, populated,
deserialized and reset/reused states across numeric/string/complex collect_list
paths, collect_set paths, aliases and array_agg with non-nullable inputs.
Validation for this change: 11 BE ASAN tests and the test_agg_state_parameters
and array_agg regression suites passed.
--
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]
