ARROW-10402: [Rust] Refactor array equality #8541

jorgecarleitao · 2020-10-27T18:05:49Z

This is a major refactor of the equal.rs module.

The rational for this change is many fold:

currently array comparison requires downcasting the array ref to its concrete types. This is painful and not very ergonomics, as the user must "guess" what to downcast for comparison. We can see this in the hacks around sort, take and concatenate kernel's tests, and some of the tests of the builders.
the code in array comparison is difficult to follow given the amount of calls that they perform around offsets.
The implementation currently indirectly uses many of the unsafe APIs that we have (via pointer aritmetics), which makes it risky to operate and mutate.
Some code is being repeated.

This PR:

adds impl PartialEq for dyn Array, to allow Array comparison based on Array::data (main change)
Makes array equality to only depend on ArrayData, i.e. it no longer depends on concrete array types (such as PrimitiveArray and related API) to perform comparisons.
Significantly reduces the risk of panics and UB when composite arrays are of different types, by checking the types on range comparison
Makes array equality be statically dispatched, via match datatype.
DRY the code around array equality
Fixes an error in equality of dictionary with equal values
Added tests to equalities that were not tested (fixed binary, some edge cases of dictionaries)
splits equal.rs in smaller, more manageable files.
Removes ArrayListOps, since it it no longer needed
Moves Json equality to its own module, for clarity.
removes the need to have two functions per type to compare arrays.
Adds the number of buffers and their respective width to datatypes from the specification. This was backported from ARROW-10109: [Rust] Add support to the C data interface for primitive types and utf8 #8401
adds a benchmark for array equality

Note that this does not implement PartialEq for ArrayData, only dyn Array, as different data does not imply a different array (due to nullability). That implementation is being worked on #8200.

IMO this PR significantly simplifies the code around array comparison, to the point where many implementations are 5 lines long.

This also improves performance by 10-40%.

Benchmark results

Previous HEAD position was 3dd3c69d7 Added bench for equality.
Switched to branch 'equal'
Your branch is up to date with 'origin/equal'.
   Compiling arrow v3.0.0-SNAPSHOT (/Users/jorgecarleitao/projects/arrow/rust/arrow)
    Finished bench [optimized] target(s) in 51.28s
     Running /Users/jorgecarleitao/projects/arrow/rust/target/release/deps/equal-176c3cb11360bd12
