Skip to content

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

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

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 all 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
102 changes: 35 additions & 67 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 All @@ -180,15 +181,11 @@ export class Decimal128 extends BSONValue {
let nDigitsStored = 0;
// Insertion pointer for digits
let digitsInsert = 0;
// The index of the first non-zero digit
let firstDigit = 0;
// The index of the last digit
let lastDigit = 0;

// 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 +238,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 +261,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 @@ -307,11 +305,7 @@ export class Decimal128 extends BSONValue {

// Done reading input
// Find first non-zero digit in digits
firstDigit = 0;

if (!nDigitsStored) {
firstDigit = 0;
lastDigit = 0;
digits[0] = 0;
nDigits = 1;
nDigitsStored = 1;
Expand All @@ -320,7 +314,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 +329,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 @@ -341,11 +339,9 @@ export class Decimal128 extends BSONValue {
while (exponent > EXPONENT_MAX) {
// Shift exponent to significand and decrease
lastDigit = lastDigit + 1;

if (lastDigit - firstDigit > MAX_DIGITS) {
if (lastDigit >= MAX_DIGITS) {
// 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 +353,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');
}
// adjust to match digits not stored
nDigits = nDigits - 1;
} else {
if (digits[lastDigit] !== 0) {
invalidErr(representation, 'inexact rounding');
}
// 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 (lastDigit + 1 < significantDigits) {
// 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');
}
}

Expand All @@ -449,8 +417,8 @@ export class Decimal128 extends BSONValue {
if (significantDigits === 0) {
significandHigh = Long.fromNumber(0);
significandLow = Long.fromNumber(0);
} else if (lastDigit - firstDigit < 17) {
let dIdx = firstDigit;
} else if (lastDigit < 17) {
let dIdx = 0;
significandLow = Long.fromNumber(digits[dIdx++]);
significandHigh = new Long(0, 0);

Expand All @@ -459,7 +427,7 @@ export class Decimal128 extends BSONValue {
significandLow = significandLow.add(Long.fromNumber(digits[dIdx]));
}
} else {
let dIdx = firstDigit;
let dIdx = 0;
significandHigh = Long.fromNumber(digits[dIdx++]);

for (; dIdx <= lastDigit - 17; dIdx++) {
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
22 changes: 22 additions & 0 deletions test/node/decimal128_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,4 +1211,26 @@ describe('Decimal128', function () {
expect(() => new Decimal128(Buffer.alloc(16))).to.not.throw();
expect(() => new Decimal128(new Uint8Array(16))).to.not.throw();
});

context('fromString()', function () {
context("when input has leading '+' and has more than 34 significant digits", function () {
it('throws BSON error on inexact rounding', function () {
expect(() =>
Decimal128.fromString('+100000000000000000000000000000000000000000000001')
).to.throw(BSON.BSONError, /inexact rounding/);
});
});

context(
'when input has 1 significant digits, 34 total digits and an exponent greater than exponent_max',
function () {
it('throws BSON error reporting overflow', function () {
expect(() => Decimal128.fromString('1000000000000000000000000000000000e6112')).to.throw(
BSON.BSONError,
/overflow/
);
});
}
);
});
});
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\"}}"
}
]
}