[fix](fe) Fix assert row join pushdown alias handling#63892
Open
morrySnow wants to merge 2 commits into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: A scalar subquery comparison can reference a projected alias from the right side of an inner join. PushDownJoinOnAssertNumRows previously identified the pushed condition slots against the project output after rewriting the condition through the project, so aliases expanded to right-child slots could be treated as if no bottom-join slots were involved and the alias projection could be attached to the left child. The rewritten plan then referenced slots that were absent from that child. This change determines slot ownership from the bottom join output after project pushdown, keeps the original pushdown child order when assembling the new join, and adds a unit test for the right-child alias case.
### Release note
Fix query planning failure for scalar subquery comparisons on projected join expressions.
### Check List (For Author)
- Test:
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PushDownJoinOnAssertNumRowsTest
- Behavior changed: Yes. The rewrite now keeps predicates on the child that actually provides the referenced slots.
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a Nereids optimizer rewrite so scalar subquery join predicates using projected aliases are pushed down to the bottom-join child that actually provides the referenced slots.
Changes:
- Determines predicate slot ownership from the bottom join output after project pushdown.
- Refactors join reconstruction into a helper that preserves child ordering.
- Adds a unit test covering a right-child projected alias predicate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownJoinOnAssertNumRows.java |
Updates pushdown slot ownership logic and join assembly. |
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PushDownJoinOnAssertNumRowsTest.java |
Adds regression coverage for right-child alias handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
No blocking issues found in the changed Nereids rule or the added unit test. The fix determines condition slot ownership after project pushdown against the bottom join output, preserves the original bottom-join child order when pushing to the right child, and keeps the upper project wrapper so alias slots remain available.
Critical checkpoint conclusions:
- Goal and proof: The PR addresses the right-child projected alias case for PushDownJoinOnAssertNumRows, and the added unit test covers that shape.
- Scope: The implementation is focused on the existing rule and test, with no new rule registration or unrelated behavior.
- Concurrency and lifecycle: Not applicable; this is a logical rewrite rule with no shared mutable runtime state or special lifecycle management.
- Configuration and compatibility: No new configuration, storage format, or FE-BE protocol compatibility concerns.
- Parallel code paths: The existing left/right pushdown paths are both routed through the helper, and the right-child alias case is now covered.
- Conditional checks: The new empty bottom-join slot guard is safe and prevents transforming scalar-only/project-only conditions that cannot be assigned to one bottom child.
- Test coverage: Coverage is appropriate for the reported alias ownership bug. I attempted
./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PushDownJoinOnAssertNumRowsTest, but it could not complete because this runner is missingthirdparty/installed/bin/protoc. - Observability, transaction, persistence, data writes, and memory tracking: Not applicable to this optimizer-only rewrite.
- Nereids-specific checks: Join types remain restricted to inner/cross joins, no new stage/rule type is introduced, and the rewrite does not introduce an obvious rule-loop risk.
User focus: No additional user-provided review focus was present.
Contributor
TPC-H: Total hot run time: 31538 ms |
Contributor
TPC-DS: Total hot run time: 171874 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Remove an internal issue reference from the assert row join pushdown test comment and fix the illustrated plan tree indentation so it matches the expected rewrite shape.
### Release note
None
### Check List (For Author)
- Test: Manual test
- Verified the diff with git diff --check and confirmed the internal issue reference no longer appears in the changed test file.
- Attempted ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PushDownJoinOnAssertNumRowsTest, but the FE build failed before executing the test because generated ImmutableFlightAuthResult references missing edu.umd.cs.findbugs.annotations.SuppressFBWarnings.
- Behavior changed: No
- Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no blocking issues found in the current patch.
Checkpoint conclusions:
- Goal and tests: The change fixes PushDownJoinOnAssertNumRows slot ownership after pushing a top join condition through a project, and adds a focused unit test for the right-child alias case. The test command is documented by the author; I did not rerun it in this runner.
- Scope: The implementation is small and focused on the affected rewrite, with helper extraction to preserve child ordering.
- Concurrency/lifecycle/config/compatibility: Not applicable; this is a planner rewrite and test-only change, with no shared mutable runtime state, lifecycle management, configuration, protocol, or storage format changes.
- Parallel code paths: The existing left-child and right-child pushdown branches now share assembly logic; I did not find another equivalent path needing the same fix.
- Conditional checks: The new bottom-join output filtering is the right ownership test after project expression substitution, and the empty-slot guard avoids rewriting conditions that do not reference the bottom join.
- Test coverage: The added unit test covers the reported right-child projected alias shape. Existing tests continue to cover left/right pushdown, project wrapping, cross join, multiple-condition rejection, outer-join rejection, and assert-project wrapping.
- Observability/transaction/data writes/memory: Not applicable for this FE logical rewrite.
- Performance: No material regression found; the added work is limited to existing expression slot inspection during rewrite.
Existing review threads: I verified the prior comments about internal issue reference removal and plan-tree indentation and did not duplicate them.
User focus: No additional user-provided review focus was present.
Contributor
TPC-H: Total hot run time: 31565 ms |
Contributor
TPC-DS: Total hot run time: 171960 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
englefly
approved these changes
May 29, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
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: None
Related PR: #57414
Problem Summary: A scalar subquery comparison can reference a projected alias from the right side of an inner join. PushDownJoinOnAssertNumRows previously identified the pushed condition slots against the project output after rewriting the condition through the project, so aliases expanded to right-child slots could be treated as if no bottom-join slots were involved and the alias projection could be attached to the left child. The rewritten plan then referenced slots that were absent from that child. This change determines slot ownership from the bottom join output after project pushdown, keeps the original pushdown child order when assembling the new join, and adds a unit test for the right-child alias case.
Release note
Fix query planning failure for scalar subquery comparisons on projected join expressions.
Check List (For Author)