Add Full UnionArray validation #1444

alamb · 2022-03-14T21:26:54Z

~~Draft (need to write tests and file a ticket)~~

Which issue does this PR close?

Rationale for this change

While reviewing #1412, I remembered to check the validation code for UnionArray and it turns out it was not implemented yet.

Since I had the union layout already loaded in my head, I figured I would pound out this PR:

What changes are included in this PR?

Validate all Union array data in validate_full

Are there any user-facing changes?

More data validation. The APIs are all the same

codecov-commenter · 2022-03-14T21:39:25Z

Codecov Report

Merging #1444 (d56cf46) into master (2932da3) will increase coverage by 0.02%.
The diff coverage is 89.38%.

@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
+ Coverage   82.71%   82.73%   +0.02%     
==========================================
  Files         187      187              
  Lines       54255    54365     +110     
==========================================
+ Hits        44877    44979     +102     
- Misses       9378     9386       +8

Impacted Files	Coverage Δ
arrow/src/array/data.rs	`83.50% <89.38%> (+0.70%)`	⬆️
arrow/src/array/transform/mod.rs	`86.46% <0.00%> (ø)`
parquet_derive/src/parquet_field.rs	`66.21% <0.00%> (+0.22%)`	⬆️
arrow/src/datatypes/datatype.rs	`66.80% <0.00%> (+0.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2932da3...d56cf46. Read the comment docs.

arrow/src/array/data.rs

viirya

Looks correct to me.

arrow/src/array/data.rs

tustvold · 2022-03-24T16:39:21Z

arrow/src/array/data.rs

+                        self.validate_offsets_full::<i8>(self.child_data.len())?;
+                    }
+                    UnionMode::Dense => {
+                        self.validate_dense_union_full()?;


I was going to suggest that this should validate that the null bitmasks are disjoint, but this may not even be a requirement - the specification says "All “unselected” values are ignored and could be any semantically correct array value."

tustvold · 2022-03-24T16:43:51Z

arrow/src/array/data.rs

@@ -1117,6 +1119,44 @@ impl ArrayData {
        )
    }

+    /// Ensures that for each union element, the offset is correct for
+    /// the corresponding child array
+    fn validate_dense_union_full(&self) -> Result<()> {


I think should also check that offsets are monotonic for a given array type, but that could definitely be left as a todo

tustvold · 2022-03-24T16:46:38Z

arrow/src/array/data.rs

-                // https://github.com/apache/arrow-rs/issues/85
-                //
-                // TODO file follow on ticket for full union validation
+            DataType::Union(_fields, mode) => {


Unless I'm missing something, we should probably also add buffer length checks into ArrayData::validate as I don't think these are currently present anywhere

what do you mean "buffer length checks"? Which buffers?

Sorry this got lost in my various rewording of this, I mean child arrays 🤦‍♂️

The length of the ArrayData::buffers are checked as part of the 'layout' in ArrayData::validate

https://github.com/apache/arrow-rs/blob/master/arrow/src/array/data.rs#L1210

The child arrays ArrayData::child_data are validated recursively in ArrayData::validate_child_data
https://github.com/apache/arrow-rs/blob/master/arrow/src/array/data.rs#L788

tustvold · 2022-03-24T16:51:52Z

arrow/src/array/data.rs

+    /// Ensures that for each union element, the offset is correct for
+    /// the corresponding child array
+    fn validate_dense_union_full(&self) -> Result<()> {
+        // safety justification is that the size of the buffers was validated in self.validate()


We could potentially make validate also check that all child arrays have the length of the parent in the case of a dense representation

I think actually that all child arrays need to have the same length for a sparse representation (which is checked in cheap validate)

Which is covered by test_validate_sparse_union_different_child_len

…alidation

!

tustvold · 2022-03-28T09:53:31Z

arrow/src/array/data.rs

+                            child_offset, i))
+                    })?;
+
+                if let Some(last_offset) = last_offset.as_ref() {


This is incorrect, the requirement is that for a given type the offsets must be increasing. But it is perfectly correct, in fact desirable, for the offsets array itself to not be increasing.

e.g. the following would be valid

types: [0, 1, 1, 0], offsets: [0, 0, 1, 0],

Otherwise the dense representation is no different from the sparse representation

tustvold · 2022-03-28T09:55:08Z

arrow/src/array/data.rs

+    #[should_panic(
+        expected = "Offset invariant failure: position 2 invalid. 0 should be >= 1"
+    )]
+    fn test_validate_dense_union_non_increasing_offset() {


This test should not panic, it is valid as no single child-array has non-increasing offsets.

This is not valid

type_ids = [0, 0, 0], offsets = [0, 1, 0],

But this is

type_ids = [0, 0, 1], offsets = [0, 1, 0],

As is the example from https://arrow.apache.org/docs/format/Columnar.html#dense-union

type_ids = [0, 0, 0, 1] offsets = [0, 1, 2, 0]

Fixed in 2769573

…alidation

alamb · 2022-04-06T18:03:38Z

I am thinking I may try and add a CI check and a feature that runs all the tests with full data checking enabled to double check this isn't introducing any issues

alamb · 2022-04-12T13:53:12Z

#1544 tracks checking validation in CI, #1546 is the PR

alamb · 2022-04-12T14:45:55Z

FWIW I tested this PR locally with full validation enabled and several tests failed.


failures:
    array::array_union::tests::test_dense_mixed_with_nulls
    array::array_union::tests::test_dense_mixed_with_nulls_and_offset
    array::equal::tests::test_union_equal_dense

Putting back to draft until is sort that out

tustvold · 2023-02-27T18:27:58Z

I plan to implement this in #1799

Add UnionArray validation

30cb59c

github-actions bot added the arrow Changes to the arrow crate label Mar 14, 2022

viirya reviewed Mar 15, 2022

View reviewed changes

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

viirya reviewed Mar 15, 2022

View reviewed changes

tustvold reviewed Mar 24, 2022

View reviewed changes

alamb added 7 commits March 24, 2022 14:44

Merge remote-tracking branch 'apache/master' into alamb/union_array_v…

ec58d86

…alidation

fix merge issue

dc40d4e

fix: write tests for sparse union, fix bugs discovered

20b445c

cleanup

7fcdc9e

add check for non decreasing offsets

c0c21f1

!

fix clippy

2bc1491

clean comments

d56cf46

alamb marked this pull request as ready for review March 24, 2022 20:53

tustvold requested changes Mar 28, 2022

View reviewed changes

alamb added 3 commits March 29, 2022 15:23

Merge remote-tracking branch 'apache/master' into alamb/union_array_v…

6271ba8

…alidation

fix child offset validation logic

2769573

fix comment

72ef638

alamb marked this pull request as draft April 12, 2022 14:46

tustvold closed this Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Full UnionArray validation #1444

Add Full UnionArray validation #1444

alamb commented Mar 14, 2022 •

edited

Loading

codecov-commenter commented Mar 14, 2022 •

edited

Loading

viirya left a comment

tustvold Mar 24, 2022

tustvold Mar 24, 2022

tustvold Mar 24, 2022

alamb Mar 24, 2022

tustvold Mar 24, 2022

alamb Mar 24, 2022

tustvold Mar 24, 2022

alamb Mar 24, 2022 •

edited

Loading

tustvold Mar 28, 2022 •

edited

Loading

tustvold Mar 28, 2022 •

edited

Loading

alamb Mar 29, 2022

alamb commented Apr 6, 2022

alamb commented Apr 12, 2022 •

edited

Loading

alamb commented Apr 12, 2022

tustvold commented Feb 27, 2023

Add Full UnionArray validation #1444

Add Full UnionArray validation #1444

Conversation

alamb commented Mar 14, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Mar 14, 2022 • edited Loading

Codecov Report

viirya 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

alamb Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

tustvold Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

tustvold Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Apr 6, 2022

alamb commented Apr 12, 2022 • edited Loading

alamb commented Apr 12, 2022

tustvold commented Feb 27, 2023

alamb commented Mar 14, 2022 •

edited

Loading

codecov-commenter commented Mar 14, 2022 •

edited

Loading

alamb Mar 24, 2022 •

edited

Loading

tustvold Mar 28, 2022 •

edited

Loading

tustvold Mar 28, 2022 •

edited

Loading

alamb commented Apr 12, 2022 •

edited

Loading