-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: Output index type instead of u32 for sum_horizontal
with boolean inputs
#20531
Conversation
sum_horizontal
with boolean inputssum_horizontal
with boolean inputs
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20531 +/- ##
=======================================
Coverage 79.05% 79.05%
=======================================
Files 1564 1564
Lines 220627 220628 +1
Branches 2502 2502
=======================================
+ Hits 174413 174416 +3
+ Misses 45640 45638 -2
Partials 574 574 ☔ View full report in Codecov by Sentry. |
@@ -221,16 +255,7 @@ pub fn sum_horizontal( | |||
|
|||
// If we have any null columns and null strategy is not `Ignore`, we can return immediately. | |||
if !ignore_nulls && non_null_cols.len() < columns.len() { | |||
// We must first determine the correct return dtype. | |||
let return_dtype = match dtypes_to_supertype(non_null_cols.iter().map(|c| c.dtype()))? { | |||
DataType::Boolean => DataType::UInt32, |
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.
Can't we leave this code? (replacing indextype). I don't see much benefit in the extra functions that do an extra vec alloc and are only used once.
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, sorry--I mixed up some of the temporal mean_horizontal code with this PR. Will fix.
I may have to re-introduce something similar in that PR but we can do that there.
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.
Done.
No description provided.