Accurately classifies all int values by size, rather than conservatively classifying all 4-byte ints as Java longs. #627
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
popematt
approved these changes
Oct 28, 2023
rmarrowstone
approved these changes
Oct 30, 2023
| @@ -143,10 +143,10 @@ private void testGetIntegerSizeIntBoundary(int boundaryValue, long pastBoundary) | |||
| { | |||
| in.next(); | |||
| // It's fine if IntegerSize recommends a larger-than-necessary type, just not a smaller one. | |||
There was a problem hiding this comment.
Suggested change
| // It's fine if IntegerSize recommends a larger-than-necessary type, just not a smaller one. |
…ely classifying all 4-byte ints as Java longs.
tgregg
force-pushed
the
accurate-integer-size
branch
from
00a49fc to
8d744c6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
Before this change, the new reader implementation classified all Ion
intvalues that consumed 4 binary Ion UInt bytes as Javalong. This was conservative, ensuring thatIonReader.getIntegerSize()would always return a Java type at least as large as necessary to hold the value (but sometimes larger). The goal was to simplify the implementation ofgetIntegerSize.However, some applications depend on
intvalues being classified accurately. Typically this is the case when an object mapping library like Jackson is used on top of ion-java for round-tripping values from Java classes. When a class is defined with a capital-IIntegerfield and that field is round-tripped as aLong, raw casts toIntegerwill fail. Additionally, unit tests that simplyassertEqualsthe expectedIntegerand the actualLongwill fail due to the type difference, even if the values are numerically equivalent.In order to avoid imposing pain on existing users, this PR proposes to accurately classify all
intvalues, such thatgetIntegerSizealways returns the smallest Java type necessary to hold the value without data loss. It turns out that doing this is not significantly more complicated, and has negligible performance impact. It should also be noted thatIonReader.getIntegerSize()is not a commonly-used method; most applications will implicitly know the maximum size ofintvalue that may exist in the data and simply call through to the corresponding IonReader value method directly.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.