You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "github-actions[bot] (via GitHub)" <gi...@apache.org> on 2026/04/03 05:01:06 UTC
Re: [PR] [Bug](function) fix wrong result of window funnel v2 + deduplication/fixed mode [doris]
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org