ARROW-10136: [Rust]: Fix null handling in StringArray and BinaryArray filtering, add BinaryArray::from_opt_vec #8303

alamb · 2020-09-29T17:48:50Z

When I use the filter kernel with Null strings, any input column that was Null turns into an empty string after filtering.

"foo"
"bar"
NULL

And the filter

true
true
true

Will result in

"foo"
"bar"
""

Rather than

"foo"
"bar"
NULL

It appears to work fine for primitive arrays (I'll comment inline). I also added BinaryArray::from_opt_vec following the model of PrimativeArray and StringArray mostly so I could write a test.

github-actions · 2020-09-29T18:06:22Z

https://issues.apache.org/jira/browse/ARROW-10136

alamb · 2020-09-29T17:53:49Z

rust/arrow/src/compute/kernels/filter.rs

@@ -353,15 +353,19 @@ impl FilterContext {
                        // foreach bit in batch:
                        if (filter_batch & self.filter_mask[j]) != 0 {
                            let data_index = (i * 64) + j;
-                            values.push(input_array.value(data_index));
+                            if input_array.is_null(data_index) {


This is the same pattern as in the handling for primative array: https://github.com/apache/arrow/pull/8303/files#diff-d7b0b7cde1850e8744ceda458c6dea81R294-L298

(this is the code for BinaryArray that @nevi-me referred to in #8303 (comment))

alamb · 2020-09-29T17:54:05Z

rust/arrow/src/compute/kernels/filter.rs

@@ -373,7 +377,11 @@ impl FilterContext {
                        // foreach bit in batch:
                        if (filter_batch & self.filter_mask[j]) != 0 {
                            let data_index = (i * 64) + j;
-                            values.push(input_array.value(data_index));
+                            if input_array.is_null(data_index) {


Likewise, this special case appears to miss the null check too

alamb · 2020-09-29T18:06:39Z

rust/arrow/src/compute/kernels/filter.rs

                        }
                    }
                }
                Ok(Arc::new(BinaryArray::from(values)))
            }
            DataType::Utf8 => {
                let input_array = array.as_any().downcast_ref::<StringArray>().unwrap();
-                let mut values: Vec<&str> = Vec::with_capacity(self.filtered_count);
+                let mut values: Vec<Option<&str>> = Vec::with_capacity(self.filtered_count);


Note using an Option is likely to increase the temporary storage requirements a bit.

It would likely be possible to avoid this allocation entirely if we used the lower level ArrayBuilder::with_bit_buffer.

I chose to follow the style of the rest of this module, though I would love opinions on trying to perf check this / optimize it (maybe a follow on JIRA ticket is enough)?

Doesn't an Option of a reference leverages null pointer optimization (on which None is represented by the null pointer, e.g. rust-lang/rust#9378)?

IMO we should follow up on this: for kernels we have been using a mutable buffer with null masks as much as possible.

Doesn't an Option of a reference leverages null pointer optimization (on which None is represented by the null pointer, e.g. rust-lang/rust#9378)?

Yes, I believe you are correct.

This program:

fn main() { println!("The size of a &str is {}", std::mem::size_of::<&str>()); println!("The size of an Option<&str> is {}", std::mem::size_of::<Option<&str>>()); }

Produces the following on my machine:

The size of a &str is 16 The size of an Option<&str> is 16

nevi-me · 2020-09-29T23:19:06Z

Thanks @alamb, this existed as https://issues.apache.org/jira/browse/ARROW-5352; so I'll close that out.

The same behaviour would occur with a BinaryArray, I haven't looked at this PR, but it's worthwhile to ensure that StringArray and BinaryArray are treated consistently.

And yes, the issue didn't affect primitive arrays, I think it was because we were pushing an empty string where we should have pushed a null slot on the null bitmap

jorgecarleitao

Thanks a lot, @alamb . Looks good so far. I left some minor comments.

jorgecarleitao · 2020-09-30T02:14:35Z

rust/arrow/src/compute/kernels/filter.rs

                        }
                    }
                }
                Ok(Arc::new(BinaryArray::from(values)))
            }
            DataType::Utf8 => {
                let input_array = array.as_any().downcast_ref::<StringArray>().unwrap();
-                let mut values: Vec<&str> = Vec::with_capacity(self.filtered_count);
+                let mut values: Vec<Option<&str>> = Vec::with_capacity(self.filtered_count);


Doesn't an Option of a reference leverages null pointer optimization (on which None is represented by the null pointer, e.g. rust-lang/rust#9378)?

IMO we should follow up on this: for kernels we have been using a mutable buffer with null masks as much as possible.

jorgecarleitao · 2020-09-30T02:15:10Z

rust/arrow/src/array/array.rs

@@ -1267,6 +1267,36 @@ impl<OffsetSize: OffsetSizeTrait> GenericBinaryArray<OffsetSize> {
        GenericBinaryArray::<OffsetSize>::from(array_data)
    }

+    fn from_opt_vec(v: Vec<Option<&[u8]>>, data_type: DataType) -> Self {


is this code tested somewhere?

It is tested (indirectly) in https://github.com/apache/arrow/pull/8303/files#diff-d7b0b7cde1850e8744ceda458c6dea81R700 -- but I think a more specific test would be valuable. I will add one.

This turned out to be a great call @jorgecarleitao -- I found a bug in this implementation while writing a test. Thank you for the suggestion. 💯

jorgecarleitao · 2020-09-30T02:20:17Z

rust/arrow/src/compute/kernels/filter.rs

+        let c = filter(&a, &b).unwrap();
+        let d = c.as_ref().as_any().downcast_ref::<StringArray>().unwrap();
+        assert_eq!(2, d.len());
+        assert_eq!("hello", d.value(0));


I suggest that we test the 3 quantities: d.is_null(0), d.value(0), d.is_null(1). Alternatively,

let expected = StringArray::from(vec![Some("hello"), None]); assert_eq!(d, expected);

jorgecarleitao · 2020-09-30T02:20:56Z

rust/arrow/src/compute/kernels/filter.rs

+        let c = filter(&a, &b).unwrap();
+        let d = c.as_ref().as_any().downcast_ref::<BinaryArray>().unwrap();
+        assert_eq!(2, d.len());
+        assert_eq!(b"hello", d.value(0));


alamb · 2020-09-30T10:00:35Z

The same behaviour would occur with a BinaryArray, I haven't looked at this PR, but it's worthwhile to ensure that StringArray and BinaryArray are treated consistently.

Thanks @nevi-me -- yes I updated the code tried to handle both types in the same way (and including tests)

alamb · 2020-09-30T10:25:20Z

Doesn't an Option of a reference leverages null pointer optimization (on which None is represented by the null pointer, e.g. rust-lang/rust#9378)?

@jorgecarleitao I believe you are correct:

This program:

fn main() {
    println!("The size of a &str is {}", std::mem::size_of::<&str>());
    println!("The size of an Option<&str> is {}", std::mem::size_of::<Option<&str>>());
}

Produces the following on my machine:

The size of a &str is 16
The size of an Option<&str> is 16

alamb

@jorgecarleitao you said:

IMO we should follow up on this: for kernels we have been using a mutable buffer with null masks as much as possible.

Can you perhaps let me know what you mean by this? Perhaps there is an example in the code you are thinking of?

alamb · 2020-09-30T10:27:12Z

rust/arrow/src/array/array.rs

@@ -1267,6 +1267,36 @@ impl<OffsetSize: OffsetSizeTrait> GenericBinaryArray<OffsetSize> {
        GenericBinaryArray::<OffsetSize>::from(array_data)
    }

+    fn from_opt_vec(v: Vec<Option<&[u8]>>, data_type: DataType) -> Self {


It is tested (indirectly) in https://github.com/apache/arrow/pull/8303/files#diff-d7b0b7cde1850e8744ceda458c6dea81R700 -- but I think a more specific test would be valuable. I will add one.

alamb · 2020-09-30T10:28:40Z

rust/arrow/src/compute/kernels/filter.rs

                        }
                    }
                }
                Ok(Arc::new(BinaryArray::from(values)))
            }
            DataType::Utf8 => {
                let input_array = array.as_any().downcast_ref::<StringArray>().unwrap();
-                let mut values: Vec<&str> = Vec::with_capacity(self.filtered_count);
+                let mut values: Vec<Option<&str>> = Vec::with_capacity(self.filtered_count);


Doesn't an Option of a reference leverages null pointer optimization (on which None is represented by the null pointer, e.g. rust-lang/rust#9378)?

Yes, I believe you are correct.

This program:

fn main() { println!("The size of a &str is {}", std::mem::size_of::<&str>()); println!("The size of an Option<&str> is {}", std::mem::size_of::<Option<&str>>()); }

Produces the following on my machine:

The size of a &str is 16 The size of an Option<&str> is 16

alamb · 2020-09-30T11:04:08Z

I think I have addressed all your (very helpful) comments @jorgecarleitao .

jorgecarleitao · 2020-09-30T11:21:21Z

@jorgecarleitao you said:

IMO we should follow up on this: for kernels we have been using a mutable buffer with null masks as much as possible.

Can you perhaps let me know what you mean by this? Perhaps there is an example in the code you are thinking of?

I am really, sorry, @alamb , I should have offered more context in the first place. :/

This in no way blocks this PR: IMO it is ready to merge if the relevant tests pass.

What I meant is that this code currently:

creates Vec<Option<T>> through an iteration
copies Vec<Option<T>> to the two buffers (when from_opt_vec is called)

it may be more efficient to create the buffers during the iteration, so that we avoid the copy (Vec -> buffers). In other words, the code in from_opt_vec could have been "injected" into the filter execution, where the MuttableBuffer and offsets and values buffer are created before the loop, and new elements are directly written to it. Does this any sense?

(as a side note, this is why I am proposing #8211 : IMO there is some boiler-plate copy-pasting to

initialize buffers
iterate
create ArrayData from buffers

which will continue to grow as we add more kernels, and whose pattern seems to be a FromIter of fixed size)

jorgecarleitao

LGTM, really nice additions.

alamb · 2020-09-30T11:39:05Z

@jorgecarleitao -- yes thank you that makes a lot of sense. I have filed https://issues.apache.org/jira/browse/ARROW-10141 to track that

alamb · 2020-10-01T11:12:54Z

@andygrove / @jorgecarleitao / @nevi-me I wonder if this PR might be merged anytime soon (I have a downstream project relying on this change)

The integration test failure https://github.com/apache/arrow/pull/8303/checks?check_run_id=1187275161 seems due to a network failure (not anything with this PR):

Error:  Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.5.1:attach-descriptor (attach-descriptor) on project arrow-java-root: Execution attach-descriptor of goal org.apache.maven.plugins:maven-site-plugin:3.5.1:attach-descriptor failed: Plugin org.apache.maven.plugins:maven-site-plugin:3.5.1 or one of its dependencies could not be resolved: Could not transfer artifact org.apache.maven:maven-archiver:jar:2.5 from/to central (https://repo.maven.apache.org/maven2): Connection reset -> [Help 1]

alamb · 2020-10-01T12:51:30Z

I'll rebase to try and get a clean test run

…ernels

alamb · 2020-10-01T18:46:55Z

All tests are passing now. 🎉

… filtering, add BinaryArray::from_opt_vec When I use the `filter` kernel with Null strings, any input column that was Null turns into an empty string after filtering. ``` "foo" "bar" NULL ``` And the filter ``` true true true ``` Will result in ``` "foo" "bar" "" ``` Rather than ``` "foo" "bar" NULL ``` It appears to work fine for primitive arrays (I'll comment inline). I also added `BinaryArray::from_opt_vec` following the model of `PrimativeArray` and `StringArray` mostly so I could write a test. Closes apache#8303 from alamb/alamb/ARROW-10136-null-filter Authored-by: alamb <andrew@nerdnetworks.org> Signed-off-by: Andy Grove <andygrove73@gmail.com>

alamb commented Sep 29, 2020

View reviewed changes

nevi-me self-requested a review September 29, 2020 23:19

nevi-me added the Component: Rust label Sep 29, 2020

nevi-me changed the title ~~ARROW-10136: [Rust][Arrow]: Fix null handling in StringArray and BinaryArray filtering, add BinaryArray::from_opt_vec~~ ARROW-10136: [Rust]: Fix null handling in StringArray and BinaryArray filtering, add BinaryArray::from_opt_vec Sep 29, 2020

jorgecarleitao reviewed Sep 30, 2020

View reviewed changes

alamb commented Sep 30, 2020

View reviewed changes

jorgecarleitao approved these changes Sep 30, 2020

View reviewed changes

alamb mentioned this pull request Sep 30, 2020

ARROW-10030: [Rust] Add support for FromIter and IntoIter for primitive types #8211

Closed

alamb added 3 commits October 1, 2020 08:52

ARROW-10136: [Rust][DataFusion]: Fix null handling in string filter k…

18efc72

…ernels

Add additional coverage for is_null filter

036eca9

Add test for {Large,}BinaryArray::from_opt_vec

b83d867

alamb force-pushed the alamb/ARROW-10136-null-filter branch from 53f04f0 to b83d867 Compare October 1, 2020 12:52

andygrove closed this in a1157b7 Oct 1, 2020

alamb deleted the alamb/ARROW-10136-null-filter branch October 1, 2020 18:56

asfimport mentioned this pull request Oct 2, 2020

[Rust][Arrow] Nulls are transformed into "" after filtering for StringArray #26147

Closed

ARROW-10136: [Rust]: Fix null handling in StringArray and BinaryArray filtering, add BinaryArray::from_opt_vec #8303

ARROW-10136: [Rust]: Fix null handling in StringArray and BinaryArray filtering, add BinaryArray::from_opt_vec #8303

Uh oh!

Conversation

alamb commented Sep 29, 2020

Uh oh!

github-actions bot commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nevi-me commented Sep 29, 2020

Uh oh!

jorgecarleitao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 30, 2020

Uh oh!

alamb commented Sep 30, 2020

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 30, 2020

Uh oh!

jorgecarleitao commented Sep 30, 2020

Uh oh!

jorgecarleitao left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Sep 30, 2020

Uh oh!

alamb commented Oct 1, 2020

Uh oh!

alamb commented Oct 1, 2020

Uh oh!

alamb commented Oct 1, 2020

Uh oh!

Uh oh!