Update Union Array to add `UnionMode`, match latest Arrow Spec, and rename `new` -> `unsafe new_unchecked()` #885

alamb · 2021-10-30T13:36:47Z

~~(Built on #810, so to see the changes proposed in this PR just look at the last commit)~~

Which issue does this PR close?

Closes #814
Closes #85
Related to #817

Rationale for this change

Follow Arrow spec for UnionArray, make it possible to validate with generic code added in #810, and conform to Rust safety conventions.

See https://arrow.apache.org/docs/format/Columnar.html#union-layout for a description of the Union layout

What changes are included in this PR?

Rename UnionArray::new to unsafe UnionArray::new_unchecked() to follow standard Rust safety conventions
Add UnionMode to DataType::Union so the type carries information on expected format
Update null values offset handling to match https://arrow.apache.org/docs/format/Columnar.html#union-layout
Add validation in ArrayData::validate

Are there any user-facing changes?

UnionArray::new has been renamed to unsafe UnionArray::new_unchecked()
Must specify Dense or Sparse when creating a DataType::Union
Accepted UnionArray format is different

alamb · 2021-10-30T13:38:26Z

arrow/src/array/array_union.rs

@@ -65,7 +65,7 @@ impl UnionArray {
    /// In both of the cases above we are accepting `Buffer`'s which are assumed to be representing
    /// `i8` and `i32` values respectively.  `Buffer` objects are untyped and no attempt is made
    /// to ensure that the data provided is valid.
-    pub fn new(
+    pub unsafe fn new_unchecked(


here is the API change

alamb · 2021-10-30T13:39:05Z

arrow/src/array/array_union.rs

    pub fn try_new(
        type_ids: Buffer,
        value_offsets: Option<Buffer>,
        child_arrays: Vec<(Field, ArrayRef)>,
        bitmap: Option<Buffer>,
    ) -> Result<Self> {
        if let Some(b) = &value_offsets {
-            let nulls = count_nulls(bitmap.as_ref(), 0, type_ids.len());
-            if ((type_ids.len() - nulls) * 4) != b.len() {
+            if ((type_ids.len()) * 4) != b.len() {


here is the core format change -- the length of type_ids must match the length of the values (rather than skipping nulls)

alamb · 2021-10-30T13:39:49Z

arrow/src/array/data.rs

-            // Note sparse unions only have one buffer (u8) type_ids,
-            // and dense unions have 2 (type_ids as well as offsets).
-            // https://github.com/apache/arrow-rs/issues/85
+        DataType::Union(_, mode) => {


we can now validate the buffer layout based on DataType!

codecov-commenter · 2021-10-30T13:51:10Z

Codecov Report

Merging #885 (c8737a1) into master (fe106e0) will increase coverage by 0.02%.
The diff coverage is 93.69%.

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   82.33%   82.35%   +0.02%     
==========================================
  Files         169      169              
  Lines       49719    49788      +69     
==========================================
+ Hits        40936    41003      +67     
- Misses       8783     8785       +2

Impacted Files	Coverage Δ
arrow/src/array/equal/mod.rs	`93.13% <ø> (ø)`
arrow/src/array/equal/utils.rs	`74.00% <ø> (ø)`
arrow/src/datatypes/datatype.rs	`66.38% <0.00%> (ø)`
parquet/src/arrow/arrow_writer.rs	`98.07% <ø> (ø)`
parquet/src/arrow/levels.rs	`82.87% <0.00%> (ø)`
parquet/src/arrow/schema.rs	`85.56% <ø> (ø)`
arrow/src/array/builder.rs	`86.49% <80.00%> (-0.05%)`	⬇️
arrow/src/array/array_union.rs	`90.76% <86.66%> (-0.22%)`	⬇️
arrow/src/array/data.rs	`82.28% <96.96%> (+1.14%)`	⬆️
arrow/src/array/array.rs	`85.25% <100.00%> (ø)`
... and 7 more

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 fe106e0...c8737a1. Read the comment docs.

alamb · 2021-12-20T16:52:50Z

Marking PRs that are over a month old as stale -- please let us know if there is additional work planned or if we should close them.

alamb · 2021-12-20T16:53:14Z

(I do have some idea in my head I can finish this one up)

arrow/src/array/array_union.rs

alamb · 2021-12-20T17:06:03Z

Ok, I got the yak shaving bug and cleaned up this PR and marked it ready for review. 🙏

alamb · 2021-12-29T19:55:02Z

@jimexist @paddyhoran @nevi-me -- might one of you have time to review this PR? It revamps how UnionArray is supported to conform to the modern Arrow spec, and adds additional validation. I would like to get it in for the 7.0.0 release in a few weeks time

paddyhoran · 2021-12-31T02:08:15Z

Sorry I've been so quiet on Arrow. I'll find some time to review in the coming days.

paddyhoran

LGTM, thanks @alamb

paddyhoran · 2022-01-01T20:14:35Z

arrow/src/array/array_union.rs

    ///
    /// The `type_ids` `Buffer` should contain `i8` values.  These values should be greater than
    /// zero and must be less than the number of children provided in `child_arrays`.  These values
    /// are used to index into the `child_arrays`.
    ///
    /// The `value_offsets` `Buffer` is only provided in the case of a dense union, sparse unions
-    /// should use `None`.  If provided the `value_offsets` `Buffer` should contain `i32` values.


I think you removed the period at the end of this line by mistake, right?

Yes, I think so. Good catch.

Fixed in c8737a1

alamb added the api-change Changes to the arrow API label Oct 30, 2021

github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels Oct 30, 2021

alamb commented Oct 30, 2021

View reviewed changes

alamb mentioned this pull request Oct 30, 2021

Implement Union validity bitmap changes from ARROW-9222 #85

Closed

alamb mentioned this pull request Nov 7, 2021

Add full data validation for ArrayData::try_new() #921

Merged

1 task

alamb force-pushed the alamb/union_fix branch from 3ace1e2 to ca55696 Compare November 8, 2021 19:13

github-actions bot added the arrow-flight Changes to the arrow-flight crate label Nov 8, 2021

alamb changed the title ~~Update Union Array to match Arrow Spec, rename new -> unsafe new_unchecked()~~ Update Union Array to add UnionMode, match latest Arrow Spec, and rename new -> unsafe new_unchecked() Nov 8, 2021

alamb added the stale-pr label Dec 20, 2021

alamb force-pushed the alamb/union_fix branch from 8cde1ed to a094eb7 Compare December 20, 2021 17:02

alamb requested a review from paddyhoran December 20, 2021 17:02

Update union array to new null handling

b481581

alamb force-pushed the alamb/union_fix branch from a094eb7 to b481581 Compare December 20, 2021 17:04

alamb commented Dec 20, 2021

View reviewed changes

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

alamb marked this pull request as ready for review December 20, 2021 17:05

alamb removed the stale-pr label Dec 21, 2021

paddyhoran approved these changes Jan 1, 2022

View reviewed changes

alamb added 3 commits January 2, 2022 06:38

Update arrow/src/array/array_union.rs

66f93e3

Merge remote-tracking branch 'apache/master' into alamb/union_fix

ab6e50c

correct comment

c8737a1

alamb merged commit 89d5273 into apache:master Jan 2, 2022

alamb deleted the alamb/union_fix branch January 2, 2022 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Union Array to add `UnionMode`, match latest Arrow Spec, and rename `new` -> `unsafe new_unchecked()` #885

Update Union Array to add `UnionMode`, match latest Arrow Spec, and rename `new` -> `unsafe new_unchecked()` #885

alamb commented Oct 30, 2021 •

edited

Loading

alamb Oct 30, 2021

alamb Oct 30, 2021

alamb Oct 30, 2021

codecov-commenter commented Oct 30, 2021 •

edited

Loading

alamb commented Dec 20, 2021

alamb commented Dec 20, 2021

alamb commented Dec 20, 2021

alamb commented Dec 29, 2021

paddyhoran commented Dec 31, 2021

paddyhoran left a comment

paddyhoran Jan 1, 2022

alamb Jan 2, 2022

alamb Jan 2, 2022

Update Union Array to add UnionMode, match latest Arrow Spec, and rename new -> unsafe new_unchecked() #885

Update Union Array to add UnionMode, match latest Arrow Spec, and rename new -> unsafe new_unchecked() #885

Conversation

alamb commented Oct 30, 2021 • 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?

alamb Oct 30, 2021

Choose a reason for hiding this comment

alamb Oct 30, 2021

Choose a reason for hiding this comment

alamb Oct 30, 2021

Choose a reason for hiding this comment

codecov-commenter commented Oct 30, 2021 • edited Loading

Codecov Report

alamb commented Dec 20, 2021

alamb commented Dec 20, 2021

alamb commented Dec 20, 2021

alamb commented Dec 29, 2021

paddyhoran commented Dec 31, 2021

paddyhoran left a comment

Choose a reason for hiding this comment

paddyhoran Jan 1, 2022

Choose a reason for hiding this comment

alamb Jan 2, 2022

Choose a reason for hiding this comment

alamb Jan 2, 2022

Choose a reason for hiding this comment

Update Union Array to add `UnionMode`, match latest Arrow Spec, and rename `new` -> `unsafe new_unchecked()` #885

Update Union Array to add `UnionMode`, match latest Arrow Spec, and rename `new` -> `unsafe new_unchecked()` #885

alamb commented Oct 30, 2021 •

edited

Loading

codecov-commenter commented Oct 30, 2021 •

edited

Loading