-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Implement tree rendering for SortPreservingMergeExec
#15140
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
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.
LGTM! Thanks @Standing-Man 👍
Thank you for making this happen. I have a suggestion: I think the only field needed inside
|
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.
Thanks @Standing-Man -- this is looking good. I think @irenjj 's suggestions are worth doing and then we can get this PR merged in
Signed-off-by: Alan Tang <[email protected]>
Signed-off-by: Alan Tang <[email protected]>
ac3edad
to
4d1c51b
Compare
Hi @2010YOUY01, @alamb, @irenjj and @Weijun-H, Thank you for your suggestions. I have implemented them in the new commit. |
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.
🚀
Thank you @Standing-Man and @2010YOUY01 and @Weijun-H |
Which issue does this PR close?
SortPreservingMergeExec
#15139 and Part of [EPIC] CompleteSQL EXPLAIN
Tree Rendering #14914.Rationale for this change
What changes are included in this PR?
Implement tree rendering for
SortPreservingMergeExec
Are these changes tested?
Yes
Are there any user-facing changes?