-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
ARROW-6996: [Python] Expose boolean filter kernel on ChunkedArray/RecordBatch/Table #6021
Conversation
python/pyarrow/table.pxi
Outdated
# FilterKernel(_context(), CDatum(self.sp_chunked_array), | ||
# CDatum(mask.sp_array), &out)) | ||
|
||
# return wrap_datum(out) |
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.
Currently, the C++ Filter version that accepts Datums only accepts Array datums. If that would accept Datums of all kinds (and do the dispatch to the specialized Filter version there), the code here could be simpler (it could be something as the above).
Would that be a desired feature of the C++ Filter kernel function?
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 brought this up when the ChunkedArray Filter functions were first implemented, the tracking JIRA is https://issues.apache.org/jira/browse/ARROW-7009
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.
So it's ok to leave that for a separate PR and go ahead here with the different overloads?
9ee58d3
to
224cf5c
Compare
Could this be rehabilitated in time for 0.17.0? Seems useful |
35bb296
to
424cf20
Compare
I rebased this, and cleaned up the code (in the meantime, the Filter kernel added Datum capabilities for all types, so that made the implementation here much cleaner) |
Cool, I'm having another look |
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.
+1. There might be a way to improve code reuse (or docstrings) across the different filter functions but that can be addressed later. Thanks @jorisvandenbossche!
No description provided.