ARROW-10944: [Rust] Implement min/max aggregate kernels for BooleanArray #8946
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8946 +/- ##
==========================================
+ Coverage 83.17% 83.18% +0.01%
==========================================
Files 197 197
Lines 48526 48573 +47
==========================================
+ Hits 40360 40407 +47
Misses 8166 8166
Continue to review full report at Codecov.
|
|
On closer inspection, is it actually correct now? What would the output of max(null, false) be in the current implementation? |
|
I think correct would be
|
A: Incorrect (the code in 075277c produces
I implemented this suggestion, as well as additional tests in b5a14d9. Thank you @Dandandan |
|
This PR introduces only private methods and are never used internally. Isn't this an issue? |
Great catch! They should be public |
|
🤦 yes that is an excellent catch @jorgecarleitao -- I plan to polish this up tomorrow. I apologize for all the noise. Thank you all for the through reviews @nevi-me and @Dandandan |
alamb
force-pushed
the
ARROW-10944-min-max-boolean
branch
from
79c9d8b
to
d24c6f1
Compare
Rationale: While Min/Max is a somewhat silly / meaningless operation, typically database systems support it for completeness. This PR adds a basic implementation and tests. More detail on usecase is [here](https://github.com/influxdata/influxdb_iox/issues/568) Closes apache#8946 from alamb/ARROW-10944-min-max-boolean Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
Rationale: While Min/Max is a somewhat silly / meaningless operation, typically database systems support it for completeness.
This PR adds a basic implementation and tests.
More detail on usecase is here