github-actions[bot] commented on code in PR #62043:
URL: https://github.com/apache/doris/pull/62043#discussion_r3031448086
##########
be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h:
##########
@@ -473,21 +492,32 @@ struct WindowFunnelStateV2 {
if (event_idx == 0) {
// Save current chain before starting a new one
if (events_timestamp[0].has_value()) {
- if (curr_level > max_level) {
- max_level = curr_level;
- }
+ max_level = std::max(curr_level, max_level);
_eliminate_chain(curr_level, events_timestamp);
}
events_timestamp[0] = {evt.timestamp, evt.timestamp, i};
curr_level = 0;
first_event = true;
} else if (first_event && !events_timestamp[event_idx -
1].has_value()) {
- // Event level jumped: predecessor was not matched
- if (curr_level >= 0) {
- if (curr_level > max_level) {
- max_level = curr_level;
+ // Event level jumped: predecessor was not matched.
+ // Before breaking the chain, check if any same-row
continuation event
+ // could advance the chain instead. Same-row events follow
consecutively
+ // with continuation flags set.
+ bool same_row_can_advance = false;
+ for (size_t j = i + 1;
+ j < events_list.size() &&
is_continuation(events_list[j].event_idx); ++j) {
+ int sr_idx = get_event_idx(events_list[j].event_idx) - 1;
+ if (sr_idx > 0 && events_timestamp[sr_idx - 1].has_value()
&&
+ !_is_same_row(events_timestamp[sr_idx -
1].last_list_idx, j)) {
Review Comment:
This lookahead is too permissive for `FIXED` mode. It treats any same-row
continuation whose predecessor level exists as proof that the current jump
should be ignored, but that also matches same-row *duplicates* that would not
advance the chain.
Concrete case: `E0@r0, E1@r1, (E3,E1 same row)@r2, E2@r3, E3@r4`. When
processing `E3@r2`, `events_timestamp[1]` is set from `r1`, so the loop marks
`same_row_can_advance = true` because `E1@r2` sees an existing predecessor
(`E0@r0`) from another row. But `E1@r2` is only a duplicate of an
already-matched level, not an advancement, so FIXED mode should still break on
the `E3` jump. With the current code the chain survives and can later reach
level 4 incorrectly.
The lookahead needs to check that the same-row event would actually promote
the chain, e.g. `sr_idx > curr_level` (or an equivalent advancement test),
rather than only checking that its predecessor exists.
--
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]
