Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: change map nullable value to false #1620

Merged
merged 4 commits into from
Sep 11, 2023
Merged

fix: change map nullable value to false #1620

merged 4 commits into from
Sep 11, 2023

Conversation

cmackenzie1
Copy link
Contributor

Description

This value was true but where arrow defines it as always false https://github.com/apache/arrow-rs/blob/master/arrow-schema/src/field.rs#L230.

This is also described in apache/arrow-rs#1697.

This also replaces key_value as the struct name with entries to remain consistent with https://github.com/apache/arrow-rs/blob/878217b9e330b4f1ed13e798a214ea11fbeb2bbb/arrow-schema/src/datatype.rs#L247-L250

The description of the main changes of your pull request

Related Issue(s)

Documentation

@github-actions github-actions bot added binding/rust Issues for the Rust crate rust labels Sep 8, 2023
wjones127
wjones127 reviewed Sep 8, 2023
View reviewed changes
rust/src/delta_arrow.rs
Copy link
Collaborator

@wjones127 wjones127 Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense. Both entries and keys are required to be non-nullable by the Arrow spec :)

https://github.com/apache/arrow/blob/c4b01c60fba85bbfc3a1b1510e179153c0f79515/format/Schema.fbs#L124

@wjones127
Copy link
Collaborator

I think we are hitting an upstream bug in MapArray::new_from_strings(): apache/arrow-rs#4807

@wjones127
Copy link
Collaborator

@cmackenzie1 for the test_record_batch_from_map_type error, I think we should regard MapArray::new_from_strings() as broken and change the test to build a map array differently for now.

I'm unclear on what the parquet2 code is doing near the other failure; I haven't touched that code base yet. LMK if you want more eyes on that and I can dig in.

@cmackenzie1
fix: change map nullable value to false
7760020 
This value was true but where arrow defines it as always false
https://github.com/apache/arrow-rs/blob/master/arrow-schema/src/field.rs#L230.

This is also described in apache/arrow-rs#1697.

This also replaces `key_value` as the struct name with `entries` to remain
consistent with https://github.com/apache/arrow-rs/blob/878217b9e330b4f1ed13e798a214ea11fbeb2bbb/arrow-schema/src/datatype.rs#L247-L250
@cmackenzie1
Copy link
Contributor Author

@wjones127 I reverted the change to parquet2 - not sure what is up with that one. Also I yanked the MapArray::new_from_strings and applied the patch inline for now. We can clean that up once arrow is upgraded here.

wjones127
wjones127 reviewed Sep 11, 2023
View reviewed changes
rust/src/delta_arrow.rs Outdated Show resolved Hide resolved
@wjones127 wjones127 enabled auto-merge (squash) September 11, 2023 02:26
@wjones127 wjones127 merged commit 78c4aab into delta-io:main Sep 11, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjones127 wjones127 wjones127 approved these changes

@houqp houqp Awaiting requested review from houqp

@xianwill xianwill Awaiting requested review from xianwill

@fvaleye fvaleye Awaiting requested review from fvaleye

@roeap roeap Awaiting requested review from roeap

@rtyler rtyler Awaiting requested review from rtyler

@mosyp mosyp Awaiting requested review from mosyp

Assignees
No one assigned
Labels
binding/rust Issues for the Rust crate rust
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Internal Parquet panic when using a Map type.
2 participants
@cmackenzie1 @wjones127