-
Notifications
You must be signed in to change notification settings - Fork 1k
Speedup sorting for inline views: 1.4x - 1.7x improvement #7856
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
Conversation
FYI @zhuqi-lucas |
Amazing work @Dandandan , reviewing now. |
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.
LGTM thank you @Dandandan !
_ => value_indices.len(), | ||
}; | ||
// 3.a Check if all views are inline (no data buffers) | ||
if values.data_buffers().is_empty() { |
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 @Dandandan , it's very clear we optimize the no data buffers path!
🤖 |
🤖: Benchmark completed Details
|
sorting is about to become even more crazy fast! |
Just when you think you can't make sorting faster.... Along comes @zhuqi-lucas and @Dandandan 🥳 🐶 |
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 @Dandandan and @zhuqi-lucas -- this looks great as well. I agree with @zhuqi-lucas that optimizing the short strings / no buffers case makes a lot of sense
Co-authored-by: Andrew Lamb <[email protected]>
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Speedup by specializing on batches with only inline views.
Are these changes tested?, are they covered by existing tests)?
existing tests
Are there any user-facing changes?
no