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: Use Big Decimal for Tree #135

Merged
merged 3 commits into from
Mar 24, 2024
Merged

Fix: Use Big Decimal for Tree #135

merged 3 commits into from
Mar 24, 2024

Conversation

Shounaks
Copy link
Contributor

For Issue: #90

@Shounaks Shounaks force-pushed the 2.17-use-big-decimal-in-trees branch from 023fd94 to f3e98ff Compare March 17, 2024 00:58
@Shounaks
Copy link
Contributor Author

I believe this is the best route of action we can change without changing the base API for TreeCodec, only if that had the ability of storing the features, or atleast passing the features throught TreeCodec.readTree(...) then it would be much easier.

@Shounaks Shounaks marked this pull request as ready for review March 17, 2024 01:04
@Shounaks Shounaks requested a review from pjfanning March 17, 2024 01:59
@cowtowncoder
Copy link
Member

Quick note: needs to go against 2.18 branch.

cowtowncoder
cowtowncoder reviewed Mar 21, 2024
View reviewed changes
jr-stree/src/main/java/com/fasterxml/jackson/jr/stree/JacksonJrsTreeCodec.java Outdated
@@ -50,6 +65,16 @@ private JrsValue nodeFrom(JsonParser p) throws IOException
return JrsBoolean.FALSE;
case JsonTokenId.ID_NUMBER_INT:
case JsonTokenId.ID_NUMBER_FLOAT:
final JsonParser.NumberType n = p.getNumberType();
Copy link
Member

Choose a reason for hiding this comment

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

Cannot use this method, have a look at jackson-databind JsonNodeDeserializer (I think) for better version. Problem being that this method will coerce type because JSON has no native knowledge of optimal FP type to use.

cowtowncoder
cowtowncoder reviewed Mar 21, 2024
View reviewed changes
jr-stree/src/main/java/com/fasterxml/jackson/jr/stree/JacksonJrsTreeCodec.java Outdated
protected final ObjectCodec _objectCodec;

// @since 2.17
protected boolean _failOnDuplicateKeys;
protected boolean _useBigDecimalForFloat;

public JacksonJrsTreeCodec withFeature(JSON.Feature... features) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to require separate configurability of tree codec when JSON already has settings. There needs to be a way for codec to access those settings.

So why wouldn't this use the approach already added in #51?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My Initial concerns were that we need to use extensions, instead of already present classes.

Copy link
Member

Choose a reason for hiding this comment

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

Tree codec is already added as an extension isn't it.

@cowtowncoder
Copy link
Member

I am confused: #51 already showed the way to pass JSON.Feature flags... why wouldn't that approached be used?

Also: care must be taken when checking expected FP type; there have been lots of fine-tuning in jackson-databind to avoid changing natural type of non-JSON formats.

@Shounaks Shounaks force-pushed the 2.17-use-big-decimal-in-trees branch from 56674a2 to 44c904c Compare March 22, 2024 00:12
@Shounaks
Copy link
Contributor Author

Hi @cowtowncoder , I know about the other PR, but was hesitant to use Extension instead of already present constructs. Yes this can be done as you say (and i have made the changes for the same).

cowtowncoder
cowtowncoder approved these changes Mar 24, 2024
View reviewed changes
Copy link
Member

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

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

LGTM -- will merge later today

@cowtowncoder
Copy link
Member

@Shounaks Ahh! I finally understood what you meant -- if using JSON.builder().treeCodec() this wouldn't work. Good point.

Still, I think I will create a separate issue for deprecating that method so that tree codecs will rather be registered as extensions and not directly assigned as TreeCodecs -- exactly because of this problem.

@cowtowncoder cowtowncoder merged commit 961480c into FasterXML:2.17 Mar 24, 2024
4 checks passed
@Shounaks
Copy link
Contributor Author

Shounaks commented Mar 24, 2024 •
edited
Loading

yes exactly!

I think I will create a separate issue for deprecating that method so that tree codecs will rather be registered as extensions and not directly assigned as TreeCodecs -- exactly because of this problem.

Thanks!

@Shounaks Shounaks deleted the 2.17-use-big-decimal-in-trees branch March 24, 2024 01:52
cowtowncoder
cowtowncoder reviewed Mar 24, 2024
View reviewed changes
jr-stree/src/main/java/com/fasterxml/jackson/jr/stree/JacksonJrsTreeCodec.java
@@ -50,6 +59,9 @@ private JrsValue nodeFrom(JsonParser p) throws IOException
return JrsBoolean.FALSE;
case JsonTokenId.ID_NUMBER_INT:
case JsonTokenId.ID_NUMBER_FLOAT:
if (_useBigDecimalForDouble) {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this has bug -- now all integer numbers become BigDecimals too. I'll fix it.

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

@cowtowncoder cowtowncoder cowtowncoder approved these changes

@pjfanning pjfanning Awaiting requested review from pjfanning

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Shounaks @cowtowncoder @pjfanning