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

Closed
3 of 4 tasks
alex-at-cascade opened this issue Oct 16, 2024 · 11 comments
Closed
3 of 4 tasks
Assignees
Labels
bug This issue is a bug. closing-soon This issue will automatically close in 4 days unless further comments are made. feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet. queued This issues is on the AWS team's backlog

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
Contributor

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

@kuhe
Copy link
Contributor

kuhe commented Oct 22, 2024

All numbers exceeding the absolute value of MAX_SAFE_INTEGER should be wrapped in NumberValue from @aws-sdk/lib-dynamodb.

See https://www.npmjs.com/package/@aws-sdk/lib-dynamodb with entry Large Numbers and NumberValue.

import { DynamoDB } from "@aws-sdk/client-dynamodb";
import { NumberValue, DynamoDBDocument } from "@aws-sdk/lib-dynamodb";

// Note, the client will not validate the acceptability of the number
// in terms of size or format.
// It is only here to preserve your precise representation.
const client = DynamoDBDocument.from(new DynamoDB({}));

await client.put({
  Item: {
    id: 1,
    smallNumber: NumberValue.from("123"),
    bigNumber: NumberValue.from("1000000000000000000000.000000000001"),
    nSet: new Set([123, NumberValue.from("456"), 789]),
  },
});

NumberValue is a non-computational string container for numerical values not accurately contained in number. If you use a large number library, you will need to convert it to string to give to the NumberValue wrapper, and from NumberValue's string on the return trip, if you set wrapNumbers=true on unmarshallOptions.

@alex-at-cascade
Copy link
Author

alex-at-cascade commented Oct 22, 2024

Seems like exceeding the absolute value of MAX_SAFE_INTEGER is not at all the same requirement as values not accurately contained in number. Something like 1.23e+40 can safely round-trip just using regular numbers. And since this is a javascript library, doing the least astonishing (in javascript) action by default would be the most sensible, which is what the V2 Converter seems to do. (If higher precision numbers in DynamoDB are coming from a non-javascript source, I would expect any special handling would only be enabled by a special option in the call.)

@alex-at-cascade
Copy link
Author

Also worth noting that one would normally expect 1.23e+40 and 1.23e-40 to be treated similarly, whereas the current implementation definitely does not do so. The root issue seems to be the confusion of magnitude versus precision.

@zshzbh
Copy link
Contributor

zshzbh commented Oct 29, 2024

Hey @alex-at-cascade,

Thanks for the feedback!

I should make it more clear - The root causes of the precision issue and the magnitude issue are the same, which is we are not using NumberValue. And the solutions towards these issues are also the same. I pushed a fix to update the error message to ask the user to use NumberValue from @aws-sdk/lib-dynamodb. I found the precision issue while troubleshooting this issue, so I also want to bring it up, just in case other people seeing this precision issue while troubleshooting this magnitude issue, so the user looking at this page would know to use NumberValue to resolve this issue.

As @kuhe replied earlier,

All numbers exceeding the absolute value of MAX_SAFE_INTEGER should be wrapped in NumberValue from @aws-sdk/lib-dynamodb.

Please use NumberValue to avoid the magnitude issue.

@zshzbh zshzbh added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Oct 29, 2024
@alex-at-cascade
Copy link
Author

A number like 1.23e+40 is perfectly fine in javascript (as the V2 Converter clearly demonstrates), it should not need to be wrapped in NumberValue, unless the consumer overrides default behavior with a special option. (Having to deep inspect every object for needless NumberValue wrappings for typical floating point numbers completely defeats the purpose of these utilities. It will force us to keep using the V2 Converter instead, since that works simply and perfectly.) The check for MAX_SAFE_INTEGER is completely pointless if someone is round-tripping from and to javascript - if a number was stringified in javascript, it can safely be unstringified directly.

@kuhe
Copy link
Contributor

kuhe commented Oct 29, 2024

I don't consider 1.23+40 to be fine in JS, since it is represented as 1.22999999999999995153194830388E40, despite its intact round trip. For large numbers, even though certain numbers can be represented exactly in Float64, they cannot be disambiguated from their neighboring values, so all of them are considered imprecise beyond the safe integer thresholds.

Here is how to handle it in the current version of the SDK, and below a feature we can implement to support ignoring precision:

