github-actions[bot] commented on code in PR #68246:
URL: https://github.com/apache/doris/pull/68246#discussion_r4056229045
##########
be/src/exprs/aggregate/aggregate_function_topn.h:
##########
@@ -59,7 +59,8 @@ struct AggregateFunctionTopNData {
using DataType = typename PrimitiveTypeTraits<T>::CppType;
void set_paramenters(int input_top_num, int space_expand_rate = 50) {
top_num = input_top_num;
- capacity = (uint64_t)top_num * space_expand_rate;
+ // Non-positive expansion rates retain all candidates during
serialization and merging.
Review Comment:
[P2] Widen the indices before making rate zero unbounded
With `capacity = UINT64_MAX`, a valid rate-zero state can accumulate more
than `INT_MAX` distinct candidates because there is no cardinality ceiling.
Direct finalization then casts `counter_vector.size()` to `int` before limiting
it by `top_num`, so a populated state can emit no values. If the state is
serialized, `write()` also iterates the 64-bit `element_number` with `auto i =
0`; the `ColumnString` size check runs only in `commit()` after `write()`
returns, so it does not guard that signed overflow. Please use size-safe result
bounds and `size_t`/`uint64_t` loop indices.
##########
be/src/exprs/aggregate/aggregate_function_topn.h:
##########
@@ -59,7 +59,8 @@ struct AggregateFunctionTopNData {
using DataType = typename PrimitiveTypeTraits<T>::CppType;
void set_paramenters(int input_top_num, int space_expand_rate = 50) {
top_num = input_top_num;
- capacity = (uint64_t)top_num * space_expand_rate;
+ // Non-positive expansion rates retain all candidates during
serialization and merging.
+ capacity = space_expand_rate <= 0 ? UINT64_MAX : (uint64_t)top_num *
space_expand_rate;
Review Comment:
[P1] Version the new TopN partial-state representation
This changes a rate-zero partial from capacity `0` with zero serialized
elements to capacity `UINT64_MAX` with every candidate, but all three TopN
functions are still selected under the same BE execution version. During a
rolling upgrade, an old BE therefore drops its rate-zero partition and the new
`merge()` ignores that empty decoded map, so the result contains only rows
scanned by upgraded BEs. Negative rates can fail instead: for `N=2, rate=-1`,
an old partial carries capacity `UINT64_MAX-1` and a new partial carries
`UINT64_MAX`, triggering the incompatible-parameter check when both are
nonempty. Stored `AGG_STATE` payloads are also still tagged version 15. Please
introduce a new execution version and preserve/select the old TopN creators for
version 15 (or otherwise fence mixed representations); reader-side
normalization cannot recover candidates that the old rate-zero serializer
omitted.
--
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]
