-
-
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
feat(python): Add name
parameter to GroupBy.len
method
#15235
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15235 +/- ##
==========================================
+ Coverage 81.25% 81.29% +0.04%
==========================================
Files 1354 1356 +2
Lines 175390 175692 +302
Branches 2518 2525 +7
==========================================
+ Hits 142505 142830 +325
+ Misses 32404 32379 -25
- Partials 481 483 +2 ☔ View full report in Codecov by Sentry. |
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 had to think on this one a bit, but I think it's a valid addition. The default name can clash, so if you're grouping by a column named len
, you cannot use the GroupBy.len
method.
Since this is supposed to be a syntactic sugar / ease of use function, it might as well be easy to use... or you might argue the other way and say it should be simple, and for more complex use cases, do not use the syntactic sugar.
Anyway, I think it should be added. If you can address the 2 comments this can be merged!
e75c6c3
to
19fab97
Compare
19fab97
to
2894443
Compare
len
colname
parameter to GroupBy.len
method
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.
Great, thanks!
Closes #15222.
Seems like a simple/reasonable addition? While adding tests for this, I also found/fixed a bug where a column name is legitimately the empty string, and we reference it from
schema_overrides
; was getting incorrectly renamed as "column_0".