-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: count of null column shouldn't panic in agg context #15710
Conversation
count | ||
}) | ||
.collect_ca_trusted_with_dtype(&keep_name, IDX_DTYPE); | ||
let out: IdxCa = if matches!(s.dtype(), &DataType::Null) { |
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.
NullArray
is sort of not a good kid. It doesn't follow the invariant that validity = None => null count = 0
. Instead, all of its elements are null, whereas validity is always None
. I'm not sure if we want to add a validity field with all zeros to it.
But anyway, this change/fix can always be viewed as a 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.
We can put this fast path immediately on L173. Then we don't have to duplicate the branch.
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.
Oh, didn't realize we had group.len()
. Updated.
CodSpeed Performance ReportMerging #15710 will improve performances by 10.39%Comparing Summary
Benchmarks breakdown
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15710 +/- ##
========================================
Coverage 81.33% 81.34%
========================================
Files 1374 1375 +1
Lines 176471 176572 +101
Branches 2544 2544
========================================
+ Hits 143539 143635 +96
- Misses 32450 32453 +3
- Partials 482 484 +2 ☔ View full report in Codecov by Sentry. |
Fixes #15705.