-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve all of the valid orderings during merging. #8169
Preserve all of the valid orderings during merging. #8169
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few minor comments, thanks @mustafasrepo for this small but powerful change. @alamb, PTAL as IOx also leverages orderings and related optimizations extensively
if self.preserve_order { | ||
result = result.with_reorder(self.sort_exprs().unwrap_or_default().to_vec()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other reviewers: This is the key change that avoids losing alternative orderings.
Co-authored-by: Mehmet Ozan Kabak <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started looking at this -- the code looks good to me. I didn't quite have enough time to study the test code to understand what it is doing.
However, please don't feel like you need to wait for my review to merge this if it is blocking something
self.orderings.first().cloned() | ||
let output_ordering = | ||
self.orderings.iter().flatten().cloned().collect::<Vec<_>>(); | ||
let output_ordering = collapse_lex_ordering(output_ordering); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is another core change, right? It preserves all the possible orderings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the place where we concatenate all valid orderings so that they can be preserved during merge
Thank you @mustafasrepo -- this is very neat |
Thanks @alamb, I will go ahead and merge this then. If you encounter any issue related to this, please let us know so we can promptly fix it |
This PR appears to have logic merge conflicts that have broken CI - #8186 @ozankabak could you perhaps take a look? |
Which issue does this PR close?
Partially closes #8064.
Rationale for this change
Currently during merging (
SortPreservingMergeExec
) we can only preserve single ordering of the one of the valid orderings of the table.Consider the case
where we have table1 (satisfies
[a ASC]
and[b ASC]
)and table 2 (satisfies
[a ASC]
and[b ASC]
)during merging of these two table if we merge according to constraint
[a ASC]
.Resulting table may be
where
[a ASC]
property is still valid. However,[b ASC]
property is lost.This PR fixes this problem. With this PR after
SortPreservingMerge
we still preserve[a ASC]
and[b ASC]
properties. After this PR result would bewhere both
[a ASC]
and[b ASC]
are satisfied.The algorithm is as follows:
[a ASC]
and[b ASC]
) but concatenated version of the all of the valid orderings ([a ASC, b ASC]
). This enables us to preserve existing valid orderings during merge.Most of the changes in this PR comes from tests, or test related utils.
What changes are included in this PR?
Are these changes tested?
Yes.
Are there any user-facing changes?