alamb commented on code in PR #11324:
URL: https://github.com/apache/arrow-rs/pull/11324#discussion_r4178155759
##########
arrow-ord/src/partition.rs:
##########
@@ -166,6 +169,46 @@ fn find_boundaries(v: &dyn Array) -> Result<BooleanBuffer,
ArrowError> {
Ok((0..slice_len).map(|i| !cmp(i, i).is_eq()).collect())
}
+fn convert_boolean_buffer_to_builder(buffer: BooleanBuffer) ->
BooleanBufferBuilder {
+ let len = buffer.len();
+
+ // This should be 0, but in case it doesn't simplify the code by doing a
copy to a new buffer
Review Comment:
I don't understand the comment about why it doesn't simplify the code. Is it
because the builder can't handle non zero offsets?
##########
arrow-ord/src/partition.rs:
##########
@@ -166,6 +169,46 @@ fn find_boundaries(v: &dyn Array) -> Result<BooleanBuffer,
ArrowError> {
Ok((0..slice_len).map(|i| !cmp(i, i).is_eq()).collect())
}
+fn convert_boolean_buffer_to_builder(buffer: BooleanBuffer) ->
BooleanBufferBuilder {
+ let len = buffer.len();
+
+ // This should be 0, but in case it doesn't simplify the code by doing a
copy to a new buffer
+ let buffer = if buffer.offset() != 0 {
+ // Recreate
+ BooleanBuffer::from_iter(buffer.iter())
+ } else {
+ buffer
+ };
+ assert_eq!(buffer.offset(), 0);
+
+ let buffer = buffer.into_inner();
+
+ let mutable = buffer
+ .into_mutable()
+ // This should not happen but just in case copy to owned mutable buffer
+ .unwrap_or_else(|buffer| {
+ let mut mutable = MutableBuffer::new(buffer.len());
+ // SAFETY: We just allocated a buffer of the same length as the
original buffer, so it is safe to set the length to the original buffer's
length.
+ unsafe { mutable.set_len(buffer.len()) };
+ mutable.as_slice_mut().copy_from_slice(buffer.as_slice());
+
+ mutable
+ });
+
+ BooleanBufferBuilder::new_from_buffer(mutable, len)
+}
+
+fn or(lhs: &mut BooleanBufferBuilder, rhs: BooleanBuffer) {
+ apply_bitwise_binary_op(
+ lhs.as_slice_mut(),
+ 0,
Review Comment:
You know this is zero because the builder always has zero offset?
##########
arrow-ord/src/partition.rs:
##########
@@ -143,12 +144,14 @@ pub fn partition(columns: &[ArrayRef]) ->
Result<Partitions, ArrowError> {
}
let acc = find_boundaries(&columns[0])?;
- let acc = columns
- .iter()
- .skip(1)
- .try_fold(acc, |acc, c| find_boundaries(c.as_ref()).map(|b| &acc |
&b))?;
+ let mut builder = convert_boolean_buffer_to_builder(acc);
- Ok(Partitions(Some(acc)))
+ for column in columns.iter().skip(1) {
+ let boundaries = find_boundaries(column.as_ref())?;
Review Comment:
Here could weconvert back to boolean buffer, call `or` and then recreate the
builder? I do agree that having a native `or` implementation on the builder
would make the code simpler
##########
arrow-ord/src/partition.rs:
##########
@@ -166,6 +169,46 @@ fn find_boundaries(v: &dyn Array) -> Result<BooleanBuffer,
ArrowError> {
Ok((0..slice_len).map(|i| !cmp(i, i).is_eq()).collect())
}
+fn convert_boolean_buffer_to_builder(buffer: BooleanBuffer) ->
BooleanBufferBuilder {
+ let len = buffer.len();
+
+ // This should be 0, but in case it doesn't simplify the code by doing a
copy to a new buffer
+ let buffer = if buffer.offset() != 0 {
+ // Recreate
+ BooleanBuffer::from_iter(buffer.iter())
+ } else {
+ buffer
+ };
+ assert_eq!(buffer.offset(), 0);
+
+ let buffer = buffer.into_inner();
+
+ let mutable = buffer
+ .into_mutable()
+ // This should not happen but just in case copy to owned mutable buffer
+ .unwrap_or_else(|buffer| {
+ let mut mutable = MutableBuffer::new(buffer.len());
+ // SAFETY: We just allocated a buffer of the same length as the
original buffer, so it is safe to set the length to the original buffer's
length.
+ unsafe { mutable.set_len(buffer.len()) };
+ mutable.as_slice_mut().copy_from_slice(buffer.as_slice());
+
+ mutable
+ });
+
+ BooleanBufferBuilder::new_from_buffer(mutable, len)
+}
+
+fn or(lhs: &mut BooleanBufferBuilder, rhs: BooleanBuffer) {
+ apply_bitwise_binary_op(
+ lhs.as_slice_mut(),
+ 0,
+ rhs.values(),
+ rhs.offset(),
+ rhs.len(),
+ |a, b| a | b,
+ )
+}
Review Comment:
Maybe it is time to add the API to `BooleanBufferBuilder` -- with a usecase
it seems reasonable to me
We could also note the differences potentially as a comment
--
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]
