You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/09/26 01:54:32 UTC
[GitHub] [arrow] nevi-me opened a new pull request #8280: ARROW-10103: [Rust] Add contains kernel
nevi-me opened a new pull request #8280:
URL: https://github.com/apache/arrow/pull/8280
Checks if a list contains a value in either a primitive or string
Large lists are also supported
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495433674
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
Review comment:
The sql behaviour regarding nulls would be different, if that is intended it should probably be noted a bit more prominently. In Postgres:
```sql
SELECT
'foo' = ANY(ARRAY['foo','bar']::text[]) AS non_null,
'foo' = ANY(ARRAY[]::text[]) AS empty,
'foo' = ANY(ARRAY[NULL]::text[]) AS null_value,
'foo' = ANY(NULL::text[]) AS null_array
```
```
| non_null | empty | null_value | null_array |
| -------- | ----- | ---------- | ---------- |
| true | false | | |
```
(empty cells in that table being `null` values)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-699615864
> Given a list array, return true if a non-null value exists in the array.
Intuitively this makes sense, unfortunately the sql rules are slightly more complex and can also return null in several cases. I'm ok with leaving it like this for now, but we might need to change it later to align more closely with sql.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8280:
URL: https://github.com/apache/arrow/pull/8280
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-699468060
@jhorstmann @jorgecarleitao I've had a look at this with a fresh head, and I've simplified the logic to:
Given a list array, return true if a non-null value exists in the array.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495375649
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +557,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
+ }
+ } else {
+ let list = list.as_any().downcast_ref::<PrimitiveArray<T>>().unwrap();
+
+ for j in 0..list.len() {
+ if list.is_valid(j) && (left.value(i) == list.value(j)) {
+ is_in = true;
+ }
+ }
+ }
+ }
+ result.append(is_in)?;
+ }
+
+ let data = ArrayData::new(
+ DataType::Boolean,
+ left.len(),
+ None,
+ None,
+ left.offset(),
+ vec![result.finish()],
+ vec![],
+ );
+ Ok(PrimitiveArray::<BooleanType>::from(Arc::new(data)))
+}
+
+/// Checks if a `GenericListArray` contains a value in the `GenericStringArray`
+pub fn contains_utf8<OffsetSize>(
+ left: &GenericStringArray<OffsetSize>,
+ right: &ListArray,
+) -> Result<BooleanArray>
+where
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
+ }
+ } else {
+ let list = list
+ .as_any()
+ .downcast_ref::<GenericStringArray<OffsetSize>>()
+ .unwrap();
+
+ for j in 0..list.len() {
+ if list.is_valid(j) && (left.value(i) == list.value(j)) {
+ is_in = true;
+ }
+ }
+ }
+ }
+ result.append(is_in)?;
+ }
+
+ let data = ArrayData::new(
+ DataType::Boolean,
+ left.len(),
+ None,
+ None,
+ left.offset(),
Review comment:
Is it correct for us to reuse the offset, or should this be 0? My intuition says the latter, but I'm too tired to figure it out. Same applies above
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495431850
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
+ }
+ } else {
+ let list = list.as_any().downcast_ref::<PrimitiveArray<T>>().unwrap();
+
+ for j in 0..list.len() {
+ if list.is_valid(j) && (left.value(i) == list.value(j)) {
+ is_in = true;
+ }
+ }
+ }
+ }
+ result.append(is_in)?;
+ }
+
+ let data = ArrayData::new(
+ DataType::Boolean,
+ left.len(),
+ None,
+ None,
+ left.offset(),
Review comment:
Offset should be 0 since the result is a new buffer that only contains the relevant data
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] velvia commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
velvia commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-701046636
@nevi-me Hi there, I'm teammates with @mcassels and @maxburke at UrbanLogiq, and we'd like to check in on the status of this PR. From looking over it, this is what I can gleam (the PR desc is just one sentence):
* This just has kernel primitives for contains for generic strings and primitives
* No specific support for dictionary-encoded string arrays? I guess this could be built on the primitive kernel?
* Is a separate PR for the rest of the support, ie logical plan, etc. coming? Would be interested in building on top of this to finish the PR as we would like to use it on a newer branch.
(we'd be happy to finish the rest of it or work on the follow on PR)
Would love to have an ETA....
Thanks!
-Evan
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495436762
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
Review comment:
PTAL at how I've done it now. Instead of using a booelan builder, I set the bits directly on the buffer like we did with `take`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495435647
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
Review comment:
I think the test is
```sql
SELECT
'foo' = ANY(ARRAY['foo','bar']::text[]) AS non_null,
'foo' = ANY(ARRAY[]::text[]) AS empty,
NULL::text = ANY(ARRAY[NULL, 'foo']::text[]) AS null_value,
'foo' = ANY(NULL::text[]) AS null_array
```
but nonetheless it still yields the same result.
I copied this from another PR mostly-verbatim, so I'm only scrutinising the code today myself.
I would prefer aligning with the SQL behaviour, that a null value can't be contained in an array if the array has nulls.
If users want to find out if an array has nulls, they can use the below or something better:
```rust
let array: ListArray<i32> = ...;
let ceil = bit_util::ceil(array.len(), 8);
let mut bools = MutableBuffer::new(ceil).
for i in -..array.len() {
if array.is_valid(i) && array.value(i).null_count() > 0 {
bools.set_bit(i);
}
}
// create bool array from bools
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495394022
##########
File path: rust/arrow/src/compute/util.rs
##########
@@ -71,6 +71,47 @@ pub(super) fn combine_option_bitmap(
}
}
+/// Compares the null bitmaps of two arrays using a bitwise `or` operation.
+///
+/// This function is useful when implementing operations on higher level arrays.
+pub(super) fn compare_option_bitmap(
+ left_data: &ArrayDataRef,
+ right_data: &ArrayDataRef,
+ len_in_bits: usize,
+) -> Result<Option<Buffer>> {
+ let left_offset_in_bits = left_data.offset();
+ let right_offset_in_bits = right_data.offset();
+
+ let left = left_data.null_buffer();
+ let right = right_data.null_buffer();
+
+ if (left.is_some() && left_offset_in_bits % 8 != 0)
Review comment:
Out of curiosity: is this true in general, or is for these (this and above) implementation, that uses this assumption?
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +557,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
+ }
+ } else {
+ let list = list.as_any().downcast_ref::<PrimitiveArray<T>>().unwrap();
+
+ for j in 0..list.len() {
+ if list.is_valid(j) && (left.value(i) == list.value(j)) {
+ is_in = true;
+ }
+ }
+ }
+ }
+ result.append(is_in)?;
+ }
+
+ let data = ArrayData::new(
+ DataType::Boolean,
+ left.len(),
+ None,
+ None,
+ left.offset(),
+ vec![result.finish()],
+ vec![],
+ );
+ Ok(PrimitiveArray::<BooleanType>::from(Arc::new(data)))
+}
+
+/// Checks if a `GenericListArray` contains a value in the `GenericStringArray`
+pub fn contains_utf8<OffsetSize>(
+ left: &GenericStringArray<OffsetSize>,
+ right: &ListArray,
+) -> Result<BooleanArray>
+where
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
+ }
+ } else {
+ let list = list
+ .as_any()
+ .downcast_ref::<GenericStringArray<OffsetSize>>()
+ .unwrap();
+
+ for j in 0..list.len() {
+ if list.is_valid(j) && (left.value(i) == list.value(j)) {
+ is_in = true;
+ }
+ }
+ }
+ }
+ result.append(is_in)?;
+ }
+
+ let data = ArrayData::new(
+ DataType::Boolean,
+ left.len(),
+ None,
+ None,
+ left.offset(),
Review comment:
I also think that this should be zero: we are building a new buffer (`result.finish()`) and thus there is no need to start from an offset. I would expect that a test with `left.offset() != 0` to not pass with this code, as we read `result`'s buffer from left's offset.
##########
File path: rust/arrow/src/compute/util.rs
##########
@@ -71,6 +71,47 @@ pub(super) fn combine_option_bitmap(
}
}
+/// Compares the null bitmaps of two arrays using a bitwise `or` operation.
+///
+/// This function is useful when implementing operations on higher level arrays.
+pub(super) fn compare_option_bitmap(
Review comment:
tests missing, I think.
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
Review comment:
I think that we can short-circuit here (`result.append(true)?; break`) (and equivalent instances). In rust's notation, I think that we are using the [`Iterator::any`](https://doc.rust-lang.org/stable/rust-by-example/fn/closures/closure_examples/iter_any.html) with some null logic sprinkled on top.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] maxburke commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
maxburke commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-701902290
> I've spent some time looking at the UL fork, and I think there might be changes there that the wider community would benefit from if you upstream them a bit more frequently. I understand that sometimes we might take longer than is ideal to complete PR reviews and merge them; but that's often a function of the sporadic availability of capacity from the Rust developers.
We would love to get our changes into the mainline Arrow, and that is our goal!, we've just been very constriained lately in how much people-power we can use to get the patches into a state our committers are happy with :slightly_smiling_face:
Slowly but surely we'll have more patches coming!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-699272912
https://issues.apache.org/jira/browse/ARROW-10103
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-701050053
Hi @velvia, I pulled these changes from UL's fork while I was rebasing the changes from #6770. I only refactored the existing kernel functions there, but didn't expand any supported data types.
The behaviour does change from what you had on your fork (https://github.com/apache/arrow/pull/8280#discussion_r495433674), so perhaps you can comment on whether this is fine, or bear that in mind when updating your fork.
Regarding an ETA, the PR's still under review, so once it's approved, we'll be able to merge it in.
Yes, you can open JIRAs (https://issues.apache.org/jira/projects/ARROW) then work on top of this PR.
I've spent some time looking at the UL fork, and I think there might be changes there that the wider community would benefit from if you upstream them a bit more frequently. I understand that sometimes we might take longer than is ideal to complete PR reviews and merge them; but that's often a function of the sporadic availability of capacity from the Rust developers.
We've also been very reliant on 2 people on the Parquet side, but I've been spending more time on the codebase so I can start picking up PRs on Parquet.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-699270344
This is extracted from #6770,
PTAL @alamb @jorgecarleitao @jhorstmann
CC @mcassels @maxburke
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495432552
##########
File path: rust/arrow/src/compute/util.rs
##########
@@ -71,6 +71,47 @@ pub(super) fn combine_option_bitmap(
}
}
+/// Compares the null bitmaps of two arrays using a bitwise `or` operation.
+///
+/// This function is useful when implementing operations on higher level arrays.
+pub(super) fn compare_option_bitmap(
+ left_data: &ArrayDataRef,
+ right_data: &ArrayDataRef,
+ len_in_bits: usize,
+) -> Result<Option<Buffer>> {
+ let left_offset_in_bits = left_data.offset();
+ let right_offset_in_bits = right_data.offset();
+
+ let left = left_data.null_buffer();
+ let right = right_data.null_buffer();
+
+ if (left.is_some() && left_offset_in_bits % 8 != 0)
Review comment:
I'm trying to solve this problem in #8262
The issue is that `buffer.slice` and `buffer_bin_or/and` currently work with byte offsets, but for boolean arrays and bitmaps the offset can start in the middle of a byte.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495436864
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
+ }
+ } else {
+ let list = list.as_any().downcast_ref::<PrimitiveArray<T>>().unwrap();
+
+ for j in 0..list.len() {
+ if list.is_valid(j) && (left.value(i) == list.value(j)) {
+ is_in = true;
+ }
+ }
+ }
+ }
+ result.append(is_in)?;
+ }
+
+ let data = ArrayData::new(
+ DataType::Boolean,
+ left.len(),
+ None,
+ None,
+ left.offset(),
+ vec![result.finish()],
+ vec![],
+ );
+ Ok(PrimitiveArray::<BooleanType>::from(Arc::new(data)))
+}
+
+/// Checks if a `GenericListArray` contains a value in the `GenericStringArray`
+pub fn contains_utf8<OffsetSize>(
+ left: &GenericStringArray<OffsetSize>,
+ right: &ListArray,
+) -> Result<BooleanArray>
+where
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
Review comment:
Changed, ta
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495432926
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
Review comment:
Here again we'd need to consider the offset of `left`, or use `left.is_valid` instead
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] velvia commented on pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
velvia commented on pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#issuecomment-701541243
@nevi-me thanks, will have a look
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495436663
##########
File path: rust/arrow/src/compute/util.rs
##########
@@ -71,6 +71,47 @@ pub(super) fn combine_option_bitmap(
}
}
+/// Compares the null bitmaps of two arrays using a bitwise `or` operation.
+///
+/// This function is useful when implementing operations on higher level arrays.
+pub(super) fn compare_option_bitmap(
Review comment:
Added, I ended up not using `compare_option_bitmap`, but it's a useful function to have
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8280: ARROW-10103: [Rust] Add contains kernel
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on a change in pull request #8280:
URL: https://github.com/apache/arrow/pull/8280#discussion_r495432114
##########
File path: rust/arrow/src/compute/kernels/comparison.rs
##########
@@ -555,11 +555,159 @@ where
compare_op_scalar!(left, right, |a, b| a >= b)
}
+/// Checks if a `GenericListArray` contains a value in the `PrimitiveArray`
+pub fn contains<T, OffsetSize>(
+ left: &PrimitiveArray<T>,
+ right: &GenericListArray<OffsetSize>,
+) -> Result<BooleanArray>
+where
+ T: ArrowNumericType,
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
+ };
+ let not_both_null_bitmap = not_both_null_bit_buffer.data();
+
+ let left_data = left.data();
+ let left_null_bitmap = match left_data.null_bitmap() {
+ Some(bitmap) => bitmap.clone().into_buffer(),
+ _ => new_all_set_buffer(left.len()),
+ };
+ let left_null_bitmap = left_null_bitmap.data();
+
+ let mut result = BooleanBufferBuilder::new(left.len());
+
+ for i in 0..left.len() {
+ let mut is_in = false;
+
+ // contains(null, null) = false
+ if bit_util::get_bit(not_both_null_bitmap, i) {
+ let list = right.value(i);
+
+ // contains(null, [null]) = true
+ if !bit_util::get_bit(left_null_bitmap, i) {
+ if list.null_count() > 0 {
+ is_in = true;
+ }
+ } else {
+ let list = list.as_any().downcast_ref::<PrimitiveArray<T>>().unwrap();
+
+ for j in 0..list.len() {
+ if list.is_valid(j) && (left.value(i) == list.value(j)) {
+ is_in = true;
+ }
+ }
+ }
+ }
+ result.append(is_in)?;
+ }
+
+ let data = ArrayData::new(
+ DataType::Boolean,
+ left.len(),
+ None,
+ None,
+ left.offset(),
+ vec![result.finish()],
+ vec![],
+ );
+ Ok(PrimitiveArray::<BooleanType>::from(Arc::new(data)))
+}
+
+/// Checks if a `GenericListArray` contains a value in the `GenericStringArray`
+pub fn contains_utf8<OffsetSize>(
+ left: &GenericStringArray<OffsetSize>,
+ right: &ListArray,
+) -> Result<BooleanArray>
+where
+ OffsetSize: OffsetSizeTrait,
+{
+ if left.len() != right.len() {
+ return Err(ArrowError::ComputeError(
+ "Cannot perform comparison operation on arrays of different length"
+ .to_string(),
+ ));
+ }
+
+ let not_both_null_bit_buffer =
+ match compare_option_bitmap(left.data_ref(), right.data_ref(), left.len())? {
+ Some(buff) => buff,
+ None => new_all_set_buffer(left.len()),
Review comment:
I think this is creating a buffer that is larger than needed since it is a bitmap storing 8 values per byte. Correct size would be `ceil(left.len(), 8)`
----------------------------------------------------------------
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:
users@infra.apache.org