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

BigDecimal invalid support #413

Closed
dybuk87 opened this issue Mar 28, 2018 · 11 comments
Closed

BigDecimal invalid support #413

dybuk87 opened this issue Mar 28, 2018 · 11 comments

Comments

@dybuk87
Copy link

dybuk87 commented Mar 28, 2018

Hi,
I think BigDecimal support is misleading in current version, currently json :
new JSONObject(" { "value" : 34300000000000000000000000.14 }").getBigDecimal("value").toPlainString()
will return 34300000000000000000000000 instead of correct value 34300000000000000000000000.14

I found in code

// if we want full Big Number support this block can be replaced with:
// return stringToNumber(string);
if (isDecimalNotation(string)) {

After uncommenting this line support is valid and I believe this should be default instead of current one because there is a lot of situation where you need full precision like money amount, or at least there should be some configuration for json parser.

@johnjaylward
Copy link
Contributor

I'll take a look at this

@johnjaylward
Copy link
Contributor

@stleary , while this is "working as designed", I do agree with @dybuk87 that we should consider changing the default handling of numbers. What's happening in this example is that the stringToValue parser is always using a Double for the representation. That is causing the value to be truncated here.

Let me know your thoughts.

@stleary
Copy link
Owner

stleary commented Mar 28, 2018

@johnjaylward Haven't looked at the code yet, but this sounds reasonable given that BigNumber is a recent enhancement.

@urfuchs
Copy link

urfuchs commented Oct 2, 2018

There is a difference between getBigDecimal and optBigDecimal.

JSONObject json = new JSONObject("{ \"key\" : 72.35 }");
BigDecimal bd1 = json.getBigDecimal(„key“);
BigDecimal bd2 = json.optBigDecimal(„key“, null);
System.out.println(bd1);
System.out.println(bd2);

prints:

72.35
72.349999999999994315658113919198513031005859375

Why is there a difference?

getBigDecimal converts the double to a String and than to a BigDecimal:
new BigDecimal(Double.valueOf(72.35).toString())
optBigDecimal converts the double directly to BigDecimal: new BigDecimal(Double.valueOf(72.35))

@johnjaylward
Copy link
Contributor

@urfuchs can you open a new issue for this? it's a different problem than the original request

@urfuchs
Copy link

urfuchs commented Oct 2, 2018

There is a difference between getBigDecimal and optBigDecimal.

JSONObject json = new JSONObject("{ \"key\" : 72.35 }");
BigDecimal bd1 = json.getBigDecimal(„key“);
BigDecimal bd2 = json.optBigDecimal(„key“, null);
System.out.println(bd1);
System.out.println(bd2);

prints:

72.35
72.349999999999994315658113919198513031005859375

Why is there a difference?

getBigDecimal converts the double to a String and than to a BigDecimal:
new BigDecimal(Double.valueOf(72.35).toString())
optBigDecimal converts the double directly to BigDecimal: new BigDecimal(Double.valueOf(72.35))

moved to #438

@stleary
Copy link
Owner

stleary commented Dec 9, 2018

Working as designed.

@stleary stleary closed this as completed Dec 9, 2018
@borgogelli
Copy link

Same code, but the output for me is:

72.349999999999994315658113919198513031005859375
72.349999999999994315658113919198513031005859375

I use the 20200518 version

@johnjaylward
Copy link
Contributor

@borgogelli , I'm not sure I understand what action you are expecting from your comment. Are you confirming the fix worked, or do you expect the behaviour to be different?

@borgogelli
Copy link

Hi @johnjaylward , both the things. First the output of my code is different from the code posted by @urfuchs . And then I have expected the result to be 72.35

@johnjaylward
Copy link
Contributor

The recommendation when using the *BigDecimal functions is to specify your scale. We purposefully expose the full double with rounding errors due to use cases like this:

double myDouble = someComplexCalculation();
JSONObject jo = new JSONObject();
jo.put("myDouble", myDouble);
// ... some other code
someFunction(jo.getBigDecimal("myDouble"));

The old method of using the string value of the underlying double did preserve the value when parsed from a string, but it lost some complexities if the double was originally stored from a calculation.

PR #453 basically solves the issue for both use cases by preserving the textual representation when parsing (by using a BigDecimal for storage), but allowing complex calculations to be stored as a double or float.

Until #453 is accepted, the expected use would be to do something like (use setScale):

double myDouble = someComplexCalculation();
JSONObject jo = new JSONObject();
jo.put("myDouble", myDouble);
// ... some other code
someFunction(jo.getBigDecimal("myDouble").setScale(5, RoundingMode.ROUND_HALF_EVEN));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants