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 serialization of large integers in JSON (#5038) #5042

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Fix serialization of large integers in JSON (#5038) #5042

merged 1 commit into from
Nov 6, 2023

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Nov 6, 2023 •
edited
Loading

Which issue does this PR close?

Closes #5038

Rationale for this change

When converting i32 to i64 by default Rust will sign extend, we don't want this behaviour

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Nov 6, 2023
@alamb alamb changed the title Fix serialization of large integers (#5038) Fix serialization of large integers in JSON (#5038) Nov 6, 2023
alamb
alamb reviewed Nov 6, 2023
View reviewed changes
arrow-json/src/reader/tape.rs
@@ -180,7 +180,7 @@ impl<'a> Tape<'a> {
TapeElement::Null => out.push_str("null"),
TapeElement::I64(high) => match self.get(idx + 1) {
TapeElement::I32(low) => {
let val = (high as i64) << 32 | low as i64;
let val = (high as i64) << 32 | (low as u32) as i64;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm this code is only used for debugging? it wasn't clear to me that this is covered by a test, but if it is only used in

arrow-rs/arrow-json/src/reader/tape.rs

Line 210 in 08b8cba

self.serialize(&mut out, idx);
I don't think that is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only used in error messages

alamb
alamb approved these changes Nov 6, 2023
View reviewed changes
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 also double checked and it seems like this pattern to use the unsigned variant is done for all other uses in arrow:

Screenshot 2023-11-06 at 8 56 11 AM

@tustvold tustvold merged commit 91acfb0 into apache:master Nov 6, 2023
24 checks passed
@alamb alamb mentioned this pull request Nov 7, 2023
Closed
6 tasks
@tustvold tustvold mentioned this pull request Nov 7, 2023
Closed
alamb pushed a commit to alamb/arrow-rs that referenced this pull request Nov 8, 2023
@tustvold @alamb
Fix serialization of large integers (apache#5038) (apache#5042)
f179188
@alamb alamb mentioned this pull request Nov 8, 2023
Merged
alamb added a commit that referenced this pull request Nov 9, 2023
@alamb @tustvold
Backport: Fix serialization of large integers (#5038) (#5042) to `48.…
a5027e8 
…0.0_maintenance` (#5059)

* Fix serialization of large integers (#5038) (#5042)

* fmt

---------

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
arrow Changes to the arrow crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression when serializing large json numbers
2 participants
@tustvold @alamb