[Bug](function) fix wrong result of window funnel v2 + deduplication/fixed mode#62043
[Bug](function) fix wrong result of window funnel v2 + deduplication/fixed mode#62043BiteTheDDDDt merged 1 commit intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Pull request overview
Fixes window_funnel_v2 DEDUPLICATION mode so that “duplicates” originating from the same physical row (multi-match rows) no longer incorrectly break the funnel chain, aligning behavior with V1.
Changes:
- Update
WindowFunnelStateV2deduplication logic to skip chain termination when the duplicate is from the same row as an existing chain event (via_is_same_row_as_chain). - Add new BE unit tests covering (1) same-row multi-match non-breaking behavior and (2) true-duplicate different-row breaking behavior.
- Add/extend regression tests and expected outputs for the fixed deduplication scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h | Adjusts DEDUPLICATION handling to distinguish same-row multi-match events from true duplicates. |
| be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp | Adds unit tests for the deduplication fix (same-row multi-match + true duplicate). |
| regression-test/suites/query_p0/aggregate/window_funnel_v2.groovy | Adds regression SQL cases targeting the deduplication bug scenario and a true-duplicate scenario. |
| regression-test/data/query_p0/aggregate/window_funnel_v2.out | Updates expected outputs for the newly added regression queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Test DEDUPLICATION mode: same-row multi-event matching should NOT break the chain. | ||
| // This is the exact scenario from the bug report where V1 returns 3 but V2 returned 2. | ||
| // | ||
| // 3 conditions, window=1 day (86400s), DEDUPLICATION mode | ||
| // Row 0 (t=10:00): event1=T, event2=T, event3=F → matches E0+E1 on same row | ||
| // Row 1 (t=11:00): event1=T, event2=T, event3=F → matches E0+E1 on same row |
There was a problem hiding this comment.
The file opens namespace doris { near the top (line 41), but it is no longer closed at the end of the file after adding these tests. This will break compilation due to an unclosed namespace. Add the corresponding } // namespace doris at the end of the file (or otherwise ensure namespaces are correctly balanced).
| // Row 0 (t=10:00): xwhat=1 → matches E0 (xwhat=1) and E1 (xwhat!=2) | ||
| // Row 1 (t=11:00): xwhat=1 → matches E0 (xwhat=1) and E1 (xwhat!=2) | ||
| // Row 2 (t=12:00): xwhat=3 → matches E0 (xwhat=1 false, but see below), E1 (xwhat!=2) and E2 (xwhat=3) | ||
| // Using conditions: xwhat=1, xwhat!=2, xwhat=3 | ||
| // Row 0: E0=T(1=1), E1=T(1!=2), E2=F(1!=3) → same-row E0+E1 | ||
| // Row 1: E0=T(1=1), E1=T(1!=2), E2=F(1!=3) → same-row E0+E1 | ||
| // Row 2: E0=F(3!=1), E1=T(3!=2), E2=T(3=3) → same-row E1+E2 |
There was a problem hiding this comment.
The inline explanation for Row 2 is internally inconsistent/misleading: it says the row "matches E0" even though xwhat=3 does not satisfy the first condition (xwhat = 1). Consider rewriting this comment to avoid suggesting E0 matches on Row 2, so the test case description stays accurate for future readers.
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
BiteTheDDDDt
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 1 issue.
- Goal / correctness: The PR intends to preserve same-row multi-match behavior in
window_funnel_v2, and the added regression coverage proves the reported dedup/fixed regressions. However, the new FIXED-mode lookahead now suppresses some real level-jump breaks, so the task is not fully correct yet. - Minimality / focus: The change is focused on the affected modes and adds targeted tests.
- Concurrency: Not applicable here; the modified code is single-threaded aggregate-state logic.
- Lifecycle / static init: No special lifecycle or static initialization concerns in this change.
- Configuration: No config changes.
- Compatibility: No incompatible FE/BE protocol or storage-format change in this diff.
- Parallel paths: The modified behavior is local to
window_funnel_v2; no missed sister path was identified in this review beyond the logic issue below. - Special conditions: The new same-row lookahead in FIXED mode needs a tighter condition; as written it treats duplicate same-row lower events as valid continuations.
- Test coverage: Added BE unit tests and regression tests cover the reported bug patterns, but there is still a missing negative case for a same-row lower event that does not actually advance the chain.
- Observability: No new observability needed.
- Transaction / persistence: Not applicable.
- Data writes / atomicity: Not applicable.
- FE-BE variable passing: Not applicable.
- Performance: The new lookahead is bounded to same-row continuation runs, so no major performance concern stood out.
- Other issues: See inline comment.
Critical checkpoint conclusions:
- Goal accomplished: Partially. Deduplication fix looks consistent, but FIXED mode still has a correctness hole.
- Small / clear / focused: Yes.
- Concurrency safety: Not applicable.
- Lifecycle safety: Not applicable.
- Config correctness: Not applicable.
- Compatibility: Yes, no compatibility issue observed.
- Parallel code paths covered: Yes for this diff scope.
- Conditional logic justified: Not yet fully; the FIXED-mode guard is too broad.
- Test coverage sufficient: No, one negative FIXED-mode case is still missing.
- Observability sufficient: Not applicable.
- Transaction / persistence correctness: Not applicable.
- Data write correctness: Not applicable.
- FE-BE variable propagation: Not applicable.
- Performance acceptable: Yes.
- Additional issues: 1 correctness issue below.
| 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)) { |
There was a problem hiding this 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.
|
run buildall |
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review finding:
be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h: the new deduplication skip rule is still incorrect when a row matchesE0plus a level already used by the current chain. In that case the code drops a valid restart candidate, so V2 can still return a smaller level than V1.
Critical checkpoints:
- Goal of the current task: Partially achieved. The reported same-row duplicate cases are addressed, but deduplication still misses at least one V1-equivalent path.
- Is the modification as small, clear, and focused as possible: Yes, the patch is focused on
window_funnel_v2plus tests, but the deduplication rule is too aggressive. - Does the code involve concurrency: Not applicable. This path is single-threaded aggregate-state evaluation.
- Is there special or non-intuitive lifecycle management: No special lifecycle issues found.
- Are configuration items added: No.
- Does it involve incompatible changes like function symbols or storage formats: No serialization or storage-format incompatibility introduced in the reviewed diff.
- Are there functionally parallel code paths to the modified one: Yes.
window_funnel/V1 behavior remains the reference for these same-row deduplication semantics, and this path still diverges. - Are there special conditional checks: Yes. The new
_is_same_row_as_chain()check is under-specified for restart semantics and needs a narrower condition. - What is the test coverage: Improved with BE UT and regression coverage, but missing a case where same-row
E0must remain available as an alternative chain start. - Does the feature need increased observability: No.
- Does it involve transaction and persistence-related modifications: No.
- Does it involve data writes and modifications: No.
- Are new variables added that need to be passed between FE and BE: No.
- Performance review: Acceptable for the reviewed change.
- Other issues: No second blocker found beyond the deduplication correctness gap above.
| // in the chain, it's a multi-match row — not a true duplicate. | ||
| // V1 doesn't break chains on same-row events, so skip it. | ||
| if (events_timestamp[0].has_value()) { | ||
| if (_is_same_row_as_chain(i, curr_level, events_timestamp)) { |
There was a problem hiding this comment.
This skip is broader than the bug description and still leaves a correctness hole in deduplication mode. If a row contributes E1 to the current chain and also matches E0, skipping that E0 throws away a valid restart candidate.
Concrete case with window = 15:
r0:E0 @ 00r1:E0 + E1 @ 10r2:E1 @ 11r3:E2 @ 12
V1 can return 3 by starting a new chain from r1 (E0@r1 -> E1@r2 -> E2@r3). With this code, E0@r1 is discarded because it is on the same row as the already-used E1, then E1@r2 is treated as a duplicate and the result stays 2. So the patch still does not preserve the V1 deduplication semantics it is trying to recover here.
There was a problem hiding this comment.
这个应该不算问题，只是语义上的diff
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Found 1 issue.
- Goal / correctness: Partially accomplished. The PR fixes the reported same-row duplicate and FIXED-mode jump cases, but deduplication mode still drops a valid restart candidate and can return a smaller level than
window_funnel/V1. - Minimality / focus: Yes. The change stays focused on
window_funnel_v2plus targeted BE/regression tests. - Concurrency: Not applicable. This is single-threaded aggregate-state logic.
- Lifecycle / static initialization: No special lifecycle or static-init risk found.
- Configuration: No config changes.
- Compatibility: No FE/BE protocol, symbol, or storage-format compatibility issue observed in this diff.
- Parallel code paths:
window_funnelremains the semantic reference for this deduplication behavior, andwindow_funnel_v2still diverges on the restart case described below. - Special conditional checks: The new
_is_same_row_as_chain()guard on theevent_idx == 0branch is too broad. It suppresses same-rowE0even when thatE0should remain available as the start of a new chain. - Test coverage: Improved, but still missing a negative deduplication case where a row contributes both
E0and a higher level already used by the current chain, and the later rows only succeed if that same-rowE0is kept as a restart candidate. - Observability: Not applicable.
- Transaction / persistence: Not applicable.
- Data writes / atomicity: Not applicable.
- FE-BE variable passing: Not applicable.
- Performance: Acceptable for the reviewed change; the new same-row scans are bounded by the local continuation run.
- Other issues: No second blocker found.
Critical checkpoint conclusions:
- Goal accomplished: No, not fully; one deduplication correctness hole remains.
- Is the modification as small, clear, and focused as possible: Yes.
- Does the code involve concurrency: Not applicable.
- Is there special or non-intuitive lifecycle management including static initialization order: No issue found.
- Are configuration items added: No.
- Does it involve incompatible changes like function symbols or storage formats: No.
- Are there functionally parallel code paths to the modified one: Yes, and V1 still exposes a semantic mismatch in deduplication mode.
- Are there special conditional checks: Yes, and the
event_idx == 0same-row skip is under-specified. - What is the test coverage: Improved but still insufficient for the restart regression above.
- Does the feature need increased observability: Not applicable.
- Does it involve transaction and persistence-related modifications: Not applicable.
- Does it involve data writes and modifications: Not applicable.
- Are new variables added that need to be passed between FE and BE: Not applicable.
- Performance review: Acceptable.
- Based on the overall review, are there any other issues: One correctness issue below.
| // in the chain, it's a multi-match row — not a true duplicate. | ||
| // V1 doesn't break chains on same-row events, so skip it. | ||
| if (events_timestamp[0].has_value()) { | ||
| if (_is_same_row_as_chain(i, curr_level, events_timestamp)) { |
There was a problem hiding this comment.
This continue still loses a valid restart candidate in deduplication mode.
Concrete case with window = 15 and conditions E0, E1, E2:
r0:E0 @ 00r1:E0 + E1 @ 10r2:E1 @ 11r3:E2 @ 12
window_funnel/V1 can return 3 by starting a new chain from r1 (E0@r1 -> E1@r2 -> E2@r3). Here V2 processes E1@r1 first, then hits this branch for E0@r1 and skips it because the row is already part of the current chain. That throws away the only viable restart start, so E1@r2 is later treated as a duplicate and the result stays 2.
So the same-row check is too broad for event_idx == 0: it avoids the reported breakage, but it also suppresses legitimate chain restarts.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
1 similar comment
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Found 1 issue.
- Correctness:
FIXEDmode still diverges from the documented V1 row-by-row semantics in cases with an intervening row that matches no funnel condition. The new implementation skips those rows entirely because they are absent fromevents_list, so it can continue a chain that V1 would stop. Minimal reproducer:E0@r0, <no condition>@r1, E1@r2now returns 2, while V1 returns 1 because the next physical row afterE0is notE1.
Critical checkpoint conclusions:
- Goal / correctness / proof: The PR clearly improves
DEDUPLICATIONand severalFIXEDsame-row edge cases, and it adds strong BE + regression coverage for those scenarios. However, the currentFIXEDrewrite does not fully accomplish the stated "Mirrors V1's row-by-row semantics" goal because non-matching intervening rows are still skipped. - Change scope: The code change is focused on the target aggregate and tests.
- Concurrency: No concurrency involved in the touched logic.
- Lifecycle / static init: No special lifecycle or static initialization concerns found.
- Config: No new configuration items.
- Compatibility: No FE/BE protocol or storage compatibility change found.
- Parallel paths: The legacy
window_funnelV1 implementation remains the semantic reference; this PR intentionally aligns V2 with it, so semantic parity is the relevant parallel-path check. - Special conditions: The revised row/continuation assumptions are documented, but the
FIXEDcomments currently overstate parity with V1. - Test coverage: Coverage is good for same-row duplicate / jump cases, but there is no regression case for an intervening non-matching physical row in
FIXEDmode. - Observability: Not applicable for this pure aggregate-state logic change.
- Transaction / persistence: Not applicable.
- Data writes / atomicity: Not applicable.
- FE/BE variable passing: Not applicable.
- Performance: No severe new asymptotic regression confirmed.
- Other issues: None beyond the
FIXEDsemantic gap above.
| /// event-producing row must match c_{K+1} or the chain breaks. Truly | ||
| /// irrelevant rows (matching no condition) are absent from events_list and | ||
| /// implicitly skipped — this is the documented 4.1 behavior change. | ||
| /// |
There was a problem hiding this comment.
This comment says the new algorithm mirrors V1 row-by-row semantics, but the code below still skips physical rows that match no condition because they never enter events_list. That changes results in FIXED mode.
Concrete case with conditions E0, E1, E2 and a large window:
r0:E0r1: matches no conditionr2:E1r3:E2
window_funnel/V1 returns 1 here, because after matching E0 it checks the next physical row (r1) and breaks immediately when that row is not E1 (aggregate_function_window_funnel.h, _match_event_list<WindowFunnelMode::FIXED>(), lines 181-191 in the current tree). This V2 implementation skips r1 entirely and continues at r2, so it returns 3 instead.
That means the implementation still follows the documented V2 semantic difference from V1, rather than fully restoring V1 behavior. At minimum this comment needs to be corrected, and if V1 parity is the intent the algorithm also needs a way to account for intervening non-matching rows plus a regression test for that case.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
1 similar comment
|
run buildall |
|
/review |
|
No issues found in the reviewed changes. Critical checkpoint conclusions:
Residual risk: review was based on code inspection and test coverage in the PR; I did not execute the BE or regression suites in this runner. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
fix update update update update update update format update update
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
…fixed mode (apache#62043) FIXED mode 修复前的问题：旧代码从前往后只扫描一遍，只维护一个链。遇到新的条件1就重启一个新链，丢弃旧链。这样可能错过从后面某个条件1开始能匹配更长链的情况。 FIXED mode 修复后的逻辑： 1. 找到所有满足条件1的行作为候选起点 2. 对每个起点，逐行往后检查： - 下一行必须匹配当前期望的下一个条件（条件链严格递增） - 如果下一行匹配了某个条件但不是期望的那个 → 链断裂，放弃这个起点 - 不匹配任何条件的行会被跳过（这是 4.1 的新行为，V1 会在此断裂） 3. 取所有起点中最长的链作为结果 DEDUP mode 修复前的问题：同样是单次扫描，一个链。无法正确处理"后面的起点能匹配更长链"的情况。 DEDUP mode 修复后的逻辑： 1. 找到所有满足条件1的行作为候选起点 2. 对每个起点，按顺序搜索下一个期望的条件： - 找到匹配条件 K 的行后，检查中间经过的行是否有任何已匹配条件的重复出现 - 有重复 → 链断裂（这就是 deduplication 的含义：同一条件出现两次就打断链） - 无重复 → 接受匹配，继续搜索下一个条件 3. 取所有起点中最长的链作为结果 两种模式的共同改变：旧代码只尝试一个起点（扫描一遍），新代码尝试每个可能的起点（多次扫描），取最优结果。这和 V1 的做法一致。 This pull request refactors and improves the implementation of the window funnel aggregate function in `window_funnel_v2`, focusing on the DEDUPLICATION and FIXED modes. The changes enhance correctness by more closely matching the intended semantics, improve efficiency with multi-pass algorithms, and add comprehensive test coverage for edge cases and complex scenarios. ### Algorithmic and Semantic Improvements * Rewrote the DEDUPLICATION mode (`_get_deduplication`) to use a multi-pass, gap-based duplicate detection algorithm, accurately matching V1 semantics by checking for duplicate events in the "gap" between matched levels and pruning impossible chains early. * Refactored the FIXED mode (`_get_fixed`) to a multi-pass, row-based approach that aligns with documented V2 semantics, including the correct handling of irrelevant events and level jumps, and added pruning for efficiency. ### Code Simplification and Maintenance * Removed legacy and now-unnecessary helper functions (`_promote_level`, `_eliminate_chain`, and related timestamp tracking), streamlining the codebase and reducing complexity. [[1]](diffhunk://#diff-1a1c09dde1a5d97a9723ffebb33ddb27344131ac2031c986ce6866bf248c5971L396-R599) [[2]](diffhunk://#diff-1a1c09dde1a5d97a9723ffebb33ddb27344131ac2031c986ce6866bf248c5971L532-L551) * Added new helper methods (`_next_row_start`, `_row_start`, `_row_contains_level`) to clarify row and event handling, making the logic more readable and maintainable. ### Test Coverage * Added a comprehensive set of regression tests for DEDUPLICATION and FIXED modes, covering multi-event rows, duplicate detection, level jumps, window boundaries, irrelevant event skipping, and various edge cases, ensuring correctness and robustness of the new algorithms.
FIXED mode 修复前的问题：旧代码从前往后只扫描一遍，只维护一个链。遇到新的条件1就重启一个新链，丢弃旧链。这样可能错过从后面某个条件1开始能匹配更长链的情况。
FIXED mode 修复后的逻辑：
4.1 的新行为，V1 会在此断裂）
DEDUP mode 修复前的问题：同样是单次扫描，一个链。无法正确处理"后面的起点能匹配更长链"的情况。
DEDUP mode 修复后的逻辑：
两种模式的共同改变：旧代码只尝试一个起点（扫描一遍），新代码尝试每个可能的起点（多次扫描），取最优结果。这和 V1 的做法一致。
This pull request refactors and improves the implementation of the window funnel aggregate function in
window_funnel_v2, focusing on the DEDUPLICATION and FIXED modes. The changes enhance correctness by more closely matching the intended semantics, improve efficiency with multi-pass algorithms, and add comprehensive test coverage for edge cases and complex scenarios.Algorithmic and Semantic Improvements
_get_deduplication) to use a multi-pass, gap-based duplicate detection algorithm, accurately matching V1 semantics by checking for duplicate events in the "gap" between matched levels and pruning impossible chains early._get_fixed) to a multi-pass, row-based approach that aligns with documented V2 semantics, including the correct handling of irrelevant events and level jumps, and added pruning for efficiency.Code Simplification and Maintenance
_promote_level,_eliminate_chain, and related timestamp tracking), streamlining the codebase and reducing complexity. [1] [2]_next_row_start,_row_start,_row_contains_level) to clarify row and event handling, making the logic more readable and maintainable.Test Coverage