Skip to content

[fix](runtime-filter) Restore _applied_rf_num update in late arrival path#62872

Open
BiteTheDDDDt wants to merge 2 commits intoapache:masterfrom
BiteTheDDDDt:fix-applied-rf-num
Open

[fix](runtime-filter) Restore _applied_rf_num update in late arrival path#62872
BiteTheDDDDt wants to merge 2 commits intoapache:masterfrom
BiteTheDDDDt:fix-applied-rf-num

Conversation

@BiteTheDDDDt
Copy link
Copy Markdown
Contributor

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_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. 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;
  • the ApplyAllRuntimeFilters=True info 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)

  • 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

…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>
Copilot AI review requested due to automatic review settings April 27, 2026 10:44
@Thearas
Copy link
Copy Markdown
Contributor

Thearas commented Apr 27, 2026

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@BiteTheDDDDt
Copy link
Copy Markdown
Contributor Author

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings.

Critical checkpoints:

  • Goal / correctness: The PR restores the missing _applied_rf_num = arrived_rf_num update in Scanner::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_lock in both update_late_arrival_runtime_filter() and clone_conjunct_ctxs(). _applied_rf_num is 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_num bookkeeping 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_num also restores the ApplyAllRuntimeFilters=True profile 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.

@BiteTheDDDDt
Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen
Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 100.00% (1/1) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 53.35% (20420/38274)
Line Coverage 36.91% (192498/521560)
Region Coverage 33.23% (149809/450768)
Branch Coverage 34.35% (65527/190776)

@hello-stephen
Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 100.00% (1/1) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 73.78% (27668/37499)
Line Coverage 57.54% (299371/520265)
Region Coverage 54.70% (248938/455094)
Branch Coverage 56.33% (107880/191506)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants