Skip to content

ARROW-10996: [Rust] [Parquet] change return value type of get_arrow_schema_from_metadata() - #9058

Closed
mqy wants to merge 4 commits into
apache:masterfrom
mqy:get_arrow_schema_from_metadata
Closed

ARROW-10996: [Rust] [Parquet] change return value type of get_arrow_schema_from_metadata()#9058
mqy wants to merge 4 commits into
apache:masterfrom
mqy:get_arrow_schema_from_metadata

Conversation

@mqy

@mqymqy commented Dec 31, 2020

Copy link
Copy Markdown
Contributor

#8936 updated crate flatbuffers to 0.8.0 , but function get_arrow_schema_from_metadata still returning Option rather than Result. This PR fixes this issue.

@mqymqy changed the title ARROW-10996: [Rust] change return value type of get_arrow_schema_from_metadataARROW-10996: [Rust] change return value type of get_arrow_schema_from_metadata()Dec 31, 2020
@github-actions

github-actionsBot commented Dec 31, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-10996

@alamb

alamb commented Dec 31, 2020

Copy link
Copy Markdown
Contributor

The full set of Rust CI tests did not run on this PR :(

Can you please rebase this PR against apache/master to pick up the changes in #9056 so that they do?

I apologize for the inconvenience.

@mqy
mqyforce-pushed the get_arrow_schema_from_metadata branch from beceee3 to 2254cb5CompareDecember 31, 2020 15:53
@mqy

mqy commented Dec 31, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@alamb thanks
I will rebase again after #9061 get merged, it has passed all checks.
ping @Dandandan

@mqy
mqyforce-pushed the get_arrow_schema_from_metadata branch from 2254cb5 to c904ee9CompareJanuary 1, 2021 07:28
@codecov-io

codecov-io commented Jan 1, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #9058 (56c24ed) into master (51672b2) will increase coverage by 0.00%.
The diff coverage is 77.77%.

Impacted file tree graph

@@ Coverage Diff @@## master #9058 +/- ##
=======================================
Coverage 82.61% 82.62% =======================================
Files 202 202 Lines 50048 50055 +7 =======================================
+ Hits 41347 41356 +9 + Misses 8701 8699 -2
Impacted FilesCoverage Δ
rust/parquet/src/arrow/schema.rs90.93% <77.77%> (+0.31%)⬆️
rust/arrow/src/memory.rs98.14% <0.00%> (-1.86%)⬇️
rust/arrow/src/ffi.rs70.00% <0.00%> (-0.29%)⬇️
rust/arrow/src/array/array_boolean.rs86.50% <0.00%> (-0.22%)⬇️
rust/parquet/src/encodings/encoding.rs95.24% <0.00%> (-0.20%)⬇️
rust/arrow/src/array/array_primitive.rs91.64% <0.00%> (-0.05%)⬇️
rust/arrow/src/array/array_list.rs93.10% <0.00%> (-0.02%)⬇️
rust/arrow/src/buffer.rs98.21% <0.00%> (-0.01%)⬇️
rust/arrow/src/array/raw_pointer.rs100.00% <0.00%> (ø)
rust/arrow/src/array/array_binary.rs90.61% <0.00%> (ø)
... and 4 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 51672b2...56c24ed. Read the comment docs.

jorgecarleitao
jorgecarleitao approved these changes Jan 1, 2021
View reviewed changes

@jorgecarleitaojorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks a lot @mqy for taking this on. I always hit those prints when searching for my own prints in the code 😆

Comment threadrust/parquet/src/arrow/schema.rs Outdated
.remove(super::ARROW_SCHEMA_META_KEY)
.map(|encoded| get_arrow_schema_from_metadata(&encoded));

let arrow_schema_metadata = match maybe_schema {

@jorgecarleitaojorgecarleitaoJan 1, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There may be an idiom for this in rust (that I do not know).

@mqymqyJan 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@jorgecarleitao thank you for the tip!

The codes were updated to use map_or for flipping Option<Result> to Result<Option>.
BTW, I found some similar usage of map_or in parquet/src/column/writer.rs 🥇

Dandandan
Dandandan reviewed Jan 1, 2021
View reviewed changes
Comment threadrust/parquet/src/arrow/schema.rs Outdated
_ => None,
};
.map(|encoded| get_arrow_schema_from_metadata(&encoded))
.map_or(Ok(None), |v| v.map(Some))?;

@DandandanDandandanJan 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this can use unwrap_or, as both of the cases map to Ok?

@mqymqyJan 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Dandandan thanks a lot! The code looks more concise!

@mqymqy changed the title ARROW-10996: [Rust] change return value type of get_arrow_schema_from_metadata()ARROW-10996: [Rust] [Parquet] change return value type of get_arrow_schema_from_metadata()Jan 1, 2021
Dandandan
Dandandan reviewed Jan 1, 2021
View reviewed changes
Comment threadrust/parquet/src/arrow/schema.rs
.remove(super::ARROW_SCHEMA_META_KEY)
.map(|encoded| get_arrow_schema_from_metadata(&encoded))
.unwrap_or_default();
.map_or(Ok(None), |v| v.map(Some))?;

@DandandanDandandanJan 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can use it here to

@mqymqyJan 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks, let me try

alamb
alamb approved these changes Jan 1, 2021
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me -- thanks @mqy

I think @Dandandan has one more suggested cleanup https://github.com/apache/arrow/pull/9058/files#r550758655 -- but I also think this PR is fine to merge as is.

Let me know if you want to make any more changes -- once you are finished we'll merge it.

Thanks again

@mqy

mqy commented Jan 1, 2021

Copy link
Copy Markdown
ContributorAuthor

@Dandandan@alamb Sorry, I failed to apply unwrap_or or something else toparquet_to_arrow_schema_by_columns,
because there are two places using arrow schema (if exists), so we can't return early.

@Dandandan

Dandandan commented Jan 1, 2021

Copy link
Copy Markdown
Contributor

@mqy no problem, is a rather small style improvement anyway.

@jorgecarleitao

jorgecarleitao commented Jan 1, 2021

Copy link
Copy Markdown
Member

Thanks a lot for helping out in the reviews, @Dandandan , much appreciated 💯

@asfimportasfimport mentioned this pull request Jan 1, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DandandanDandandanDandandan left review comments
@alambalambalamb approved these changes
@jorgecarleitaojorgecarleitaojorgecarleitao approved these changes

Assignees

No one assigned

Labels

Component: ParquetComponent: Rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mqy@alamb@codecov-io@Dandandan@jorgecarleitao