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.

Sign up for GitHub

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

fix(NODE-5546): decimal 128 fromString performs inexact rounding #613

Merged
merged 5 commits into from
Aug 24, 2023
Merged

fix(NODE-5546): decimal 128 fromString performs inexact rounding #613

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
097190b
fix(NODE-5546): fix additional rounding bug
W-A-James Aug 21, 2023
a806d53
fix(NODE-5546): fix fromString implementation
W-A-James Aug 23, 2023
1177d20
test(NODE-5546): sync decimal128 tests
W-A-James Aug 23, 2023
36a098c
fix(NODE-5546): remove unused variable and restore gte sign
W-A-James Aug 24, 2023
301a7d8
test(NODE-5546): Add new unit tests
W-A-James Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 31 additions & 56 deletions src/decimal128.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export class Decimal128 extends BSONValue {
static fromString(representation: string): Decimal128 {
// Parse state tracking
let isNegative = false;
let sawSign = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎵 I saw the sign
And it opened up my eyes, I saw the sign
Life is demanding without understanding 🎵

let sawRadix = false;
let foundNonZero = false;

Expand Down Expand Up @@ -187,8 +188,6 @@ export class Decimal128 extends BSONValue {

// Exponent
let exponent = 0;
// loop index over array
let i = 0;
// The high 17 digits of the significand
let significandHigh = new Long(0, 0);
// The low 17 digits of the significand
Expand Down Expand Up @@ -241,6 +240,7 @@ export class Decimal128 extends BSONValue {

// Get the negative or positive sign
if (representation[index] === '+' || representation[index] === '-') {
sawSign = true;
isNegative = representation[index++] === '-';
}

Expand All @@ -263,7 +263,7 @@ export class Decimal128 extends BSONValue {
continue;
}

if (nDigitsStored < 34) {
if (nDigitsStored < MAX_DIGITS) {
if (representation[index] !== '0' || foundNonZero) {
if (!foundNonZero) {
firstNonZero = nDigitsRead;
Expand Down Expand Up @@ -320,7 +320,11 @@ export class Decimal128 extends BSONValue {
lastDigit = nDigitsStored - 1;
significantDigits = nDigits;
if (significantDigits !== 1) {
while (digits[firstNonZero + significantDigits - 1] === 0) {
while (
representation[
firstNonZero + significantDigits - 1 + Number(sawSign) + Number(sawRadix)
] === '0'
) {
significantDigits = significantDigits - 1;
}
}
Expand All @@ -331,7 +335,7 @@ export class Decimal128 extends BSONValue {
// to represent user input

// Overflow prevention
if (exponent <= radixPosition && radixPosition - exponent > 1 << 14) {
if (exponent <= radixPosition && radixPosition > exponent + (1 << 14)) {
exponent = EXPONENT_MIN;
} else {
exponent = exponent - radixPosition;
Expand All @@ -342,10 +346,9 @@ export class Decimal128 extends BSONValue {
// Shift exponent to significand and decrease
lastDigit = lastDigit + 1;

if (lastDigit - firstDigit > MAX_DIGITS) {
if (lastDigit - firstDigit >= MAX_DIGITS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this with a test case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it, firstDigit is always 0; it doesn't change at all throughout the entire function, so I'm going to go ahead and remove all references to it.

With regards to changing to the gte sign, the cases that trigger this branch are clamped zeros with large positive increments (see spec tests in decimal128-1.json), but any number with a sufficiently large exponent can enter this block.

If we consider a number like 1000000000000000000000000000000000e6112, which has 1 significant digit, 34 digits before the exponent and an exponent that's above max exponent, then what happens when we have a gt sign here is that we end up with the following result:

> Decimal128.fromString('1000000000000000000000000000000000e6112')
new Decimal128("0E+6111")

since we skip the iteration when lastDigit reaches MAX_DIGITS and proceed without error despite the fact that we should have overflowed. This leads us to get to our encoding and produce the incorrect result we see above.

With the gte sign in place, we get the following result, which is what we expect

> Decimal128.fromString('1000000000000000000000000000000000e6112')
Uncaught:
BSONError: "1000000000000000000000000000000000e6112" is not a valid Decimal128 string - overflow
    at invalidErr (/home/wajames/js-bson/lib/bson.cjs:1402:11)
    at Decimal128.fromString (/home/wajames/js-bson/lib/bson.cjs:1540:17)

it seems like we should have spec test that test corner cases like this.

// Check if we have a zero then just hard clamp, otherwise fail
const digitsString = digits.join('');
if (digitsString.match(/^0+$/)) {
if (significantDigits === 0) {
exponent = EXPONENT_MAX;
break;
}
Expand All @@ -357,85 +360,57 @@ export class Decimal128 extends BSONValue {

while (exponent < EXPONENT_MIN || nDigitsStored < nDigits) {
// Shift last digit. can only do this if < significant digits than # stored.
if (lastDigit === 0 && significantDigits < nDigitsStored) {
exponent = EXPONENT_MIN;
significantDigits = 0;
break;
if (lastDigit === 0) {
if (significantDigits === 0) {
exponent = EXPONENT_MIN;
break;
}

invalidErr(representation, 'exponent underflow');
}

if (nDigitsStored < nDigits) {
if (
representation[nDigits - 1 + Number(sawSign) + Number(sawRadix)] !== '0' &&
significantDigits !== 0
) {
invalidErr(representation, 'inexact rounding not allowed');
}
// adjust to match digits not stored
nDigits = nDigits - 1;
} else {
if (digits[lastDigit] !== 0) {
invalidErr(representation, 'inexact rounding not allowed');
}
// adjust to round
lastDigit = lastDigit - 1;
}

if (exponent < EXPONENT_MAX) {
exponent = exponent + 1;
} else {
// Check if we have a zero then just hard clamp, otherwise fail
const digitsString = digits.join('');
if (digitsString.match(/^0+$/)) {
exponent = EXPONENT_MAX;
break;
}
invalidErr(representation, 'overflow');
}
}

// Round
// We've normalized the exponent, but might still need to round.
if (lastDigit - firstDigit + 1 < significantDigits) {
let endOfString = nDigitsRead;

// If we have seen a radix point, 'string' is 1 longer than we have
// documented with ndigits_read, so inc the position of the first nonzero
// digit and the position that digits are read to.
if (sawRadix) {
firstNonZero = firstNonZero + 1;
endOfString = endOfString + 1;
}
// if negative, we need to increment again to account for - sign at start.
if (isNegative) {
// if saw sign, we need to increment again to account for - or + sign at start.
if (sawSign) {
firstNonZero = firstNonZero + 1;
endOfString = endOfString + 1;
}

const roundDigit = parseInt(representation[firstNonZero + lastDigit + 1], 10);
let roundBit = 0;

if (roundDigit >= 5) {
roundBit = 1;
if (roundDigit === 5) {
roundBit = digits[lastDigit] % 2 === 1 ? 1 : 0;
for (i = firstNonZero + lastDigit + 2; i < endOfString; i++) {
if (parseInt(representation[i], 10)) {
roundBit = 1;
break;
}
}
}
}

if (roundBit) {
let dIdx = lastDigit;

for (; dIdx >= 0; dIdx--) {
if (++digits[dIdx] > 9) {
digits[dIdx] = 0;

// overflowed most significant digit
if (dIdx === 0) {
if (exponent < EXPONENT_MAX) {
exponent = exponent + 1;
digits[dIdx] = 1;
} else {
return new Decimal128(isNegative ? INF_NEGATIVE_BUFFER : INF_POSITIVE_BUFFER);
}
}
}
}
if (roundDigit !== 0) {
invalidErr(representation, 'inexact rounding not allowed');
}
}

Expand Down
5 changes: 0 additions & 5 deletions test/node/bson_corpus.spec.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,8 @@ function normalize(cEJ) {

const parseErrorForDecimal128 = scenario => {
// TODO(NODE-3637): remove regex of skipped tests and and add errors to d128 parsing
const skipRegex = /dqbsr|Inexact/;
for (const parseError of scenario.parseErrors) {
it(parseError.description, function () {
if (skipRegex.test(parseError.description)) {
this.skip();
}

expect(
() => BSON.Decimal128.fromString(parseError.string),
`Decimal.fromString('${parseError.string}') should throw`
Expand Down
24 changes: 24 additions & 0 deletions test/node/specs/bson-corpus/decimal128-1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,30 @@
"canonical_bson": "18000000136400000000000a5bc138938d44c64d31cc3700",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"1.000000000000000000000000000000000E+999\"}}"
},
{
"description": "Clamped zeros with a large positive exponent",
"canonical_bson": "180000001364000000000000000000000000000000FE5F00",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+6111\"}}"
},
{
"description": "Clamped zeros with a large negative exponent",
"canonical_bson": "180000001364000000000000000000000000000000000000",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-6176\"}}"
},
{
"description": "Clamped negative zeros with a large positive exponent",
"canonical_bson": "180000001364000000000000000000000000000000FEDF00",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+6111\"}}"
},
{
"description": "Clamped negative zeros with a large negative exponent",
"canonical_bson": "180000001364000000000000000000000000000000008000",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-6176\"}}"
}
]
}