Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Added support for union scalars #930

Merged
merged 5 commits into from May 1, 2022

Conversation

ncpenke
Copy link
Contributor

@ncpenke ncpenke commented Apr 3, 2022

No description provided.

@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Merging #930 (5b991f4) into main (3a3e41b) will decrease coverage by 0.04%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##             main     #930      +/-   ##
==========================================
- Coverage   71.53%   71.49%   -0.05%     
==========================================
  Files         355      357       +2     
  Lines       19607    19688      +81     
==========================================
+ Hits        14026    14076      +50     
- Misses       5581     5612      +31     
Impacted Files Coverage Δ
src/array/null.rs 44.18% <ø> (ø)
src/scalar/equal.rs 58.33% <0.00%> (-2.54%) ⬇️
src/scalar/union.rs 45.45% <45.45%> (ø)
src/scalar/mod.rs 81.08% <100.00%> (+2.95%) ⬆️
src/io/ipc/read/file_async.rs 57.02% <0.00%> (-3.85%) ⬇️
src/io/ipc/read/reader.rs 75.79% <0.00%> (-1.29%) ⬇️
src/io/ipc/mod.rs 0.00% <0.00%> (ø)
src/io/flight/mod.rs 0.00% <0.00%> (ø)
src/io/ipc/write/stream.rs 73.33% <0.00%> (ø)
src/io/parquet/write/mod.rs 58.51% <0.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 3a3e41b...5b991f4. Read the comment docs.

Copy link
Owner

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @ncpenke , looks great!

I left some comments as I think we can simplify the scalar further due to the observation that Union does not have a validity - values are always valid.

@@ -296,6 +293,13 @@ impl Array for UnionArray {
None
}

fn is_valid(&self, index: usize) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary? A union does not have a "validity". I could not find where this was being used.

src/array/union/mod.rs Outdated Show resolved Hide resolved
src/scalar/mod.rs Outdated Show resolved Hide resolved
src/scalar/union.rs Outdated Show resolved Hide resolved
src/scalar/union.rs Show resolved Hide resolved
src/scalar/union.rs Outdated Show resolved Hide resolved
@ncpenke
Copy link
Contributor Author

ncpenke commented Apr 29, 2022

@jorgecarleitao Latest changes with your feedback - thanks!

Copy link
Owner

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome, super clean and easy to follow.

Thanks a lot. Left a minor documentation suggestion.

src/scalar/union.rs Outdated Show resolved Hide resolved
Co-authored-by: Jorge Leitao <jorgecarleitao@gmail.com>
@jorgecarleitao jorgecarleitao merged commit 9a38663 into jorgecarleitao:main May 1, 2022
@jorgecarleitao jorgecarleitao added the feature A new feature label May 1, 2022
@jorgecarleitao jorgecarleitao changed the title Add support for union scalars Added support for union scalars May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants