Update Union Array to add `UnionMode`, match latest Arrow Spec, and rename `new` -> `unsafe new_unchecked()` by alamb · Pull Request #885 · apache/arrow-rs

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

here is the API change

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

arrow/src/array/array_union.rs

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

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

    /// 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

Comments

Conversation

alamb commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

alamb Oct 30, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Oct 30, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Oct 30, 2021

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alamb commented Dec 20, 2021

Uh oh!

alamb commented Dec 20, 2021

Uh oh!

Uh oh!

alamb commented Dec 20, 2021

Uh oh!

alamb commented Dec 29, 2021

Uh oh!

paddyhoran commented Dec 31, 2021

Uh oh!

paddyhoran left a comment

Choose a reason for hiding this comment

Uh oh!

paddyhoran Jan 1, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Jan 2, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Jan 2, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Oct 30, 2021 •

edited

Loading

codecov-commenter commented Oct 30, 2021 •

edited

Loading