-
Notifications
You must be signed in to change notification settings - Fork 1k
Description
Is your feature request related to a problem or challenge? Please describe what you are trying to do.
I spent quite a while reading the code for various bitwise kernels while working
with @rluvaton on
I found there are many different, somewhat overlapping functions that are spread over the codebase
This makes it
- Hard to find the appropriate API
- Harder to optimize the correct operations as it is not clear always where
the relevant code is is located
I also think my experience in #8793 suggests there is significant room for optimization,
and that it is important to have two sets of functions: Modify in place and return new buffer
I think part of the current state is somewhat due to history and some of these functions predated the Buffer APIs
Describe the solution you'd like
I would like the implementation for the "core" APIs to be clear:
- Create a new
Bufferfrom a unary / binary bitwise operation - Apply a unary/binary bitwise operation to an existing Mutable buffer in place
After #8619 we have 2
Describe alternatives you've considered
Thus, I propose we do the following:
- Add new
Buffer::bitwise_unaryandBuffer::bitwise_binaryfunctions (that do the same thing asbitwise_bin_op_helperandbitwise_unary_op_helper) but are easier to find and use, and consistently named withPrimitiveArray::unaryandPrimitiveArray::binaryfunctions - Add
BooleanArray::binaryandBooleanArray::unaryfunctions that use the new Buffer functions internally - Deprecate
bitwise_bin_op_helper, andbitwise_unary_op_helperin favor of the new Buffer methods - Deprecate special methods such as
buffer_bin_ormethods in favor of using the new Buffer methods directly
Then we'll basically have two core APIs:
- Apply bitwise operations in place (via apply_bitwise_unary / apply_bitwise_binary introduced in feat: add
apply_unary_opandapply_binary_opbitwise operations #8619) - Create a new buffer with the result of bitwise operations (via
Buffer::bitwise_unary/Buffer::bitwise_binary)
Then all other APIs will then be thin wrappers around the core APIs and we can spend optimziation and testing efforts on these two core APIs.
Additional context