-
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
Use repartition in window functions to speed up #569
Conversation
0ac4a3a
to
1658606
Compare
This comment has been minimized.
This comment has been minimized.
a61d9ee
to
bb71637
Compare
bb71637
to
0f7ce2f
Compare
Nice, almost 5 times improvement for some queries! But do those have to do with the changes wrt sort or with regards to partition by? |
} | ||
|
||
fn required_child_distribution(&self) -> Distribution { | ||
Distribution::SinglePartition | ||
Distribution::UnspecifiedDistribution |
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.
Would this be correct with a window without any partition by
clause?
In that case I think the required partitions should be 1, as the aggregate function can not be computed only over a part of the data.
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! this is fixed.
aebff92
to
809cd80
Compare
I don't expect that much of an improvement. let me redo the benchmark after #573 is merged. |
a2c90a1
to
9f18613
Compare
latest against the master
@Dandandan turns out it's not that clear cut |
5688cf5
to
22ce613
Compare
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.
👍
9af8b64
to
93d2a91
Compare
93d2a91
to
723e7bc
Compare
Which issue does this PR close?
this pull request is based on #571 so review that first
Closes #435 as this will make it stale
Rationale for this change
benchmark:
What changes are included in this PR?
Are there any user-facing changes?