-
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
test: Port (last) repartition.rs
query to sqllogictest
#8936
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.
Thank you @simicd -- this looks reasonable to me but I am not quite sure about the equivalence of these tests
@metesynnada I reviewed #5278 and given the changes in DataFusion since then I am not quite sure how to map its findings to this test.
Can you double check and provide your feedback on if the test proposed in this PR is equivalent to the test in repartition.rs
?
----CoalesceBatchesExec: target_batch_size=8192 | ||
------FilterExec: c3@2 > 0 | ||
--------RepartitionExec: partitioning=RoundRobinBatch(3), input_partitions=1 | ||
----------StreamingTableExec: partition_sizes=1, projection=[c1, c2, c3], infinite_source=true |
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.
👍
--CoalescePartitionsExec | ||
----CoalesceBatchesExec: target_batch_size=8192 | ||
------FilterExec: c3@2 > 0 | ||
--------RepartitionExec: partitioning=RoundRobinBatch(3), input_partitions=1 |
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 partitioning is round robin partitioning where the original test uses hash partitioning. I am not sure if that is equivalent.
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 see! Thanks for looking into it
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 don't think there will be a difference. It's hard to reproduce the hash partitioning with unbounded tables. However, this would work well enough.
Co-authored-by: Andrew Lamb <[email protected]>
--CoalescePartitionsExec | ||
----CoalesceBatchesExec: target_batch_size=8192 | ||
------FilterExec: c3@2 > 0 | ||
--------RepartitionExec: partitioning=RoundRobinBatch(3), input_partitions=1 |
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 don't think there will be a difference. It's hard to reproduce the hash partitioning with unbounded tables. However, this would work well enough.
I took the liberty of merging this branch into |
Thanks again |
Which issue does this PR close?
Closes #8212
Rationale for this change
Move tests to sqllogictest
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No