Add FixedSizeBinaryArray::try_from_sparse_iter_with_size #3054

maxburke · 2022-11-09T00:42:19Z

Rationale for this change

FixedSizeBinaryArray::try_from_sparse_iter generates incorrect results when the provided iterator only produces None values.

What changes are included in this PR?

This change also implements the comparison kernel for FixedSizeBinary types.

Are there any user-facing changes?

This change adds a new function, FixedSizeBinaryArray::try_from_iter_with_size, which takes a default size parameter, and markes the existing try_from_sparse_iter as deprecated.

…lid array when the iterator only produces None values

tustvold

Left some comments

tustvold · 2022-11-10T21:52:24Z

arrow-array/src/array/fixed_size_binary_array.rs

+
+    pub fn try_from_sparse_iter_with_size<T, U>(
+        mut iter: T,
+        asserted_size: Option<i32>,


I don't follow the need for a separate asserted_size and detected_size. Looking at the logic I think we could just have mut size: Option<i32> and everything would work correctly?

I think if we were to just have a single size option we would need to have separate implementations of try_from_sparse_iter and try_from_sparse_iter_with_size.

(IMO, I think the former is problematic and should be deprecated / removed)

I don't see why you need to distinguish between the two different sizes in the implementation, it doesn't change if the size is determined by being passed in or from the first non-null element. They must all be equal regardless?

I separated the instances of try_from_sparse_iter and try_from_sparse_iter_with_size because I think the corner cases of making the two implementations match up wasn't worth the complexity.

tustvold · 2022-11-10T21:52:34Z

arrow/src/compute/kernels/comparison.rs

@@ -2270,6 +2270,18 @@ macro_rules! typed_compares {
                as_largestring_array($RIGHT),
                $OP,
            ),
+            (DataType::FixedSizeBinary(_), DataType::FixedSizeBinary(_)) => {


This change does not appear to be related?

Yeah, it's not, but it was something we were hoping to upstream and I think it accidentally fell into the branch. Would you like me to update this PR to mention this change?

I think either we need tests of it, or we need to roll it back

Tests added.

tustvold · 2022-11-10T21:53:42Z

arrow-array/src/array/fixed_size_binary_array.rs

+        Self::try_from_sparse_iter_with_size(iter, None)
+    }
+
+    pub fn try_from_sparse_iter_with_size<T, U>(


I think a doc comment similar to the one above would be good, especially to explain what the size argument is

tustvold · 2022-11-10T21:56:12Z

arrow-array/src/array/fixed_size_binary_array.rs

+
+    pub fn try_from_sparse_iter_with_size<T, U>(


Suggested change

pub fn try_from_sparse_iter_with_size<T, U>(

pub fn try_from_sparse_iter_with_size<T, U>(iter: T, size: i32) -> Result<Self, ArrowError> {

try_from_sparse_iter_with_size_opt::<T, U>(iter, Some(size))

}

fn try_from_sparse_iter_with_size_opt<T, U>(

I think this makes for better ergonomics

I made the parameter Optional instead

alamb · 2022-11-11T13:53:50Z

If we can get this PR merged in the next 4-5 hours it will be included in arrow 27.0.0.

…onal

…dd docstring

maxburke · 2022-11-11T23:45:57Z

If we can get this PR merged in the next 4-5 hours it will be included in arrow 27.0.0.

rats

alamb · 2022-11-12T13:33:16Z

If we can get this PR merged in the next 4-5 hours it will be included in arrow 27.0.0.

😢

viirya · 2022-11-12T21:39:54Z

But there are a few places using the deprecated function, see CI.

error: use of deprecated associated function `arrow_array::FixedSizeBinaryArray::try_from_sparse_iter`: This function will fail if the iterator produces only None values; prefer `try_from_sparse_iter_with_size`
   --> arrow/src/array/ffi.rs:215:43
    |
215 |         let array = FixedSizeBinaryArray::try_from_sparse_iter(values.into_iter())?;
    |                                           ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

maxburke · 2022-11-12T21:46:36Z

But there are a few places using the deprecated function, see CI.

error: use of deprecated associated function `arrow_array::FixedSizeBinaryArray::try_from_sparse_iter`: This function will fail if the iterator produces only None values; prefer `try_from_sparse_iter_with_size`
   --> arrow/src/array/ffi.rs:215:43
    |
215 |         let array = FixedSizeBinaryArray::try_from_sparse_iter(values.into_iter())?;
    |                                           ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

Yup, just saw the build failure and submitted a fix. I wasn't testing locally with the ffi feature enabled.

viirya · 2022-11-12T22:27:52Z

There are some clippy errors that are not related to this change. Submitted #3096 to fix that.

tustvold · 2022-11-13T07:37:55Z

@maxburke could you possibly merge master into this branch, so the CI goes green. I don't seem to have permissions to do this, there is a checkbox to allow maintainers the ability to edit PRed branches

…rse_iter_with_size

maxburke · 2022-11-13T16:17:53Z

@maxburke could you possibly merge master into this branch, so the CI goes green. I don't seem to have permissions to do this, there is a checkbox to allow maintainers the ability to edit PRed branches

done!

viirya · 2022-11-13T18:23:42Z

Thanks @maxburke

ursabot · 2022-11-13T18:31:38Z

Benchmark runs are scheduled for baseline = b7af85c and contender = 20d81f5. 20d81f5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

alamb · 2022-11-14T20:04:04Z

🎉

maxburke added 4 commits October 20, 2022 12:50

implement ord for FixedSizeBinary types

8fac8fc

add ord test

edc745e

add compare for fixed size binary arrays

f5db19a

FixedSizeBinaryArray::try_from_sparse_iter has fails to generate a va…

c7ea585

…lid array when the iterator only produces None values

github-actions bot added the arrow Changes to the arrow crate label Nov 9, 2022

maxburke added 2 commits November 8, 2022 16:44

Merge branch 'master' into try_from_sparse_iter_with_size

702fe25

Tweak try_from_sparse_iter_with_size to take an Option<i32>; pass tests

8f787a7

tustvold reviewed Nov 10, 2022

View reviewed changes

maxburke added 4 commits November 11, 2022 14:37

simplify try_from_sparse_iter_with_size, make size parameter non-opti…

761fa15

…onal

add test for fixed size binary comparisons

85faf04

move tests to use FixedSizeBinaryArray::from_sparse_iter_with_size, a…

7162712

…dd docstring

format + fix failing tests

763b9da

alamb requested a review from tustvold November 12, 2022 13:33

tustvold approved these changes Nov 12, 2022

View reviewed changes

viirya approved these changes Nov 12, 2022

View reviewed changes

fix build

6210d69

Merge branch 'master' of github.com:apache/arrow-rs into try_from_spa…

4d7498c

…rse_iter_with_size

viirya merged commit 20d81f5 into apache:master Nov 13, 2022

tustvold mentioned this pull request Nov 14, 2022

FixedSizeBinaryArray::try_from_sparse_iter failed when given all Nones #1508

Closed

tustvold mentioned this pull request Nov 15, 2022

not panic when given all nones to FixedSizeBinaryArray::try_from_sparse_iter #1904

Closed

viirya mentioned this pull request Jan 2, 2023

Add native comparison kernel support for FixedSizeBinaryArray #1109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FixedSizeBinaryArray::try_from_sparse_iter_with_size #3054

Add FixedSizeBinaryArray::try_from_sparse_iter_with_size #3054

maxburke commented Nov 9, 2022 •

edited

Loading

tustvold left a comment

tustvold Nov 10, 2022

maxburke Nov 11, 2022

tustvold Nov 11, 2022

maxburke Nov 11, 2022

tustvold Nov 10, 2022

maxburke Nov 11, 2022

tustvold Nov 11, 2022

maxburke Nov 11, 2022

tustvold Nov 10, 2022

maxburke Nov 11, 2022

tustvold Nov 10, 2022

maxburke Nov 11, 2022

alamb commented Nov 11, 2022

maxburke commented Nov 11, 2022

alamb commented Nov 12, 2022

viirya commented Nov 12, 2022

maxburke commented Nov 12, 2022

viirya commented Nov 12, 2022

tustvold commented Nov 13, 2022 •

edited

Loading

maxburke commented Nov 13, 2022

viirya commented Nov 13, 2022

ursabot commented Nov 13, 2022

alamb commented Nov 14, 2022

-    pub fn try_from_sparse_iter_with_size<T, U>(
+    pub fn try_from_sparse_iter_with_size<T, U>(iter: T, size: i32) -> Result<Self, ArrowError> {
+        try_from_sparse_iter_with_size_opt::<T, U>(iter, Some(size))
+    }
+    fn try_from_sparse_iter_with_size_opt<T, U>(

Add FixedSizeBinaryArray::try_from_sparse_iter_with_size #3054

Add FixedSizeBinaryArray::try_from_sparse_iter_with_size #3054

Conversation

maxburke commented Nov 9, 2022 • edited Loading

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 11, 2022

maxburke commented Nov 11, 2022

alamb commented Nov 12, 2022

viirya commented Nov 12, 2022

maxburke commented Nov 12, 2022

viirya commented Nov 12, 2022

tustvold commented Nov 13, 2022 • edited Loading

maxburke commented Nov 13, 2022

viirya commented Nov 13, 2022

ursabot commented Nov 13, 2022

alamb commented Nov 14, 2022

maxburke commented Nov 9, 2022 •

edited

Loading

tustvold commented Nov 13, 2022 •

edited

Loading