Gnuplot not found, using plotters backend
equal_512               time:   [36.861 ns 36.894 ns 36.934 ns]                       
                        change: [-43.752% -43.400% -43.005%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
  7 (7.00%) high mild
  5 (5.00%) high severe

equal_nulls_512         time:   [2.3271 us 2.3299 us 2.3331 us]                             
                        change: [-10.846% -9.0877% -7.7336%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe

equal_string_512        time:   [49.219 ns 49.347 ns 49.517 ns]                              
                        change: [-30.789% -30.538% -30.235%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  3 (3.00%) high mild
  6 (6.00%) high severe

equal_string_nulls_512  time:   [3.7873 us 3.7939 us 3.8013 us]                                    
                        change: [-8.2944% -7.0636% -5.4266%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) high mild
  8 (8.00%) high severe

All tests are there, plus new tests for some of the edge cases and untested arrays.

This change is backward incompatible array1.equals(&array2) no longer works: use array1 == array2 instead, which is the idiomatic way of comparing structs and trait objects in rust.

github-actions · 2020-10-27T18:09:26Z

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

rust/arrow/src/array/equal/null.rs

rust/arrow/src/array/data.rs

rust/arrow/src/array/equal/variable_size.rs

jorgecarleitao · 2020-11-06T08:00:33Z

I have simplified this code further to allow the compiler to optimize out some functions. The code is now 10-40% faster compared to current master. I updated the description accordingly.

nevi-me · 2020-11-07T05:48:13Z

rust/arrow/src/array/equal/structure.rs

+    lhs_start: usize,
+    rhs_start: usize,
+    len: usize,
+) -> bool {


Something that we can address later, is that if a struct slot is null, we should carry that nullness across to its children. This is actually the problem that @carols10cents and I encountered on both parquet roundtrips and integration tests.

The spec (http://arrow.apache.org/docs/format/Columnar.html#struct-layout) says:

While a struct does not have physical storage for each of its semantic slots (i.e. each scalar C-like struct), an entire struct slot can be set to null via the validity bitmap. Any of the child field arrays can have null values according to their respective independent validity bitmaps. This implies that for a particular struct slot the validity bitmap for the struct array might indicate a null slot when one or more of its child arrays has a non-null value in their corresponding slot. When reading the struct array the parent validity bitmap takes priority.

I think we can address this in a separate PR, if we still find that struct_equal doesn't cover that edge case.

nevi-me

Thanks @jorgecarleitao

I'm happy that this PR's been updated for logical equality, and I'd propose we merge it in, then look at #8590 and #8200 to address edge-cases where the IPC and Parquet tests still fail equality tests.

@carols10cents @alamb @paddyhoran @jhorstmann what's your opinion?

alamb

I can't say I made it through this entire PR carefully, but I reviewed about 50% of it carefully and skimmed the rest -- what I did see had a logical and understandable structure I would feel comfortable supporting / maintaining, FWIW.

I agree with @jorgecarleitao that having a uniform definition of array equality will solve many of the challenges we have seen (especially with parquet <--> arrow round tripping)

Thus my opinion is that we should merge this.

alamb · 2020-11-07T11:48:21Z

rust/arrow/src/array/array.rs

@@ -112,10 +112,10 @@ pub trait Array: fmt::Debug + Send + Sync + ArrayEqual + JsonEqual {
    /// // Make slice over the values [2, 3, 4]
    /// let array_slice = array.slice(1, 3);
    ///
-    /// assert!(array_slice.equals(&Int32Array::from(vec![2, 3, 4])));
+    /// assert_eq!(array_slice.as_ref(), &Int32Array::from(vec![2, 3, 4]));


alamb · 2020-11-07T11:49:30Z

rust/arrow/src/array/builder.rs

@@ -531,7 +531,7 @@ impl<T: ArrowPrimitiveType> ArrayBuilder for PrimitiveBuilder<T> {

            for i in 0..len {
                // account for offset as `ArrayData` does not


this comment may need to be updated as the code seems to no longer account for offset

alamb · 2020-11-07T11:50:05Z

rust/arrow/src/array/builder.rs

        assert_eq!(finished.len(), expected.len());
        assert_eq!(finished.null_count(), expected.null_count());
-        assert!(finished.equals(&(*expected)));
+        assert_eq!(finished, expected);


this is certainly a lot nicer

alamb · 2020-11-07T11:50:36Z

rust/arrow/src/array/builder.rs

@@ -3999,7 +3996,7 @@ mod tests {
        let expected_list =
            FixedSizeListArray::from(Arc::new(expected_list_data) as ArrayDataRef);
        let expected_list = FixedSizeBinaryArray::from(expected_list);
-        // assert!(expected_list.values().equals(&*finished.values()));
+        // assert_eq!(expected_list.values(), finished.values());


not that you did it, but I wonder why this code is commented out -- maybe your changes will have fixed it

alamb · 2020-11-07T11:59:56Z

rust/arrow/src/array/data.rs

+    pub(super) fn buffer<T: ArrowNativeType>(&self, buffer: usize) -> &[T] {
+        let values = unsafe { self.buffers[buffer].data().align_to::<T>() };
+        if values.0.len() != 0 || values.2.len() != 0 {
+            panic!("The buffer is not byte-aligned with its interpretation")


👍 for the panic

alamb · 2020-11-07T12:05:06Z

rust/arrow/src/array/data.rs

+        new_data
+    }
+
+    /// Returns the buffer `buffer` as a slice of type `T`. The slice is already offset.


Suggested change

/// Returns the buffer `buffer` as a slice of type `T`. The slice is already offset.

/// Returns self.buffers at index `buffer` as a slice of type `T` starting at self.offset

alamb · 2020-11-07T12:12:30Z

rust/arrow/src/array/equal/mod.rs

+}
+
+#[inline]
+fn equal_values(


Suggested change

fn equal_values(

/// Compares two arrays for equality starting at `lhs_start` and `rhs_start`

/// `lhs` and `rhs` *must* have the same data type

fn equal_values(

jhorstmann · 2020-11-07T15:13:42Z

rust/arrow/src/array/data.rs

@@ -121,7 +120,7 @@ impl ArrayData {
    /// Returns whether the element at index `i` is null
    pub fn is_null(&self, i: usize) -> bool {
        if let Some(ref b) = self.null_bitmap {
-            return !b.is_set(i);
+            return !b.is_set(self.offset + i);


Thanks a lot, I think I have another ticket that would be fixed by this change

jorgecarleitao · 2020-11-07T17:11:54Z

@alamb , @nevi-me and @jhorstmann , thanks a lot for taking a close look into this, also for the amazing work yesterday and today on reviewing and merging stuff. Really impressive! 💯

I rebased this and resolved conflicts.

nevi-me · 2020-11-07T17:25:03Z

@alamb , @nevi-me and @jhorstmann , thanks a lot for taking a close look into this, also for the amazing work yesterday and today on reviewing and merging stuff. Really impressive! 💯

I rebased this and resolved conflicts.

I'm super-excited, because this likely unlocks us on the failures that I've been getting on #8200.

This is a major refactor of the `equal.rs` module. The rational for this change is many fold: * currently array comparison requires downcasting the array ref to its concrete types. This is painful and not very ergonomics, as the user must "guess" what to downcast for comparison. We can see this in the hacks around `sort`, `take` and `concatenate` kernel's tests, and some of the tests of the builders. * the code in array comparison is difficult to follow given the amount of calls that they perform around offsets. * The implementation currently indirectly uses many of the `unsafe` APIs that we have (via pointer aritmetics), which makes it risky to operate and mutate. * Some code is being repeated. This PR: 1. adds `impl PartialEq for dyn Array`, to allow `Array` comparison based on `Array::data` (main change) 2. Makes array equality to only depend on `ArrayData`, i.e. it no longer depends on concrete array types (such as `PrimitiveArray` and related API) to perform comparisons. 3. Significantly reduces the risk of panics and UB when composite arrays are of different types, by checking the types on `range` comparison 4. Makes array equality be statically dispatched, via `match datatype`. 5. DRY the code around array equality 6. Fixes an error in equality of dictionary with equal values 7. Added tests to equalities that were not tested (fixed binary, some edge cases of dictionaries) 8. splits `equal.rs` in smaller, more manageable files. 9. Removes `ArrayListOps`, since it it no longer needed 10. Moves Json equality to its own module, for clarity. 11. removes the need to have two functions per type to compare arrays. 12. Adds the number of buffers and their respective width to datatypes from the specification. This was backported from apache#8401 13. adds a benchmark for array equality Note that this does not implement `PartialEq` for `ArrayData`, only `dyn Array`, as different data does not imply a different array (due to nullability). That implementation is being worked on apache#8200. IMO this PR significantly simplifies the code around array comparison, to the point where many implementations are 5 lines long. This also improves performance by 10-40%. <details> <summary>Benchmark results</summary> ``` Previous HEAD position was 3dd3c69 Added bench for equality. Switched to branch 'equal' Your branch is up to date with 'origin/equal'. Compiling arrow v3.0.0-SNAPSHOT (/Users/jorgecarleitao/projects/arrow/rust/arrow) Finished bench [optimized] target(s) in 51.28s Running /Users/jorgecarleitao/projects/arrow/rust/target/release/deps/equal-176c3cb11360bd12 Gnuplot not found, using plotters backend equal_512 time: [36.861 ns 36.894 ns 36.934 ns] change: [-43.752% -43.400% -43.005%] (p = 0.00 < 0.05) Performance has improved. Found 12 outliers among 100 measurements (12.00%) 7 (7.00%) high mild 5 (5.00%) high severe equal_nulls_512 time: [2.3271 us 2.3299 us 2.3331 us] change: [-10.846% -9.0877% -7.7336%] (p = 0.00 < 0.05) Performance has improved. Found 11 outliers among 100 measurements (11.00%) 4 (4.00%) high mild 7 (7.00%) high severe equal_string_512 time: [49.219 ns 49.347 ns 49.517 ns] change: [-30.789% -30.538% -30.235%] (p = 0.00 < 0.05) Performance has improved. Found 9 outliers among 100 measurements (9.00%) 3 (3.00%) high mild 6 (6.00%) high severe equal_string_nulls_512 time: [3.7873 us 3.7939 us 3.8013 us] change: [-8.2944% -7.0636% -5.4266%] (p = 0.00 < 0.05) Performance has improved. Found 12 outliers among 100 measurements (12.00%) 4 (4.00%) high mild 8 (8.00%) high severe ``` </details> All tests are there, plus new tests for some of the edge cases and untested arrays. This change is backward incompatible `array1.equals(&array2)` no longer works: use `array1 == array2` instead, which is the idiomatic way of comparing structs and trait objects in rust. Closes apache#8541 from jorgecarleitao/equal Authored-by: Jorge C. Leitao <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

github-actions bot added the Component: Rust label Oct 27, 2020

jorgecarleitao mentioned this pull request Oct 29, 2020

ARROW-10413: [Rust] [Parquet] Unignore some tests that are passing now #8546

Closed

jorgecarleitao requested review from sunchao and nevi-me October 29, 2020 04:26

jorgecarleitao marked this pull request as ready for review October 29, 2020 04:36

nevi-me mentioned this pull request Oct 29, 2020

ARROW-8421: [Rust] [Parquet] Merge in-progress writer & reader #8548

Merged

nevi-me reviewed Nov 3, 2020

View reviewed changes

rust/arrow/src/array/equal/null.rs Show resolved Hide resolved

rust/arrow/src/array/data.rs Show resolved Hide resolved

rust/arrow/src/array/data.rs Show resolved Hide resolved

rust/arrow/src/array/equal/variable_size.rs Outdated Show resolved Hide resolved

jorgecarleitao mentioned this pull request Nov 4, 2020

ARROW-10042: [Rust] Fix tests involving ArrayData/Buffer equality #8590

Closed

nevi-me reviewed Nov 7, 2020

View reviewed changes

nevi-me approved these changes Nov 7, 2020

View reviewed changes

alamb approved these changes Nov 7, 2020

View reviewed changes

jhorstmann reviewed Nov 7, 2020

View reviewed changes

jorgecarleitao added 4 commits November 7, 2020 18:00

Moved JsonEqual to its own module.

3aefe84

Added bench for equality.

8cde095

Moved function to inside ArrayData and unit-tested it.

eec30c7

Refactored equality.

5801f5c

nevi-me closed this in a04a15a Nov 7, 2020

jorgecarleitao deleted the equal branch December 4, 2020 07:40

jorgecarleitao restored the equal branch December 4, 2020 07:41

jorgecarleitao deleted the equal branch December 14, 2020 07:35

mqy mentioned this pull request Jan 5, 2021

ARROW-11131: [Rust] Improve performance of bool_equal #9101

Closed

alamb mentioned this pull request Sep 9, 2021

PyO3 bridge for pyarrow interoperability / fix arrow integration test apache/arrow-rs#691

Merged

asfimport mentioned this pull request Nov 7, 2020

[Rust] Improve array equality #26385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-10402: [Rust] Refactor array equality #8541

ARROW-10402: [Rust] Refactor array equality #8541

jorgecarleitao commented Oct 27, 2020 •

edited

Loading

github-actions bot commented Oct 27, 2020

jorgecarleitao commented Nov 6, 2020

nevi-me Nov 7, 2020

nevi-me left a comment

alamb left a comment

alamb Nov 7, 2020

alamb Nov 7, 2020

alamb Nov 7, 2020

alamb Nov 7, 2020

alamb Nov 7, 2020

alamb Nov 7, 2020

alamb Nov 7, 2020

jhorstmann Nov 7, 2020

jorgecarleitao commented Nov 7, 2020

nevi-me commented Nov 7, 2020

		@@ -531,7 +531,7 @@ impl<T: ArrowPrimitiveType> ArrayBuilder for PrimitiveBuilder<T> {

		for i in 0..len {
		// account for offset as `ArrayData` does not

	/// Returns the buffer `buffer` as a slice of type `T`. The slice is already offset.
	/// Returns self.buffers at index `buffer` as a slice of type `T` starting at self.offset

ARROW-10402: [Rust] Refactor array equality #8541

ARROW-10402: [Rust] Refactor array equality #8541

Conversation

jorgecarleitao commented Oct 27, 2020 • edited Loading

github-actions bot commented Oct 27, 2020

jorgecarleitao commented Nov 6, 2020

Choose a reason for hiding this comment

nevi-me left a comment

Choose a reason for hiding this comment

alamb 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

jorgecarleitao commented Nov 7, 2020

nevi-me commented Nov 7, 2020

jorgecarleitao commented Oct 27, 2020 •

edited

Loading