Skip to content

Conversation

@nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Jan 16, 2021

Please see the JIRA for the issue details https://issues.apache.org/jira/browse/ARROW-11271.

@nevi-me nevi-me requested a review from sunchao January 16, 2021 20:11
@github-actions
Copy link

@nevi-me
Copy link
Contributor Author

nevi-me commented Jan 16, 2021

This is not as severe as i thought, so it's fine if it doesn't make it to 3.0.0 @alamb @kszucs

@codecov-io
Copy link

codecov-io commented Jan 16, 2021

Codecov Report

Merging #9216 (6966a9a) into master (1393188) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9216   +/-   ##
=======================================
  Coverage   81.61%   81.61%           
=======================================
  Files         215      215           
  Lines       51867    51891   +24     
=======================================
+ Hits        42329    42351   +22     
- Misses       9538     9540    +2     
Impacted Files Coverage Δ
rust/parquet/src/arrow/schema.rs 91.66% <100.00%> (+0.16%) ⬆️
rust/parquet/src/encodings/encoding.rs 94.86% <0.00%> (-0.20%) ⬇️

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 eaa7b7a...6966a9a. Read the comment docs.

@kszucs
Copy link
Member

kszucs commented Jan 17, 2021

This is not as severe as i thought, so it's fine if it doesn't make it to 3.0.0 @alamb @kszucs

We have packaging issues so we probably need to create another release candidate. We can include this patch as well.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

I reviewed the code in this PR and I think it does what the description says. It also seems to be a relatively small change with minimal impacts (and reduces the number of panic! in the code). I think we can merge it. I will do so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants