[fix](runtime-filter) Restore _applied_rf_num update in late arrival path#62872
[fix](runtime-filter) Restore _applied_rf_num update in late arrival path#62872BiteTheDDDDt wants to merge 2 commits intoapache:masterfrom
Conversation
…path ### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#59786 Problem Summary: PR apache#59786 (commit e78d089) refactored Scanner::try_append_late_arrival_runtime_filter() and accidentally removed the trailing `_applied_rf_num = arrived_rf_num;` assignment. As a result, _applied_rf_num is permanently 0 after construction: * the fast-path `_applied_rf_num == _total_rf_num` early return at the top of the function never fires, so every batch goes through the full late-arrival check; * `arrived_rf_num == _applied_rf_num` only short-circuits when no runtime filter has ever arrived, so once any RF arrives every subsequent call needlessly clears _conjuncts, re-clones them, and appends the old ctxs into _stale_expr_ctxs (CPU waste + slow memory growth); * the `ApplyAllRuntimeFilters=True` info string in profile (file_scanner.cpp) is never emitted; * `DCHECK(_applied_rf_num < _total_rf_num)` is effectively dead because the left-hand side is always 0. Restore the single missing assignment after cloning the new conjunct ctxs to bring back the original behavior. ### Release note None ### Check List (For Author) - Test: No need to test (one-line restoration of removed assignment; behavior covered by existing runtime-filter regression tests) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
Restores correct late-arrival runtime filter bookkeeping in Scanner::try_append_late_arrival_runtime_filter() by reintroducing the missing _applied_rf_num update that was inadvertently removed during the refactor in #59786.
Changes:
- Re-adds
_applied_rf_num = arrived_rf_num;after successfully cloning updated conjunct contexts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
### What problem does this PR solve? Related PR: apache#62872 Problem Summary: Lock in the fix for `Scanner::try_append_late_arrival_runtime_filter` so the regression introduced in PR apache#59786 cannot reappear. The new BE unit test instantiates a Scanner over MockScanLocalState/MockScanOperatorX, signals two runtime filters, and asserts that `_applied_rf_num` advances to `_total_rf_num` and that the second invocation hits the fast-path early return without re-cloning conjuncts. ### Release note None ### Check List (For Author) - Test: Unit Test - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
There was a problem hiding this comment.
No blocking findings.
Critical checkpoints:
- Goal / correctness: The PR restores the missing
_applied_rf_num = arrived_rf_numupdate inScanner::try_append_late_arrival_runtime_filter(). That matches the scanner call chain and fixes the repeated re-clone / missed fast-path regression described in the PR. The added unit test covers the scanner-side state transition that regressed. - Scope: The production change is minimal and focused: one production line plus a targeted BE unit test.
- Concurrency: The relevant shared state remains protected by
ScanLocalStateBase::_conjuncts_lockin bothupdate_late_arrival_runtime_filter()andclone_conjunct_ctxs()._applied_rf_numis scanner-local, so this change does not introduce a new cross-thread race. - Lifecycle / initialization: No new production lifecycle hazards or static-initialization risks were introduced.
- Config / compatibility / persistence: No new configs, FE-BE protocol changes, persistence changes, or transaction / storage-format changes are involved here.
- Parallel paths: I checked the operator-side and multicast late-arrival runtime-filter paths; this regression is specific to the scanner-side
_applied_rf_numbookkeeping and the fix is correctly localized. - Tests: The added BE unit test is appropriate for this regression. I did not run the test suite in this review environment.
- Observability / performance: Restoring
_applied_rf_numalso restores theApplyAllRuntimeFilters=Trueprofile signal and avoids the repeated clear / clone work that the regression caused.
User focus points:
- No additional user-provided review focus.
Conclusion:
- I did not find a blocking issue in this PR.
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Related PR: #59786
Problem Summary: PR #59786 (commit e78d089) refactored
Scanner::try_append_late_arrival_runtime_filter()and accidentally removed the trailing_applied_rf_num = arrived_rf_num;assignment. As a result_applied_rf_numis permanently 0 after construction:_applied_rf_num == _total_rf_numearly return at the top of the function never fires, so every batch goes through the full late-arrival check;arrived_rf_num == _applied_rf_numonly short-circuits when no runtime filter has ever arrived. Once any RF arrives, every subsequent call needlessly_conjuncts.clear()s, re-clones the conjunct ctxs and appends the old ones into_stale_expr_ctxs— wasted CPU plus slow memory growth;ApplyAllRuntimeFilters=Trueinfo string in scanner profile (file_scanner.cpp:384) is never emitted;DCHECK(_applied_rf_num < _total_rf_num)is effectively dead because the left-hand side is always 0.Restore the single missing assignment after cloning the new conjunct ctxs.
Release note
None
Check List (For Author)