Fix multiple sources build side in AdaptiveReorderPartitionedJoin#23454
Conversation
|
Please add add a regression test for this one. |
gaurav8297
force-pushed
the
gaurav8297/fix_adaptive_partitioned_join
branch
from
d79a3b3 to
15f7f03
Compare
losipiuk
left a comment
losipiuk
left a comment
There was a problem hiding this comment.
Seem fine - but hard to follow.
So more idiot-friendly comments would be nice.
There was a problem hiding this comment.
should we comply to SystemSessionProperties.isUseExactPartitioning here?
There was a problem hiding this comment.
I think this is only applicable for remote exchange and in our case we only care about changing local exchange. Additionally, we have a check (!expectedRightProperties.isSatisfiedBy(rightProperties)) using deriveStreamPropertiesRecursively so it won't add extra local exchange if not needed.
There was a problem hiding this comment.
This is the same as visitPlan(node, ctx);. Looks like you can just drop if. Or whole visitAggregation actually
There was a problem hiding this comment.
Added some more checks. PTAL
There was a problem hiding this comment.
what if there is partitined exchange? Looks like we will add another one even if not needed
There was a problem hiding this comment.
If there's a partitioned exchange which comply with partitioning scheme needed for join then we won't add anything because of this check. !expectedRightProperties.isSatisfiedBy(rightProperties)
gaurav8297
force-pushed
the
gaurav8297/fix_adaptive_partitioned_join
branch
from
15f7f03 to
cf6e515
Compare
2 participants
Description
Fix: #23407
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: