alamb commented on code in PR #8619:
URL: https://github.com/apache/arrow-rs/pull/8619#discussion_r2478518122
##########
arrow-buffer/src/util/bit_util.rs:
##########
@@ -94,6 +94,55 @@ pub fn ceil(value: usize, divisor: usize) -> usize {
value.div_ceil(divisor)
}
+/// Read up to 8 bits from a byte slice starting at a given bit offset.
+///
+/// # Arguments
+///
+/// * `slice` - The byte slice to read from
+/// * `number_of_bits_to_read` - Number of bits to read (must be < 8)
+/// * `bit_offset` - Starting bit offset within the first byte (must be < 8)
+///
+/// # Returns
+///
+/// A `u8` containing the requested bits in the least significant positions
+///
+/// # Panics
+/// - Panics if `number_of_bits_to_read` is 0 or >= 8
+/// - Panics if `bit_offset` is >= 8
+/// - Panics if `slice` is empty or too small to read the requested bits
+///
+#[inline]
+pub(crate) fn read_up_to_byte_from_offset(
+ slice: &[u8],
+ number_of_bits_to_read: usize,
+ bit_offset: usize,
+) -> u8 {
+ assert!(number_of_bits_to_read < 8, "can read up to 8 bits only");
Review Comment:
given this is marked as inline i wonder if we should use `debug_assert` here
to avoid all these checks at runtime on release builds
##########
arrow-buffer/src/buffer/mutable_ops.rs:
##########
@@ -0,0 +1,1031 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use super::MutableBuffer;
+use crate::bit_chunk_iterator::BitChunks;
+use crate::util::bit_util;
+
+impl MutableBuffer {
+ /// Apply a binary bitwise operation on self (mutate) with respect to
another buffer (right).
+ ///
+ /// # Arguments
+ ///
+ /// * `self` - The mutable buffer to be modified in-place
+ /// * `offset_in_bits` - Starting bit offset in Self buffer
+ /// * `right` - slice of bit-packed bytes in LSB order
+ /// * `right_offset_in_bits` - Starting bit offset in the right buffer
+ /// * `len_in_bits` - Number of bits to process
+ /// * `op` - Binary operation to apply (e.g., `|a, b| a & b`)
+ ///
Review Comment:
I think it would help to explain more of how cool this code is
For example:
```suggestion
/// Apply a binary bitwise operation on self (mutate) with respect to
another buffer (right).
///
/// Note: applies the operation a 64-bits (u64) at a time.
///
/// # Arguments
///
/// * `self` - The mutable buffer to be modified in-place
/// * `offset_in_bits` - Starting bit offset in Self buffer
/// * `right` - slice of bit-packed bytes in LSB order
/// * `right_offset_in_bits` - Starting bit offset in the right buffer
/// * `len_in_bits` - Number of bits to process
/// * `op` - Binary operation to apply (e.g., `|a, b| a & b`). Applied a
word at a time
///
```
##########
arrow-buffer/src/builder/boolean.rs:
##########
@@ -258,6 +268,180 @@ impl BooleanBufferBuilder {
}
}
+impl Not for BooleanBufferBuilder {
Review Comment:
I didn't see any tests for these new operations (and my favorite test `cargo
llvm-cov test --html -p arrow-buffer` confirms they are uncovered)
It seems like the use case of these operations are to modify an in progress
BooleanBufferBuilder with a `BooleanBuffer`?
What about an API like the following?
```rust
impl BooleanBufferBuilder {
/// Apply a binary bitwise function to all bits in the builder
fn apply_bitwise_op(&mut self, rhs: &BooleanBuffer, op: F)
where
F: FnMut(u64, u64) -> u64,
{
// validate that the BooleanBuffer is right size
// then directly modify self.mutable_buffer
...
}
}
```
##########
arrow-buffer/src/builder/boolean.rs:
##########
@@ -258,6 +268,180 @@ impl BooleanBufferBuilder {
}
}
+impl Not for BooleanBufferBuilder {
+ type Output = BooleanBufferBuilder;
+
+ fn not(mut self) -> Self::Output {
+ self.buffer.bitwise_unary_op(0, self.len, |a| !a);
+ Self {
+ buffer: self.buffer,
+ len: self.len,
+ }
+ }
+}
+
+impl BitAnd<&BooleanBuffer> for BooleanBufferBuilder {
+ type Output = BooleanBufferBuilder;
+
+ /// Performs a bitwise AND operation between this buffer and `rhs`,
returning the result as a new buffer.
+ ///
+ /// # Panics
+ /// Panics if the lengths of the two buffers are not equal
+ fn bitand(mut self, rhs: &BooleanBuffer) -> Self::Output {
+ self &= rhs;
Review Comment:
I don't really understand what this calls (doesn't it recursively call
itself 🤔)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
