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

Fix data corruption in json decoder f64-to-i64 cast #652

Merged
merged 3 commits into from
Aug 2, 2021

Conversation

xianwill
Copy link
Contributor

@xianwill xianwill commented Aug 2, 2021

Which issue does this PR close?

Closes #653

Rationale for this change

Described in issue #653

What changes are included in this PR?

Add special handling for i64 and u64 to the build_primitive_array method in Decoder. By avoiding the cast to f64 we avoid the data corruption.

Are there any user-facing changes?

No

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 2, 2021
@xianwill xianwill marked this pull request as ready for review August 2, 2021 16:25
@houqp houqp requested a review from nevi-me August 2, 2021 16:59
Copy link
Member

@houqp houqp left a comment

Choose a reason for hiding this comment

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

I was going to cc you when I saw the PR, but then I noticed you actually wrote it :D

@houqp
Copy link
Member

houqp commented Aug 2, 2021

@xianwill there is rustfmt error that needs to be addressed.

@xianwill
Copy link
Contributor Author

xianwill commented Aug 2, 2021

@xianwill there is rustfmt error that needs to be addressed.

@houqp - Fixed

@houqp houqp added the bug label Aug 2, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2021

Codecov Report

Merging #652 (980f990) into master (b38a4b6) will increase coverage by 0.00%.
The diff coverage is 75.00%.

❗ Current head 980f990 differs from pull request most recent head d916826. Consider uploading reports for the commit d916826 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #652   +/-   ##
=======================================
  Coverage   82.50%   82.50%           
=======================================
  Files         168      168           
  Lines       47237    47243    +6     
=======================================
+ Hits        38971    38977    +6     
  Misses       8266     8266           
Impacted Files Coverage Δ
arrow/src/json/reader.rs 83.99% <75.00%> (-0.01%) ⬇️
parquet/src/encodings/encoding.rs 95.04% <0.00%> (+0.19%) ⬆️

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 b38a4b6...d916826. Read the comment docs.

Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

LGTM

@nevi-me nevi-me merged commit b075c3c into apache:master Aug 2, 2021
alamb pushed a commit that referenced this pull request Aug 5, 2021
* Add failing test for JSON writer i64 bug

* Add special handling for i64/u64 to json decoder array builder

* Fix linter error - linter wants .flatten on a new line
alamb added a commit that referenced this pull request Aug 6, 2021
* Add failing test for JSON writer i64 bug

* Add special handling for i64/u64 to json decoder array builder

* Fix linter error - linter wants .flatten on a new line

Co-authored-by: Christian Williams <christianwilliams79@gmail.com>
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 bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON decoder data corruption for large i64/u64
5 participants