Repository navigation
[feature](runtime filter) Add decoupled runtime filter support - #62737
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
englefly
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 30496 ms |
Contributor
TPC-DS: Total hot run time: 168769 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
englefly
force-pushed
the
decouple-rf
branch
2 times, most recently
from
May 19, 2026 06:57
6d0e79f to
b263279
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 30866 ms |
Contributor
TPC-DS: Total hot run time: 167488 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31265 ms |
Contributor
TPC-DS: Total hot run time: 167220 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes. I found one additional blocker beyond the already-open review threads: the new decoupled runtime filter path is enabled by default and introduces an FE-to-BE semantic change (expr_order = -1) that old BEs do not understand, so rolling upgrade can crash or fail queries.
Critical checkpoint conclusions:
- Goal/test proof: the PR aims to add decoupled runtime filters and includes FE/BE unit/regression coverage, but the coverage does not address mixed-version FE/BE compatibility.
- Scope/focus: implementation is mostly focused on runtime filter generation/translation/execution, but default enablement expands blast radius.
- Concurrency/lifecycle: no new thread lifecycle issue found in the reviewed paths; runtime filter wait semantics still have existing open review threads.
- Configuration: new session variables are added; default
enable_decoupled_runtime_filter=trueis unsafe without backend capability gating. - Compatibility: blocking issue found for rolling upgrade because old BE indexes
build_expr_ctxs[expr_order]and cannot handle-1. - Parallel paths: Nereids FE and BE hash-join runtime-filter paths were reviewed; existing open threads already cover expression mapping, wait-time grouping, and const-column materialization issues.
- Tests/results: tests were added, but they do not cover old-BE compatibility or mixed-version behavior.
- Observability/performance: no additional observability blocker found; performance gating by target row count is present.
- User focus: no additional user-provided review focus was supplied.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29004 ms |
Contributor
TPC-DS: Total hot run time: 172852 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run cloud_p0 |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
englefly
marked this pull request as ready for review
June 1, 2026 02:06
starocean999
approved these changes
Jun 1, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
zhaorongsheng
pushed a commit
to zhaorongsheng/doris
that referenced
this pull request
Jun 4, 2026
…e#62737) ### What problem does this PR solve? support decoupled rf. (A join B) join (C join D) decoupled rf: B->C and B->D Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Copilot <[email protected]>
morrySnow
reviewed
Jul 3, 2026
morrySnow
left a comment
morrySnow
left a comment
Contributor
There was a problem hiding this comment.
Code Review — Decoupled Runtime Filter (#62737)
This PR adds decoupled runtime filter support, allowing RF producers and predicate sources to be on different join nodes. The feature is well-structured with FE planning/decision logic and BE execution support. Below are the findings from a thorough review.
🟡 Correctness Concerns
1. Dead code: RuntimeFilterContext.removeRuntimeFilter() is never called
- File:
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/RuntimeFilterContext.java, line 140 - Summary: The new
removeRuntimeFilter(RuntimeFilter rf)method performs full cleanup across all data structures (target scans, targetExprIdToFilter, builder node, joinToTargetExprId), but it is never invoked anywhere in the codebase. A grep forremoveRuntimeFilteracross the entirefe/directory returns only the method definition. If this was intended as part of a removal path for decoupled RFs, the call site is missing. If it's dead code, it adds 24 lines of untested maintenance surface. - Fix: Either add the intended call site or remove the method (annotate with
@VisibleForTestingif it's reserved for tests).
2. hasFilterInSubtree is called twice on the same subtree redundantly
- File:
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/RuntimeFilterGenerator.java, lines 508 and 563 - Summary: In
generateDecoupledRuntimeFilters(),hasFilterInSubtree(decoupledBuilder.right())is called at line 508 for pruning, and if it returnstrue, the result is immediately discarded; the method is called again at line 563 insideshouldPreferDecoupledRf(). This doubles the subtree traversal cost for every decoupled RF candidate in the unknown-stats path.hasFilterInSubtreerecursively walks every node in the subtree — O(subtree_size). - Fix: Cache the
hasFilterInSubtreeresult from line 508 in a local boolean and pass it toshouldPreferDecoupledRf, or restructure to avoid the duplicate call.
🟡 Code Quality Issues
3. Encapsulation break: runtimeFilterWaitInfinitely made public
- File:
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java, line 1750 - Summary: The field
runtimeFilterWaitInfinitelywas changed fromprivatetopublicto allow direct access fromRuntimeFilterTranslator.setWaitTimeMs(). However, a public getter method already exists and should have been used instead. Direct field access breaks encapsulation — any code can now modify this session variable without going through the getter/setter pattern used by all otherSessionVariablefields. - Fix: Revert the visibility change to
privateand use the existing getter method inRuntimeFilterTranslator.setWaitTimeMs().
4. Copy-paste duplication: markDecoupledRfAsNonBlocking and markStandardRfAsNonBlocking are ~85% identical
- File:
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/RuntimeFilterGenerator.java, lines 603-615 and 618-631 - Summary: Both methods iterate runtime filters, match on the same four fields (
exprOrder,builderNode,srcExpr,type), and callrf.setNonBlocking(true). The only differences are: (1) the collection iterated over (targetExprIdToFilter.values()vsgetNereidsRuntimeFilter()), and (2) the exprOrder match (== -1constant vs== exprOrderparameter). Both are called within the same loop iteration at lines 538/540. Any change to the matching logic must be applied identically to both. - Fix: Extract a shared helper:
private void markRfAsNonBlocking(Collection<RuntimeFilter> filters, int expectedExprOrder, Plan builderNode, Expression srcExpr, TRuntimeFilterType type)
5. Magic number: exprOrder == -1 sentinel scattered without a named constant
- Files:
RuntimeFilterGenerator.java,RuntimeFilterPushDownVisitor.java,RuntimeFilter.java(planner),RuntimeFilterTranslator.java,runtime_filter_producer_helper.cpp - Summary: The value
-1is used as a sentinel for decoupled runtime filters in at least 6 files, across both FE (Java) and BE (C++). There is no named constant, no enum value, and no centralized documentation. Every new code path that processes runtime filters must remember to checkexprOrder >= 0before indexing into equi-conjunct lists. A missed check produces anIndexOutOfBoundsExceptionor silent wrong behavior. - Fix: Define a constant, e.g.,
public static final int DECOUPLED_RF_EXPR_ORDER = -1, and use it consistently.
🟢 Minor Observations
6. findBuilderForDecoupledRf/ tryFindDeeperBuilder use hard-coded instanceof chains
- File:
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/RuntimeFilterGenerator.java, lines 641-678 and 692-730 - Summary: These methods handle
AbstractPhysicalJoin,PhysicalProject,PhysicalDistribute, andPhysicalFilterwith explicitinstanceofchecks. Any new plan node type introduced in the future (e.g.,PhysicalSortin the probe path,PhysicalWindow, a new exchange operator) would cause these methods to silently returnnull, skipping a decoupled RF opportunity with no warning. The traversal should either use a generic visitor pattern or fall through with a transparent pass-through for unknown node types.
7. _decoupled_filter_indices is redundant state
- File:
be/src/exec/runtime_filter/runtime_filter_producer_helper.h, line 214 - Summary: The
_decoupled_filter_indicesvector stores indices that could be derived on-the-fly from the originalruntime_filter_descs(checkingexpr_order < 0). Maintaining it in parallel with_filter_expr_contextsis a consistency risk — if a future code path modifies one vector without updating the other, the two would desynchronize.
No blockers found — the PR's core logic is sound and the changes are well-tested with both unit tests and regression tests. The issues above are cleanup/robustness improvements.
Contributor
Inline Review Comments1. Dead code —
|
Contributor
|
Correction to review item #6 (RuntimeFilterProducerHelperCross): After deeper verification, this finding is REFUTED. The cross-join path creates VExprContexts for ALL filter descriptors by position (nested_loop_join_build_operator.cpp lines 77-81 create Additionally, finding about |
englefly
added a commit
to englefly/incubator-doris
that referenced
this pull request
Oct 12, 2026
…port (apache#62737) ### What problem does this PR solve? support decoupled rf. (A join B) join (C join D) decoupled rf: B->C and B->D Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Copilot <[email protected]>
englefly
added a commit
to englefly/incubator-doris
that referenced
this pull request
Oct 12, 2026
…decoupled runtime filter Regenerated with -forceGenOut on branch-4.2 + apache#62737 after rebasing onto the latest branch-4.2: tpcds_sf100 queries 4/11/31/32/74/92 (shape, no_stats_shape, noStatsRfPrune, rf_prune), tpcds_sf1000 queries 4/11/24/30/32/81/92 (shape, hint) and query4/11/24/30/81 in shape_check/tpcds_sf1000_constraints, tpcds_sf10t_orc queries 4/11/18/31/32/54/74/92, tpch_sf1000 q8/q9 plus q2/q17/q18 in shape/no_stats/nostats_rf_prune/rf_prune and q9 in tpch_sf1000_constraints. Only runtime-filter annotations change (build RF target lists and applied RFs); plan shapes, join distributions and predicates are unchanged.
englefly
added a commit
to englefly/incubator-doris
that referenced
this pull request
Oct 13, 2026
…port (apache#62737) support decoupled rf. (A join B) join (C join D) decoupled rf: B->C and B->D Issue Number: close #xxx Related PR: #xxx Problem Summary: None - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Copilot <[email protected]>
englefly
added a commit
to englefly/incubator-doris
that referenced
this pull request
Oct 13, 2026
…decoupled runtime filter Regenerated with -forceGenOut on branch-4.2 + apache#62737 after rebasing onto the latest branch-4.2: tpcds_sf100 queries 4/11/31/32/74/92 (shape, no_stats_shape, noStatsRfPrune, rf_prune), tpcds_sf1000 queries 4/11/24/30/32/81/92 (shape, hint) and query4/11/24/30/81 in shape_check/tpcds_sf1000_constraints, tpcds_sf10t_orc queries 4/11/18/31/32/54/74/92, tpch_sf1000 q8/q9 plus q2/q17/q18 in shape/no_stats/nostats_rf_prune/rf_prune and q9 in tpch_sf1000_constraints. Only runtime-filter annotations change (build RF target lists and applied RFs); plan shapes, join distributions and predicates are unchanged.
yiguolei
pushed a commit
that referenced
this pull request
Oct 13, 2026
…port (#68212) Cherry-pick of #62737 to branch-4.2 [feature](runtime filter) Add decoupled runtime filter support Adaptations for branch-4.2: * `RuntimeFilterConsumer` keeps branch-4.2's `wait_infinitely` computation (no BITMAP_FILTER term) and wraps it in the new `desc->__isset.wait_time_ms` branch. * `RuntimeFilterTranslator`/generator call branch-4.2's `PushDownContext.createPushDownContextForHashJoin` factory instead of master's unified `createPushDownContext`. * The three new session variables use branch-4.2's `@VariableMgr.VarAttr`; `runtimeFilterWaitInfinitely` becomes public as upstream requires. Rebased onto the latest branch-4.2 (Variant relational-ops RF guards merged into `RuntimeFilterPushDownVisitor`, new Variant RF unit tests preserved in `RuntimeFilterTest`). CI fixes: * shape_check baselines regenerated with `-forceGenOut` for every case that failed in P0 regression: tpcds_sf100 4/11/31/32/74/92 (shape, no_stats_shape, noStatsRfPrune, rf_prune), tpcds_sf1000 4/11/24/30/32/81/92 (shape, hint), tpcds_sf1000_constraints 4/11/24/30/81, tpcds_sf10t_orc 4/11/18/31/32/54/74/92, tpch_sf1000 q2/q8/q9/q17/q18 (shape, shape_no_stats, nostats_rf_prune, rf_prune), tpch_sf1000_constraints q9 and the dphyper variants tpcds_sf1000 query54/query72 and tpch_sf1000 q8/q9/q18. Every regenerated file differs from the branch-4.2 baseline only in runtime-filter annotations (build RF target lists and applied RFs); plan shapes, join distributions and predicates are untouched. For the dphyper files the regenerated lines match the mismatches reported by CI one by one. * `RuntimeFilterTest#testDecoupledRuntimeFilter` now asserts the branch-4.2 observable behaviour instead of a separate `exprOrder == -1` filter. On branch-4.2 `RuntimeFilterContext#getRuntimeFilterBySrcAndType` identifies a filter by (srcExpr, type, builderNode) only, so the decoupled RF is merged into the standard RF of the builder join (master dedups per target scan including exprOrder and keeps them apart). The test checks, with and without the feature, that the join2 RF additionally targets `supplier.s_suppkey` and that the join1 RF becomes non-blocking when the decoupled RF is preferred. Testing: * `run-fe-ut.sh --run org.apache.doris.nereids.postprocess.RuntimeFilterTest` -> Tests run: 33, Failures: 0, Errors: 0. * regression suite `nereids_syntax_p0/decoupled_runtime_filter` passes. * every shape_check suite touching a regenerated baseline passes locally (tpcds_sf100/sf1000/sf1000_constraints/sf10t_orc, tpch_sf1000/tpch_sf1000_constraints, including the dphyper suites). The decoupled RFs are reproduced because the local cluster has the same loaded tpcds/tpch data as CI. * Unrelated red checks from the previous run: BE UT failures were `FileCacheProfileReporterTest` (x3) and `RegexGramRecallTest.DynamicPatternRegexExecution`, which are known flaky and are muted in TeamCity (same tests muted on other PR builds), and the NonConcurrent `FE IMAGE GENERATED FAIL / CHECKPOINT FAIL` failure is infra-wide (identical failure on #68788). --------- Signed-off-by: Aurélien Pupier <[email protected]> Co-authored-by: Aurélien Pupier <[email protected]> Co-authored-by: Copilot <[email protected]>
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.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)