Skip to content

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

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

Standing-Man
Copy link
Contributor

Which issue does this PR close?

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?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 11, 2025
Copy link
Member

@Weijun-H Weijun-H left a 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 👍

@2010YOUY01
Copy link
Contributor

Thank you for making this happen.

I have a suggestion: I think the only field needed inside SPM is the sort keys, how about making it consistent with those in SortExec? https://github.com/apache/datafusion/pull/15077/files#diff-1e143b33b083f63548925777992d4b79de3eb3fccbdcf37d35eff841717533d2

  1. Use sort keys instead of expr to be more explicit
  2. If there are multiple sort key, print them together like sort keys: col1 asc nulls first, col2 desc nulls last

Copy link
Contributor

@alamb alamb left a 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

@Standing-Man Standing-Man force-pushed the SortPreservingMergeExec branch from ac3edad to 4d1c51b Compare March 12, 2025 01:44
@Standing-Man
Copy link
Contributor Author

Hi @2010YOUY01, @alamb, @irenjj and @Weijun-H, Thank you for your suggestions. I have implemented them in the new commit.

@Standing-Man Standing-Man requested review from irenjj and alamb March 12, 2025 02:16
Copy link
Contributor

@2010YOUY01 2010YOUY01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@alamb alamb merged commit efb75f3 into apache:main Mar 12, 2025
24 checks passed
@alamb
Copy link
Contributor

alamb commented Mar 12, 2025

Thank you @Standing-Man and @2010YOUY01 and @Weijun-H

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement tree explain for SortPreservingMergeExec
5 participants