branch-4.1: [fix](be) Fix RuntimeFilter selectivity sampling_frequency lost during VExprContext recreation #62355#62810
Open
BiteTheDDDDt wants to merge 1 commit intoapache:branch-4.1from
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Pull request overview
Cherry-picks the fix from #62355 into branch-4.1 to ensure runtime filter selectivity tracking remains functional when VExprContext is recreated, by preserving and propagating sampling_frequency via the VRuntimeFilterWrapper.
Changes:
- Persist
sampling_frequencyinsideVRuntimeFilterWrapperand apply it toVExprContextduringopen(). - Update
RuntimeFilterConsumer::_get_push_exprs()to pass the computedsampling_frequencyinto all createdVRuntimeFilterWrapperinstances. - Add/extend unit tests covering context recreation and sampling/reset behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| be/src/exprs/vruntimefilter_wrapper.h | Extends wrapper constructor to carry sampling_frequency (defaulting to DISABLE). |
| be/src/exprs/vruntimefilter_wrapper.cpp | Propagates wrapper-stored sampling_frequency into VExprContext during open(). |
| be/src/exec/runtime_filter/runtime_filter_consumer.cpp | Computes sampling frequency once and passes it into wrapper creation for all RF predicate types. |
| be/test/exec/runtime_filter/runtime_filter_selectivity_test.cpp | Adds regression/unit tests documenting reset behavior and proper accumulation under valid sampling. |
| be/test/exec/runtime_filter/vruntimefilter_wrapper_sampling_test.cpp | Adds focused tests validating propagation across VExprContext recreation paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…g VExprContext recreation (apache#62355) introduced by apache#59832 Problem Summary: RuntimeFilter selectivity tracking was completely non-functional because `sampling_frequency` was lost during VExprContext recreation. In `RuntimeFilterConsumer::_get_push_exprs()`, `sampling_frequency=32` was set on a temporary `probe_ctx` VExprContext. However, only VRuntimeFilterWrapper expressions (VExpr) were returned, not the VExprContext. When `_append_rf_into_conjuncts()` later created a new VExprContext via `VExprContext::create_shared(expr)`, the new context had default `_sampling_frequency=-1` (DISABLE_SAMPLING). With `_sampling_frequency=-1`, the condition `(_judge_counter++) >= -1` evaluated to `0 >= -1 → true` on every call, causing `reset_judge_selectivity()` to fire every time. This meant selectivity counters were perpetually reset and never accumulated, making the runtime filter selectivity optimization completely ineffective. **Fix**: Store `sampling_frequency` in VRuntimeFilterWrapper (which survives VExprContext recreation) and propagate it to VExprContext in `VRuntimeFilterWrapper::open()`, which is called on both original and cloned contexts. Fixed a bug where RuntimeFilter selectivity tracking was non-functional due to sampling_frequency being lost during VExprContext recreation, causing runtime filters that should be skipped (due to low selectivity) to never be identified. - Test: Unit Test - Added 2 regression tests to runtime_filter_selectivity_test.cpp - Added 3 new tests in vruntimefilter_wrapper_sampling_test.cpp - All 22 tests pass - Behavior changed: No (selectivity tracking was broken before, this makes it work as designed) - Does this need documentation: No --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> (cherry picked from commit 191061a)
morningman
force-pushed
the
pick-62355-to-4.1
branch
from
e08250e to
b190901
Compare
Contributor
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick of #62355 to branch-4.1.
Conflict resolution
be/src/exec/runtime_filter/runtime_filter_consumer.cpp— branch-4.1 already replacedif (null_aware) return Status::InternalError(...)withDCHECK(null_aware == false) << "..."for the bitmap-predicate branch. Kept 4.1'sDCHECKstyle and applied the newsampling_frequencyargument inVRuntimeFilterWrapper::create_shared(...). The other two min/max branches in the same file already usedDCHECKon both branches and merged automatically.No code logic from #62355 was changed.
Original PR
#62355