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

Incorrect deserialization for BigDecimal numbers #1770

Closed
cristian-mocanu-mob opened this issue Sep 19, 2017 · 6 comments
Closed

Incorrect deserialization for BigDecimal numbers #1770

cristian-mocanu-mob opened this issue Sep 19, 2017 · 6 comments
Labels
2.16 Issues planned for 2.16 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Milestone

Comments

@cristian-mocanu-mob
Copy link

If the number in JSON cannot be represented as Double and DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS is enabled, big numbers are returned as Infinity.

The test below should pass:

        final ObjectMapper objectMapper = new ObjectMapper();
        objectMapper.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS);

        final JsonNode jsonNode = objectMapper.readTree("7976931348623157e309");
        Assert.assertTrue(jsonNode instanceof NumericNode);

        // the following fails with NumberFormatException, because jsonNode is a DoubleNode with a value of POSITIVE_INFINITY
        Assert.assertTrue(jsonNode.decimalValue().compareTo(new BigDecimal("7976931348623157e309")) == 0);

The problem is reproducible in the latest released version, 2.9.1.

@cowtowncoder
Copy link
Member

Thank you for reporting this, does sound like a bug.

@cowtowncoder
Copy link
Member

Hmmh. This may be tricky to handle... problem being that detection of NaN case is triggered, and since value itself does round up to positive infinity (when attempted with double), value is not coerced as BigDecimal as it has no NaN options.

Will add failing test, see if I can figure out a way to handle this as expected.

@cowtowncoder
Copy link
Member

@GedMarc maybe this comment is for different issue... ?

@GedMarc
Copy link

GedMarc commented Jan 24, 2019

Oh Geez! This is for the double deserialization as float into big decimal!
This week has taken it out of me sigh, apologies!

@cowtowncoder cowtowncoder added ACTIVE and removed 2.9 labels Mar 19, 2019
@cowtowncoder cowtowncoder added 2.11 and removed 2.10 labels Mar 26, 2020
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 26, 2020

For context, looks like fix for #1028 possibly caused this. So code that should coerce value results in parser trying to determine if it got a NaN and... in a way it is, wrt range of Double. But it would not be for BigDecimal.

This is tricky to solve, JsonParser (actual JSON implementation) will need to be careful not to effectively coerce "too big"/"too small" numbers into positive/negative infinity, but only report true for isNaN() when actual marker is used. Or I need to figure out some other logic...
Seems likely, however, that this requires help from streaming level and can not be solved purely by databind.

@cowtowncoder cowtowncoder added 2.12 and removed 2.11 labels Apr 2, 2020
@cowtowncoder cowtowncoder changed the title incorrect deserialization for big decimal numbers Incorrect deserialization for BigDecimal numbers May 21, 2020
@cowtowncoder cowtowncoder added 2.13 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue and removed 2.12 labels Oct 27, 2020
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jul 15, 2021
@cowtowncoder
Copy link
Member

@pjfanning This issue is something where I was hoping lazy parsing might work. I do not remember all the details on exact failure but seems like this could maybe be revisited to see it was now solvable in 2.16.

cowtowncoder added a commit that referenced this issue Nov 8, 2023
dongjoon-hyun pushed a commit to apache/spark that referenced this issue Nov 18, 2023
### What changes were proposed in this pull request?
The pr aims to upgrade FasterXML jackson from 2.15.2 to 2.16.0.

### Why are the changes needed?
New version that fix some bugs, release notes as follows:
- 2.1.6.0 https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.16, eg:
[Databind](https://github.com/FasterXML/jackson-databind) [#1770](FasterXML/jackson-databind#1770): Incorrect deserialization for BigDecimal numbers
- 2.15.3 https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15.3, eg:
[Databind](https://github.com/FasterXML/jackson-databind) [#3968](FasterXML/jackson-databind#3968): Records with additional constructors failed to deserialize

The last upgrade occurred 6 months ago, #41414

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
Pass GA.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes #43859 from panbingkun/SPARK-45967.

Authored-by: panbingkun <pbk1982@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
szehon-ho pushed a commit to szehon-ho/spark that referenced this issue Aug 7, 2024
The pr aims to upgrade FasterXML jackson from 2.15.2 to 2.16.0.

New version that fix some bugs, release notes as follows:
- 2.1.6.0 https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.16, eg:
[Databind](https://github.com/FasterXML/jackson-databind) [apache#1770](FasterXML/jackson-databind#1770): Incorrect deserialization for BigDecimal numbers
- 2.15.3 https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15.3, eg:
[Databind](https://github.com/FasterXML/jackson-databind) [apache#3968](FasterXML/jackson-databind#3968): Records with additional constructors failed to deserialize

The last upgrade occurred 6 months ago, apache#41414

No.

Pass GA.

No.

Closes apache#43859 from panbingkun/SPARK-45967.

Authored-by: panbingkun <pbk1982@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issues planned for 2.16 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

3 participants