[opt](merge into) Put MERGE INTO target on the probe side - #66565
Open
starocean999 wants to merge 1 commit into
Open
[opt](merge into) Put MERGE INTO target on the probe side#66565starocean999 wants to merge 1 commit into
starocean999 wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28909 ms |
Contributor
TPC-DS: Total hot run time: 167601 ms |
Contributor
ClickBench: Total hot run time: 23.83 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
starocean999
marked this pull request as ready for review
August 7, 2026 06:44
starocean999
requested review from
924060929,
englefly and
morrySnow
as code owners
August 7, 2026 06:44
morrySnow
changed the title
morrySnow
approved these changes
Aug 7, 2026
morrySnow
left a comment
morrySnow
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR! The semantic equivalence between source LEFT_OUTER target and target RIGHT_OUTER source is correct, and moving the target to the probe side so runtime filters can benefit is a solid optimization. The extraction into MergeUtils is clean.
I submitted a few minor inline comments for consideration — none are blocking. LGTM overall.
morrySnow
reviewed
Aug 7, 2026
| * Build the base join between merge target and source, with the target on the LEFT (probe) | ||
| * side. Doris builds the hash table on the right child, and the target side is structurally | ||
| * the wide one: it must carry every table column plus the row identity for the sink, while | ||
| * the source usually only carries join keys and new values. Keeping the target on the probe |
Contributor
There was a problem hiding this comment.
The Javadoc mentions that "Doris builds the hash table on the right child" — it would be helpful to also explicitly note that (which excludes but allows and ) is a co-motivator for this choice. This makes the rationale self-contained for future readers who might wonder why RIGHT_OUTER was chosen over LEFT_OUTER.
morrySnow
reviewed
Aug 7, 2026
| LogicalPlan result = (LogicalPlan) generateBasePlan.invoke(command); | ||
| Assertions.assertInstanceOf(LogicalJoin.class, result); | ||
| LogicalJoin<?, ?> logicalJoin = (LogicalJoin<?, ?>) result; | ||
| // WHEN NOT MATCHED needs the unmatched source rows: source is the preserved right side |
Contributor
There was a problem hiding this comment.
The new test asserts the left child is a LogicalSubQueryAlias but does not verify the alias value. For consistency with testGenerateBasePlanWithAlias (which extracts the alias and asserts .getAlias() equals "alias"), consider adding the same assertion here.
morrySnow
reviewed
Aug 7, 2026
| * Build the base join between merge target and source, with the target on the LEFT (probe) | ||
| * side. Doris builds the hash table on the right child, and the target side is structurally | ||
| * the wide one: it must carry every table column plus the row identity for the sink, while | ||
| * the source usually only carries join keys and new values. Keeping the target on the probe |
Contributor
There was a problem hiding this comment.
The Javadoc mentions that "Doris builds the hash table on the right child" — it would be helpful to also explicitly note that RuntimeFilterGenerator.DENIED_JOIN_TYPES (which excludes LEFT_OUTER_JOIN but allows RIGHT_OUTER_JOIN and INNER_JOIN) is a co-motivator for this choice. This makes the rationale self-contained for future readers who might wonder why RIGHT_OUTER was chosen over LEFT_OUTER.
Contributor
|
Minor observation: The |
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
3 participants
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.
Issue Number: close #66435
Related PR: #xxx
Problem Summary:
Doris builds the hash table on the right child, so the structurally wide side — the target must carry all columns plus the row identity (for Iceberg: struct<file_path, row_position, ...> with a full S3 URI per row) — always became the build side. Worse, LEFT_OUTER_JOIN is in DENIED_JOIN_TYPES, so the merge join could never produce runtime filters at all. The internal OLAP path (MergeIntoCommand) used LEFT_OUTER_JOIN unconditionally, even without WHEN NOT MATCHED clauses (the external path already had the INNER optimization). For reference, Trino plans MERGE as target RIGHT JOIN source and only allows dynamic filters on INNER || RIGHT — exactly complementary to Doris's denied list.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)