github-actions[bot] commented on code in PR #67805:
URL: https://github.com/apache/doris/pull/67805#discussion_r3995490626
##########
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:
[P2] Keep the no-limit collect marker compact
This shared default also applies when `HasLimit=false`, although those
collect states never read or update a limit and still serialize `max_size`
unconditionally. ZigZag/base128 encodes the previous `-1` in one byte, but
`Int32::min - 1` needs five, adding four bytes to every partial group for
one-argument `collect_list`/`collect_set` (and their aliases) plus non-nullable
`array_agg`. At 10 million groups that is about 40 MB of avoidable
shuffle/spill metadata. Please reserve the collision-free sentinel for limited
specializations, or otherwise keep the no-limit wire value compact, and add a
no-limit serialization-size test.
##########
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:
[P1] Keep all-empty percentile-array merges order-independent
When both `percentile_approx_array` states are initialized but contain no
samples, this branch always replaces the destination's `levels` and compression
with the RHS metadata. For example, merging all-NaN states with quantiles
`[0.25]` and `[0.25, 0.75]` returns one or two NaNs solely according to operand
order, and serialization preserves that behavior for `_union`/`_merge`.
Distributed partial-state order is unspecified, so the same state set can
produce different array shapes. Please select all-empty metadata
deterministically while still letting a later contributor override it, and
cover reversed direct and serialized merges with different quantile-array
lengths.
--
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]
