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

util-dynamodb should handle (and round-trip) largish floating point numbers #6571

Open
3 of 4 tasks
alex-at-cascade opened this issue Oct 16, 2024 · 1 comment
Open
3 of 4 tasks
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue

Comments

@alex-at-cascade
Copy link

Checkboxes for prior research

Describe the bug

After attempting to upgrade some code to the V3 SDK, we encountered the following problems. Both of the following now throw an error:

  • marshall({ foo: 1.23e+40 }): "Error: Number 1.23e+40 is greater than Number.MAX_SAFE_INTEGER. Use BigInt."
  • unmarshall({ foo: { N: "1.23e+40" } }): "Error: 1.23e+40 can't be converted to BigInt. Set options.wrapNumbers to get string value."

Neither of these cases should be an error, since they are well within the range of what DynamoDB supports for numbers. And both work exactly as expected with the old V2 Converter functions. (In addition, the suggestion to use BigInt for floating point values is bizarre. Related to this is that numbers lacking a decimal point but greater than MAX_SAFE_INTEGER do strangely get converted to BigInt instead of Number when unmarshalled, which then triggers an error when the result is passed into JSON.stringify.)

Regression Issue

  • Select this option if this issue appears to be a regression.

SDK version number

"@aws-sdk/util-dynamodb": "^3.670.0"

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v18.20.0

Reproduction Steps

const { unmarshall, marshall } = require("@aws-sdk/util-dynamodb");
marshall({ foo: 1.23e+40 });
unmarshall({ foo: { N: "1.23e+40" } });

Observed Behavior

In the first case: "Error: Number 1.23e+40 is greater than Number.MAX_SAFE_INTEGER. Use BigInt."
In the second case: "Error: 1.23e+40 can't be converted to BigInt. Set options.wrapNumbers to get string value."

Expected Behavior

Same as the V2 Converter, namely:

  • {foo: {N: "1.23e+40"}}
  • {foo: 1.23e+40}

Possible Solution

As this is a JS SDK, for numerical values, the converters should perform direct Number operations following typical JS conventions. (Any astonishing type conversions should be eliminated, unless specific options are provided.)

Additional Information/Context

No response

@alex-at-cascade alex-at-cascade added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 16, 2024
@zshzbh zshzbh self-assigned this Oct 17, 2024
@zshzbh zshzbh added p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Oct 17, 2024
@zshzbh
Copy link

zshzbh commented Oct 17, 2024

Hey @alex-at-cascade ,

I can reproduce this issue.

There's 2 issues I found

1 - The error messages you posted in this github issue. V2 marshall function support numbers that are bigger than Number.MAX_SAFE_INTEGER, but V3 requires a conversion. unmarshall function requires to convert the numbers that are bigger than Number.MAX_SAFE_INTEGER to convert to a string.

2 - The value is not correct if I use BigInt to fix the error in V3

const marshalled_V2 = AWS.DynamoDB.Converter.marshall({foo: 1.23e+40});
console.log("Marshalled_v2:", JSON.stringify(marshalled_V2, null, 2));
//V3
// Marshall a JavaScript object to DynamoDB format
const marshalled = marshall({ foo: BigInt(1.23e+40) });
console.log("Marshalled:", JSON.stringify(marshalled, null, 2));

The result I have

Marshalled_v2: {
  "foo": {
    "N": "1.23e+40"
  }
}
Marshalled: {
  "foo": {
    "N": "12299999999999999515319483038827796234240"
  }
}

1.23e+40 should be 1.23× 10^40, but the marshalled value is 12299999999999999515319483038827796234240.
This is due to BigInt is designed to represent integers, and the fractional part is truncated during the conversion.

I will report this issue to the SDK dev team and I will keep you updated!

Thanks!
Maggie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests

2 participants