[fix](fe) Prevent unsafe CTE runtime filter pushdown#65247
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
| @VisibleForTesting | ||
| public static boolean canPushDownRuntimeFiltersIntoCTEProducer( | ||
| List<RuntimeFilter> rfsToPushDown, CTEId cteId) { | ||
| Preconditions.checkArgument(!rfsToPushDown.isEmpty()); |
There was a problem hiding this comment.
do not check, return false and add a warning log instead
| return false; | ||
| } | ||
| Expression producerTargetExpression = getProducerTargetExpression(rf, cteProducer.getCteId()); | ||
| Preconditions.checkArgument(checkTargetChild(producerTargetExpression).equals(producerSlot)); |
There was a problem hiding this comment.
do not check, return false
There was a problem hiding this comment.
Automated review summary:
I found no additional distinct inline issue to submit in this pass. The two suspicious fail-closed/precondition points in RuntimeFilterGenerator.java are already covered by existing inline threads at lines 882 and 925, so I did not duplicate them.
Critical checkpoint conclusions:
- Goal and tests: the patch prevents shared CTE producer runtime-filter pushdown when producer-side target expressions differ, and preserves the full producer target expression when pushdown is still safe. The added unit test covers the same-expression and different-expression helper behavior.
- Scope and focus: reviewed the full two-file PR scope. There was no additional user-provided review focus.
- Concurrency, lifecycle, config, persistence, compatibility: this FE post-processing change does not add concurrency, lifecycle ownership, session/config items, persistence formats, or new FE-BE thrift fields.
- Parallel paths: reviewed the CTE producer path with the runtime-filter pushdown visitor, expression replacement/equality, runtime-filter context removal, and legacy runtime-filter translation for non-identity target expressions.
- Tests and validation: validation is static only in this runner. The checkout lacks
thirdparty/installedand executablethirdparty/installed/bin/protoc, so I could not run the targeted FE unit test; localgit diff --check HEAD^ HEADalso could not run because this shallow checkout lacksHEAD^. - Subagent conclusions:
optimizer-rewritefound no new candidates;tests-session-configfound no new candidates. Convergence round 1 completed with both subagents replyingNO_NEW_VALUABLE_FINDINGSfor the same final empty inline-comment set.
67dbbe4 to
78aadf0
Compare
### What problem does this PR solve? Issue Number: None Related PR: apache#62383 Problem Summary: Runtime filters pushed from multiple CTE consumers into a shared CTE producer only checked for the same build-side source expression. After runtime filter generation was unified in apache#62383, OR expansion can create multiple consumer-side probe expressions such as pk + 6 and pk - 1 for the same shared producer. Mapping those filters to only the producer slot loses the expression difference and can apply one branch-specific filter to the shared scan, producing wrong results for anti joins. This patch requires all producer-side target expressions to be identical before pushing filters into the shared producer, and preserves the full producer target expression when doing the pushdown. It also adds a regression case with two shared CTE consumers whose producer-side target expressions are cast(pk as bigint) + 6 and cast(pk as bigint) - 1. ### Release note Fix incorrect query results caused by unsafe runtime filter pushdown into shared CTE producers. ### Check List (For Author) - Test: Unit Test / Regression test - ./run-fe-ut.sh --run org.apache.doris.nereids.postprocess.RuntimeFilterTest#testPushSharedCteRuntimeFilterOnlyForSameProducerTargetExpression - Added regression-test/suites/query_p0/runtime_filter/cte-runtime-filter.groovy coverage for shared CTE runtime filters with different producer-side target expressions. Not run locally because no FE was listening on the configured regression port 9030. - Behavior changed: Yes. The optimizer no longer pushes branch-specific runtime filters into a shared CTE producer when consumer probe expressions differ, which restores correct query results. - Does this need documentation: No
78aadf0 to
d6b6b99
Compare
|
/review |
|
run buildall |
There was a problem hiding this comment.
Reviewed PR 65247 at head d6b6b99. This was a static review: thirdparty/installed and thirdparty/installed/bin/protoc are missing in this runner, so I did not run FE unit tests or regression tests locally.
Critical checkpoint conclusions:
- Goal/test proof: the source change addresses the intended unsafe shared CTE runtime-filter pushdown by comparing producer-side target expressions before pushing into the shared producer and by preserving the full producer target expression for pushdown. Test coverage was added, but the regression result is not captured in generated expected output; see the inline comment.
- Scope/focus: the code change is focused to RuntimeFilterGenerator plus FE/regression coverage. No additional user-provided focus was supplied.
- Concurrency/lifecycle/config/persistence/protocol: no new concurrency, lifecycle, config item, persistence format, or FE-BE protocol boundary was introduced by this patch.
- Parallel paths: I checked RuntimeFilterPushDownVisitor, RuntimeFilterTranslator non-identity target translation/grouping, PhysicalCTEConsumer producer-slot mapping, and RuntimeFilter target contracts; I did not find another substantiated source-code issue.
- Existing review context: the existing RuntimeFilterGenerator.java threads at old lines 882 and 925 already cover the production-code Preconditions/return-false concerns, so I did not duplicate them.
Subagent conclusions: optimizer-rewrite reported NO_NEW_VALUABLE_FINDINGS after reviewing the CTE runtime-filter rewrite path. tests-session-config proposed TSC-001, which I accepted as MAIN-001 and submitted as the inline comment below. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
| on cast(p1.pk as bigint) + 6 = b.pk | ||
| and cast(p2.pk as bigint) - 1 = b.pk | ||
| ''' | ||
| assertEquals([[1L]], sql(sharedCteRuntimeFilterSql)) |
There was a problem hiding this comment.
This new wrong-result regression should be recorded as a named qt_/order_qt_ regression output instead of a direct assertEquals(sql(...)) assertion. The suite already has regression-test/data/query_p0/runtime_filter/cte-runtime-filter.out, but this PR does not update that file, so the new expected count is not part of the generated expected-output artifact. Please convert these checks to named qt_ blocks and include the regenerated .out output.
englefly
left a comment
There was a problem hiding this comment.
rf下压cte 应该检查rf.target, 这个pr 补上了这个漏洞. 因为RF 的最初版本里target只有slot,所以当时不需要检查targetExpression. 后来将target 从slot 扩展到 expression, 这里没有跟进
|
PR approved by at least one committer and no changes requested. |
What problem does this PR solve?
Issue Number: None
Related PR: #62383
Problem Summary:
RQG found a wrong-result case in the OR expansion + anti join + shared scan shape. A representative query is:
With runtime filters disabled (
SET runtime_filter_type = ''), the RQG case returns the expected count0. With runtime filters enabled before this fix, it returned100.The regression was introduced by #62383, which unified runtime filter generation and changed runtime filters to the v2-style single-target pushdown path. That made OR-expanded branches create separate runtime filters on CTE consumers of the same shared producer. In this case the consumers use different probe expressions after mapping back to the producer, such as
pk + 6andpk - 1.The shared CTE producer pushdown only checked that every consumer had a runtime filter with the same build-side source expression. It did not check that the consumer probe expressions mapped to the same producer-side target expression. It then pushed the filter using only the producer slot, losing the
+ 6/- 1expression difference. As a result, one branch-specific runtime filter could be applied to the shared scan and affect the other branch, causing the anti join to return wrong results.The added regression case uses a shared CTE with two consumers and a build row
10; one consumer needscast(pk as bigint) + 6 = 10, the other needscast(pk as bigint) - 1 = 10. The correct count is1both with runtime filters enabled and disabled.This patch only pushes runtime filters into a shared CTE producer when all candidate filters map to the same producer-side target expression, and it preserves that full producer target expression during pushdown.
Release note
Fix incorrect query results caused by unsafe runtime filter pushdown into shared CTE producers.
Check List (For Author)
Test: Unit Test / Regression test
./run-fe-ut.sh --run org.apache.doris.nereids.postprocess.RuntimeFilterTest#testPushSharedCteRuntimeFilterOnlyForSameProducerTargetExpressionregression-test/suites/query_p0/runtime_filter/cte-runtime-filter.groovycoverage for shared CTE runtime filters with different producer-side target expressions. Not run locally because no FE was listening on the configured regression port9030.Behavior changed: Yes. The optimizer no longer pushes branch-specific runtime filters into a shared CTE producer when consumer probe expressions differ, which restores correct query results.
Does this need documentation: No