alamb commented on a change in pull request #8645:
URL: https://github.com/apache/arrow/pull/8645#discussion_r522935301
##########
File path: rust/arrow/src/util/bit_util.rs
##########
@@ -99,36 +99,6 @@ pub unsafe fn unset_bit_raw(data: *mut u8, i: usize) {
*data.add(i >> 3) ^= BIT_MASK[i & 7];
}
-/// Sets bits in the non-inclusive range `start..end` for `data`
-///
-/// # Safety
-///
-/// Note this doesn't do any bound checking, for performance reason. The
caller is
-/// responsible to guarantee that both `start` and `end` are within bounds.
-#[inline]
-pub unsafe fn set_bits_raw(data: *mut u8, start: usize, end: usize) {
Review comment:
🎉
##########
File path: rust/arrow/src/array/builder.rs
##########
@@ -525,7 +515,7 @@ impl<T: ArrowPrimitiveType> ArrayBuilder for
PrimitiveBuilder<T> {
let sliced = array.buffers()[0].data();
// slice into data by factoring (offset and length) * byte
width
self.values_builder
- .write_bytes(&sliced[(offset * mul)..((len + offset) *
mul)], len)?;
+ .write_bytes(&sliced[(offset * mul)..((len + offset) *
mul)], len);
Review comment:
Nice 👁️ -- I agree that removing the `len` parameter entirely would be
the best course of action here
##########
File path: rust/arrow/src/array/builder.rs
##########
@@ -397,18 +396,9 @@ impl<T: ArrowPrimitiveType> BufferBuilder<T> {
/// Writes a byte slice to the underlying buffer and updates the `len`,
i.e. the
/// number array elements in the builder. Also, converts the `io::Result`
/// required by the `Write` trait to the Arrow `Result` type.
- fn write_bytes(&mut self, bytes: &[u8], len_added: usize) -> Result<()> {
- let write_result = self.buffer.write(bytes);
- // `io::Result` has many options one of which we use, so pattern
matching is
- // overkill here
- if write_result.is_err() {
- Err(ArrowError::MemoryError(
- "Could not write to Buffer, not big enough".to_string(),
- ))
- } else {
- self.len += len_added;
- Ok(())
- }
+ fn write_bytes(&mut self, bytes: &[u8], len_added: usize) {
Review comment:
I agree with @jhorstmann here that we should remove `len_added` (maybe
as another PR) -- the length that is actually added is the `bytes.len()` rather
than `len_added` so having the caller have to provide both leaves the
opportunity for additional latent bugs
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
