fix: Fix derived table breaks on with joins #573
Conversation
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
meta-cla
bot
added
the
CLA Signed
MBkkt
reviewed
Oct 31, 2025
| } | ||
| } | ||
|
|
||
| TEST_F(PlanTest, outerJoinWithInnerJoin) { |
Collaborator
There was a problem hiding this comment.
this test pass on current main, is it expected?
MBkkt
reviewed
Oct 31, 2025
Comment on lines
+30
to
+31
| #define AXIOM_ASSERT_PLAN(plan, matcher) \ | ||
| ASSERT_TRUE(matcher->match(plan)) << plan->toString(true, true); | ||
|
|
Collaborator
There was a problem hiding this comment.
this macros already defined in this file
MBkkt
reviewed
Oct 31, 2025
Comment on lines
1863
to
2049
|
|
||
| // For example on the right of left outer join, a filter must not go to the enclosing dt but must make its own dt. | ||
| if (!contains(allowedInDt, PlanType::kFilterNode)) { | ||
| return wrapInDt(node); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
There's no test for this.
Even if we allow filter for right part of non-inner join, outerJoinWithInnerJoin test still will pass
oerling
force-pushed
the
pushd-fix-pr
branch
5 times, most recently
from
8f17090 to
c585092
Compare
ToGraph builds derived tables bottom up. On the returning edge of recursion we add joins or dt postprocessing steps like group by or limit. When there are things that do not fit the implied processing order of a dt, i.e. joins, group by, having, orderby, limit/ofset, we wrap the plan so far in another dt. For example, scan, limit, filter, limit would have (scan, limit) in a dt and the filter and second limit in a dt containing the first one. Now, when a dt as above is to the right of a join, we must start the dt to the right from scratch, meaning that the tables in the dt must be empty and not contain tables from the left side. On the other hand, for a right side that does not introduce a dt break, we must add the tables on the right to the dt where the left side was added. To this effect we set and check allowedInDt correctly also for filters.
oerling
force-pushed
the
pushd-fix-pr
branch
from
c585092 to
9454b48
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
ToGraph builds derived tables bottom up. On the returning edge of recursion we add joins or dt postprocessing steps like group by or limit. When there are things that do not fit the implied processing order of a dt, i.e. joins, group by, having, orderby, limit/ofset, we wrap the plan so far in another dt. For example,
scan, limit, filter, limit
would have (scan, limit) in a dt and the filter and second limit in a dt containing the first one. Now, when a dt as above is to the right of a join, we must start the dt to the right from scratch, meaning that the tables in the dt must be empty and not contain tables from the left side. On the other hand, for a right side that does not introduce a dt break, we must add the tables on the right to the dt where the left side was added.
To this effect we set and check allowedInDt correctly also for filters.