const { unmarshall, marshall } = require("@aws-sdk/util-dynamodb");
const { NumberValue } = require("@aws-sdk/lib-dynamodb");

const impreciseNumber = 1.23e40;

const m = marshall({ foo: NumberValue.from(String(impreciseNumber)) }, {});

console.log("marshalled", m);

const u = unmarshall({ foo: { N: "1.23e+40" } }, { wrapNumbers: true });

console.log("unmarshalled", u);

function convertNumberValue(obj, mapper) {
  if (typeof obj !== "object" || !obj) {
    return obj;
  }
  for (const [key, val] of Object.entries(obj)) {
    if (val instanceof NumberValue) {
      obj[key] = mapper(obj[key]);
    } else {
      obj[key] = convertNumberValue(obj[key], mapper);
    }
  }
  return obj;
}

console.log("converted to JS number", convertNumberValue(u, Number));

Output:

marshalled { foo: { N: '1.23e+40' } }
unmarshalled { foo: NumberValue { value: '1.23e+40' } }
converted to JS number { foo: 1.23e+40 }

We don't want people to unsuspectingly use imprecise numbers, so I suggest we create the following opt-in features:

const impreciseNumber = 1.23e40;

// opt-in boolean to not throw error on imprecise number.
const m = marshall({ foo: impreciseNumber }, { allowImpreciseNumbers: true });

// allow a function to be used in the wrapNumbers option. 
// If you specify `Number`, then even large numeric values will be converted to the basic number type.
const u = unmarshall({ foo: { N: "1.23e+40" } }, { wrapNumbers: Number });

@kuhe kuhe added queued This issues is on the AWS team's backlog feature-request New feature or enhancement. May require GitHub community feedback. labels Oct 29, 2024
@alex-at-cascade
Copy link
Author

As long as there's a way (via an option) to round-trip in javascript with a minimum of fuss (which will by far be the most common use of this library), it will be usable at least. (I do understand that, in the unusual case of higher precision numbers generated from some other source/language, there could be loss of precision on conversion - but even then, checking against MAX_SAFE_INTEGER might be the wrong check, since a small floating point number from another source could have many more significant digits than javascript supports and we would still lose precision.)

@alex-at-cascade
Copy link
Author

One could also consider, when unmarshalling, given const n = Number(a.N), if a.N === marshall(n).N always return n regardless of options, since we'd be generating a javascript value that would give the identical DynamoDB value when sent back in.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Oct 30, 2024
@kuhe kuhe self-assigned this Nov 1, 2024
@kuhe kuhe added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Nov 11, 2024
@kuhe
Copy link
Contributor

kuhe commented Nov 11, 2024

Ran has contributed this feature, as of https://www.npmjs.com/package/@aws-sdk/lib-dynamodb/v/3.689.0.

The syntax to use numbers and ignore precision is:

import { DynamoDB } from "@aws-sdk/client-dynamodb";
import { DynamoDBDocument } from "@aws-sdk/lib-dynamodb";

const ddb = new DynamoDB();
const doc = DynamoDBDocument.from(ddb, {
  marshallOptions: {
    allowImpreciseNumbers: true,
  },
  unmarshallOptions: {
    wrapNumbers: Number,
  },
});

await doc.put({
  TableName: "test",
  Item: {
    id: "imprecise-number",
    data: 1.23e40,
  },
});

const get = await doc.get({
  TableName: "test",
  Key: {
    id: "imprecise-number",
  },
});

console.log("get", get);
get {
  '$metadata': {
    httpStatusCode: 200,
    requestId: '....',
    extendedRequestId: undefined,
    cfId: undefined,
    attempts: 1,
    totalRetryDelay: 0
  },
  Item: { id: 'imprecise-number', data: 1.23e+40 }
}

@kuhe kuhe added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 11, 2024
@kuhe kuhe assigned RanVaknin and unassigned kuhe Nov 11, 2024
@kuhe kuhe closed this as completed Nov 11, 2024
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. closing-soon This issue will automatically close in 4 days unless further comments are made. feature-request New feature or enhancement. May require GitHub community feedback. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet. queued This issues is on the AWS team's backlog
Projects
None yet
Development

No branches or pull requests

4 participants