-
-
Notifications
You must be signed in to change notification settings - Fork 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
perf: Add fast paths for series.arg_sort and dataframe.sort #19872
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19872 +/- ##
==========================================
- Coverage 79.62% 79.61% -0.01%
==========================================
Files 1564 1564
Lines 217989 218171 +182
Branches 2477 2477
==========================================
+ Hits 173564 173704 +140
- Misses 43857 43899 +42
Partials 568 568 ☔ View full report in Codecov by Sentry. |
#[allow(non_upper_case_globals)] | ||
const is_not_categorical_enum: bool = true; | ||
|
||
if by_column.len() == 1 && is_not_categorical_enum { |
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.
Disabled fast path for categorical columns due to #19900
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, I have left some comments.
We should split the bug fix PR from the optimizations, they should not be in a single PR.
// 2) If array is reverse sorted -> we do a stable reverse. | ||
if is_sorted_flag != IsSorted::Not { | ||
let len_final = if let Some((limit, _desc)) = options.limit { | ||
let limit = limit as usize; |
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 think we can simplify here somewhat after we replace with std::cmp::min
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.
Have rewritten this, let me know if it looks fine now.
{ | ||
let mut current_start: IdxSize = 0; | ||
let mut current_end: IdxSize = 1; | ||
let mut flattened_iter = iters.into_iter().flatten(); |
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.
We need to use iter
as into_iter
boxes.
I also don't like flattened iterators. If we can write this with explicitly looping over the chunks we should prefer that.
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.
Have rewritten this, let me know if it looks fine now.
rev_idx.reverse(); | ||
rev_idx | ||
}, | ||
None => rev_idx, |
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.
What does this return? An empty Vec?
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.
Yes, for arrays of length zero we return an empty vec (From Vec::with_capacity(0)
).
Have created a separate PR for the bug fix- #20004 |
@ritchie46 I have made the requested changes, please have a look . |
9a52e39
to
c9cc500
Compare
b6827d2
to
c38636c
Compare
Thanks a lot @siddharth-vi. Great improvements. Left one comment, but I think we can do that in a later PR. |
@@ -159,6 +159,33 @@ macro_rules! sort_with_fast_path { | |||
}} | |||
} | |||
|
|||
macro_rules! arg_sort_fast_path { |
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.
No blocker for this PR, but I rather see this in a generic function. Could be a follow up.
Add fast paths for series.arg_sort and dataframe.sort(single column) in the following cases -
For the second case, if maintain_order is false, technically we can simply return the reverse of 0..len. However current behavior is that we still do a stable sort on it, maintaining order of elements. This is also the expectation in some unrelated test cases. To maintain this behavior, I have implemented a linear algorithm to do a stable reverse sort. This is faster than the current implementation which is also linear for sorting already sorted data, but takes more time due to creating a copy of the data.
Closes #19364
Benchmarks
dataframe.sort
Code
series.arg_sort
Code
Testing
We need to ensure that even if we take fast path we do not change the final array. I have modified some pre existing tests to also test for fast path. I have added an additional test which checks for correctness of sorting.