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

Document.FromJson() changes the decimal value #1555

Open
simon10says opened this issue Mar 13, 2020 · 13 comments
Open

Document.FromJson() changes the decimal value #1555

simon10says opened this issue Mar 13, 2020 · 13 comments
Assignees
Labels
breaking-change This issue requires a breaking change to remediate. bug This issue is a bug. dynamodb p2 This is a standard priority issue queued v4

Comments

@simon10says
Copy link

When using Document.FromJson(), the decimal value always increment/decrement by 1 when the number of digits are more than 18.

Expected Behavior

The FromJson() conversion should not alter the value and should support up to 38 digits of precision as supported by DynamoDB.

Current Behavior

  1. If I've about 18 digits, the last value always increment/decrement by 1. E.g. xxx.1234, will become xxx.1233 (where xxx are the first 14 digits)

Steps to Reproduce (for bugs)

Create a blank Console application in Visual Studio and run the code below:

static void Main(string[] args)
{
    var doc = Document.FromJson("{\"decimal\":1584097275961.1234}");
    Console.WriteLine($"doc: {doc["decimal"]}");
}

You will see the result doc: 1584097275961.1233; it should be xxx.1234

Your Environment

  • AWSSKD.dynamoDBv2: 3.3.105.9
  • Visual Studio version: Microsoft Visual Studio Community 2019 Version 16.4.5
@simon10says
Copy link
Author

Related to #1039

@klaytaybai
Copy link
Contributor

Hi @simon10says, thanks for reviving interest in this issue. It is concerning to me. However, due to C# limitations with number precision at high digits and our current implementation, a decision was made to wait until a next major version for a fix. We recommend that anybody using large numbers that require greater precision than offered by the decimal class use strings to store these numbers instead so as to not lose any precision.

Since the previous issue is so old and might not get as much attention, I'm going to mark that one as a duplicate and keep this one open going forward.

@klaytaybai klaytaybai added breaking-change This issue requires a breaking change to remediate. bug This issue is a bug. documentation This is a problem with documentation. labels Mar 16, 2020
@simon10says
Copy link
Author

Hi @klaytaybai , our teams had already used the workaround you suggested, so, we are good for now. However, IMHO, I urge you to add the workaround into the next release asap because this is the kind of bug that causes a 'ghost' effect in the code and the result, which go undetected by the developers and end-user, but caused tainted data; and I wonder how many have gone undetected with this bug. For us,

  1. We capped our precision to 28 digits as defined by C# decimal even thought dynamoDB is limited to 38 digits
  2. One of our developer decided to add in a test case for 18 digits precision because after our long internal testing, we always get the 'good' result and have never hit the 'bad' result' and it is the 18th digital precision that is the key
  3. We spent days debugging this and the very last thing we could have suspect is this FromJson()

Bugs could be fixed; but if we had went live with tainted data, it will be catastrophe

@NimaAra
Copy link

NimaAra commented Oct 21, 2020

This bug needs to be fixed. Any update on this please?

@ashishdhingra ashishdhingra added dynamodb doc-apireference and removed doc-apireference documentation This is a problem with documentation. labels Oct 23, 2020
@github-actions
Copy link

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Oct 30, 2021
@github-actions github-actions bot closed this as completed Nov 1, 2021
@Kralizek
Copy link
Contributor

Kralizek commented Nov 1, 2021

@ashishdhingra is it normal that issues with the label "bug" are closed automatically for staleness?

@github-actions
Copy link

github-actions bot commented Nov 2, 2022

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 2, 2022
@ashishdhingra
Copy link
Contributor

No close

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 3, 2022
@ashishdhingra ashishdhingra added needs-review p2 This is a standard priority issue labels Dec 14, 2022
@ashishdhingra
Copy link
Contributor

Reproducible using customer provided code. The value gets changed from 1584097275961.1234 to 1584097275961.1233.
Analysis:

  • Amazon.DynamoDBv2.DocumentModel.JsonUtils.FromJson() uses ThirdParty.Json.LitJson behind the scenes. JsonMapper.ReadValue() is used to value from JSON.
    • It uses JsonReader::StringReader behind the scenes.
      • It uses JsonReader.ProcessNumber() to parse string representation of number. Until this point, value 1584097275961.1234 is intact. Thereafter, it uses Double.TryParse() which changes value to 1584097275961.1233.
      • Decimal type is recommended for high precision (use Double for faster speed with some loss in precision). For instance, Convert.ToDecimal(number) with input as "1584097275961.1234" preserves the value 1584097275961.1234.

Per Floating-point numeric types, below are the precision limits:

C# type/keyword Approximate range Precision Size .NET type
float ±1.5 x 10−45 to ±3.4 x 1038 ~6-9 digits 4 bytes System.Single
double ±5.0 × 10−324 to ±1.7 × 10308 ~15-17 digits 8 bytes System.Double
decimal ±1.0 x 10-28 to ±7.9228 x 1028 28-29 digits 16 bytes System.Decimal

For customer's scenario, System.Decimal is the appropriate type as of now.

@Lanayx
Copy link

Lanayx commented Nov 5, 2024

Another case:

0.0000087 in JSON gets translated into "8.7e-06", while should stay as is

@ashishdhingra
Copy link
Contributor

ashishdhingra commented Nov 6, 2024

PR #3534, addressing this issue, has been merged into our next major version which we are are currently on preview 4. We are hoping to a preview 5 within the next couple weeks that will include this change.

Keeping this issue open until preview 5 is released.

@ashishdhingra ashishdhingra self-assigned this Nov 6, 2024
@Kralizek
Copy link
Contributor

Kralizek commented Nov 6, 2024

Is there a possibility to backport the fix to V3?

@normj
Copy link
Member

normj commented Nov 6, 2024

@Kralizek If it was a simple merge I would but we don't have System.Text.Json available in all of the targets for V3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This issue requires a breaking change to remediate. bug This issue is a bug. dynamodb p2 This is a standard priority issue queued v4
Projects
None yet
Development

No branches or pull requests

7 participants