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

Add JsonNodeFeature.USE_BIG_DECIMAL_FOR_FLOATS to allow overriding DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS #4801

Closed
cowtowncoder opened this issue Nov 19, 2024 · 8 comments · Fixed by #5058
Labels
2.19 Issues planned at 2.19 or later 3.0 Issue planned for initial 3.0 release

Comments

@cowtowncoder
Copy link
Member

Describe your Issue

Due to eager binding of number type when reading JsonNode -- basically, decision must be made between reading:

  1. Slightly faster, but potentially lossy Double vs
  2. Bit slower but unlimited range/precision BigDecimal

(to get either DoubleNode or BigDecimalNode)

there are cases where common buffering use cases expose unexpected precision loss -- particular since by default Double is used. See, for example:

FasterXML/jackson-modules-java8#326

This behavior can be changed via DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS, but that is global setting. Instead, it would perhaps make sense to add a JsonNodeFeature which would only affect JsonNode reading, but would have precedence over DeserializationFeature.

I haven't thought this fully through wrt implementation but this seems like potentially useful thing to have.

@cowtowncoder cowtowncoder added the 2.19 Issues planned at 2.19 or later label Nov 19, 2024
@cowtowncoder cowtowncoder added the 3.0 Issue planned for initial 3.0 release label Mar 12, 2025
@cowtowncoder
Copy link
Member Author

@JooHyukKim If you looking for things to work on, here is one.

@pjfanning
Copy link
Member

pjfanning commented Mar 22, 2025

Can we agree how the DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS and JsonNodeFeature.USE_BIG_DECIMAL_FOR_FLOATS enable/disable state interacts?

Can I suggest?

DeserializationFeature JsonNodeFeature Overall Effect
true true true
true false true
true undefined (default) true
false (default) true true
false (default) false false
false (default) undefined (default) false (default)

So if either DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS or JsonNodeFeature.USE_BIG_DECIMAL_FOR_FLOATS is true, then for JsonNode calculation, USE_BIG_DECIMAL_FOR_FLOATS is true.

@JooHyukKim
Copy link
Member

I concur @pjfanning

@cowtowncoder
Copy link
Member Author

Actually: one thing to consider -- JsonNodeFeatures have 3 states: enabled, disabled, undefined.

So we should have six states, but basically if JsonNodeFeature is "enabled" or "disabled", that takes effect as-is; otherwise ("undefined" / default), DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS setting (which only has "enabled" / "disabled").

Ability to have "undefined" (or "default") is one improvement new EnumFeature, JsonNodeFeature (and... upcoming DateTimeFeature?) has that is nice.

@pjfanning
Copy link
Member

@cowtowncoder I added undefined in #4801 (comment) -- does this make sense?

@cowtowncoder
Copy link
Member Author

@pjfanning almost but not quite (unless I misread it). What I think should happen is that since JsonNodeFeature.USE_BIG_DECIMAL_FOR_FLOATS is more specific, it should have precedence. Put it another way, logic can be expressed as:

  1. Default setting comes from DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS: true or false -- this is global setting for all datatypes
  2. JsonNodeFeature.USE_BIG_DECIMAL_FOR_FLOATS may override this, but only if it has been explicitly set astrueorfalse`.

So I think row 2 should be changed from

DeserializationFeature JsonNodeFeature Overall Effect
true false true

into

DeserializationFeature JsonNodeFeature Overall Effect
true false false

and otherwise table is ok.

Does this make sense?

@JooHyukKim
Copy link
Member

Okay final?

DeserializationFeature JsonNodeFeature Overall Effect
true true true
true false false
true undefined (default) true
false (default) true true
false (default) false false
false (default) undefined (default) false (default)

@JooHyukKim
Copy link
Member

Came up with #5058, seems easier than I thought 🤔

@cowtowncoder cowtowncoder changed the title Consider adding JsonNodeFeature to allow overriding DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS Add JsonNodeFeature.USE_BIG_DECIMAL_FOR_FLOATS to allow overriding DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS Apr 2, 2025
cowtowncoder added a commit that referenced this issue Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.19 Issues planned at 2.19 or later 3.0 Issue planned for initial 3.0 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants