-
Notifications
You must be signed in to change notification settings - Fork 72
[REVIEW] Fast path when possible for non numeric aggregation #236
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
[REVIEW] Fast path when possible for non numeric aggregation #236
Conversation
The PR should be ready for an inital review. |
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.
The changes look great and have a huge performance gains for these specific cases. Added a few comments inline.
It would be great to include some test cases to test_groupby as well that add code coverage for this case.
Thanks , have addressed all of your reviews, I think this is ready for another review.
I have included test cases for
Added Datetime tests with commit 6598259 For test cases corresponding to |
rerun tests |
Codecov Report
@@ Coverage Diff @@
## main #236 +/- ##
==========================================
- Coverage 99.61% 99.50% -0.12%
==========================================
Files 64 64
Lines 2608 2622 +14
Branches 367 372 +5
==========================================
+ Hits 2598 2609 +11
- Misses 8 9 +1
- Partials 2 4 +2
Continue to review full report at Codecov.
|
rerun tests |
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; thanks for the work here @VibhuJawa 😄
This PR adds fast path when possible for non numeric aggregation. The PR speeds up aggregations by avoiding the apply based path here.
Motivating Example
Mainline
With this PR (236):