Skip to content

Conversation

@getChan
Copy link
Contributor

@getChan getChan commented Apr 6, 2025

Which issue does this PR close?

  • Closes #.

Rationale for this change

easy to understand because it means the number of RepartitionExec input's output partition number

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

explain tree displaying

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Apr 6, 2025
@getChan getChan changed the title chore: Repartitionexec display tree chore: improve RepartitionExec display tree Apr 6, 2025
f,
"output_partition_count={}",
"input_partition_count={}",
self.input.output_partitioning().partition_count()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the output partition number is required to display in this node, but it's not implemented correctly. (or maybe now the planner always set the output partition the same as the input partition count)
I suggest to change the implementation instead, maybe we can display both the input and output partition count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for review.
The output partition number is already included in the partitioning_scheme, so it is displayed.
ex)

13)┌─────────────┴─────────────┐
14)│      RepartitionExec      │
15)│    --------------------   │
16)│   input_partition_count:  │
17)│             1             │
18)│                           │
19)│    partitioning_scheme:   │
20)│     RoundRobinBatch(4)    │
21)└─────────────┬─────────────┘

Copy link
Contributor

@2010YOUY01 2010YOUY01 Apr 7, 2025

Choose a reason for hiding this comment

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

Maybe as a follow-up task we can update it to be more clear like

input_partition_count: 1
output_partition_count: 4

partition_scheme: RoundRobin

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.

Makes sense, thank you.

@2010YOUY01 2010YOUY01 merged commit 5a6db9f into apache:main Apr 8, 2025
29 checks passed
nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
* fix RepartitionExec tree displayformat

* pass sqllogictests
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.

2 participants