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.

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

Update test output for Rust 1.58 release #1173

Closed
wants to merge 2 commits into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jan 14, 2022

Rationale for this change

Test output changed with rust 1.58

What changes are included in this PR?

  1. Add a reference to non_send_fields_in_send_ty seems to be misguided rust-lang/rust-clippy#8045 explaining why that lint is disabled
  2. Update test output due to change in rust: rust-lang/rust@8731d4d#diff-363ad34c0858be71721f73b0880238813baf8f8b36ebbb494baa6b29103dd8d5 / Automatic exponential formatting in Debug rust-lang/rust#86479

Kudos to @jhorstmann for researching both of those

Are there any user-facing changes?

No

@alamb alamb changed the title Fix tests for Rust 1.58 release Update test output for Rust 1.58 release Jan 14, 2022
@github-actions github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels Jan 14, 2022
@alamb alamb requested a review from sunchao January 14, 2022 16:34
Copy link
Contributor Author

@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 plan to merge this PR in to get the CI green again in this repo, but I would love to hear any thoughts that @sunchao has (e.g. if the change in rust has now introduced a bug in parquet)

@@ -1081,9 +1081,9 @@ mod tests {
fn test_convert_float_to_string() {
assert_eq!(format!("{}", Field::Float(1.0)), "1.0");
assert_eq!(format!("{}", Field::Float(9.63)), "9.63");
assert_eq!(format!("{}", Field::Float(1e-15)), "0.000000000000001");
assert_eq!(format!("{}", Field::Float(1e-15)), "1e-15");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know if this is correct output or not

The code that handles formatting this is here: https://github.com/apache/arrow-rs/blob/master/parquet/src/record/api.rs#L716-L729

Which came in from @sunchao in the original donation 3 years ago in 6d12823

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the same question as @jhorstmann on #1169

I don't know why we assert that exact formatting though. Also seems intentional because there is a range check in fmt::Display for parquet::record::api.

Copy link
Contributor

Choose a reason for hiding this comment

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

Only just seeing this, but I created #1178 which preserves the old behaviour as far as I could guess at it. Happy for you to go with this PR though if you would prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1178 looks better to me .

@codecov-commenter
Copy link

Codecov Report

Merging #1173 (0f033b6) into master (231cf78) will increase coverage by 0.04%.
The diff coverage is 77.27%.

❗ Current head 0f033b6 differs from pull request most recent head 466394a. Consider uploading reports for the commit 466394a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
+ Coverage   82.59%   82.63%   +0.04%     
==========================================
  Files         173      173              
  Lines       50919    50858      -61     
==========================================
- Hits        42055    42026      -29     
+ Misses       8864     8832      -32     
Impacted Files Coverage Δ
arrow/src/buffer/immutable.rs 98.92% <ø> (ø)
parquet/src/schema/printer.rs 72.47% <50.00%> (ø)
arrow/src/compute/kernels/arithmetic.rs 90.86% <73.33%> (+5.29%) ⬆️
arrow/src/util/test_util.rs 92.04% <100.00%> (ø)
parquet/src/record/api.rs 91.61% <100.00%> (ø)

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 5fb0534...466394a. Read the comment docs.

@alamb alamb closed this Jan 15, 2022
@alamb alamb deleted the alamb/clippy3 branch January 15, 2022 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants