Skip to content

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

Signed-off-by: Alan Tang <jmtangcs@gmail.com>
Signed-off-by: Alan Tang <jmtangcs@gmail.com>
@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 alamb and irenjj 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