Skip to content

Commit f8f17a5

Browse files
yaooqinntrxcllnt
andauthored
GH-45250: [JS] Fix denominator precision loss and remove unnecessary safe integer check for fractional part (#45251)
<!-- Thanks for opening a pull request! If this is your first pull request you can find detailed information on how to contribute here: * [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request) * [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html) If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project. Then could you also rename the pull request title in the following format? GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY} or MINOR: [${COMPONENT}] ${SUMMARY} --> ### Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ### What changes are included in this PR? This PR uses string constructor for bigint to calculate the power of 10 with scale as the exponent value of the expression. To Fix precision loss like ```typecript > BigInt(10) ** BigInt(25); 10000000000000000000000000n > BigInt(Math.pow(10, 25)) 10000000000000000905969664n ``` Also, we remove the unnecessary safe integer check for the fraction part. ### Are these changes tested? add some unit tests ### Are there any user-facing changes? no <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking. --> <!-- **This PR includes breaking changes to public APIs.** --> <!-- Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious. --> <!-- **This PR contains a "Critical Fix".** --> * GitHub Issue: #45250 --------- Co-authored-by: Paul Taylor <[email protected]>
1 parent 984519d commit f8f17a5

File tree

2 files changed

+19
-5
lines changed

2 files changed

+19
-5
lines changed

js/src/util/bn.ts

+7-4
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,14 @@ export function bigNumToNumber<T extends BN<BigNumArray>>(bn: T, scale?: number)
9191
number |= word * (BigInt(1) << BigInt(64 * i++));
9292
}
9393
}
94-
if (typeof scale === 'number') {
95-
const denominator = BigInt(Math.pow(10, scale));
94+
if (typeof scale === 'number' && scale > 0) {
95+
const denominator = BigInt('1'.padEnd(scale + 1, '0'));
9696
const quotient = number / denominator;
97-
const remainder = number % denominator;
98-
return bigIntToNumber(quotient) + (bigIntToNumber(remainder) / bigIntToNumber(denominator));
97+
const remainder = negative? -(number % denominator) : number % denominator;
98+
const integerPart = bigIntToNumber(quotient);
99+
const fractionPart = `${remainder}`.padStart(scale, '0');
100+
const sign = negative && integerPart === 0 ? '-' : '';
101+
return +`${sign}${integerPart}.${fractionPart}`;
99102
}
100103
return bigIntToNumber(number);
101104
}

js/test/unit/bn-tests.ts

+12-1
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
// under the License.
1717

1818
import * as Arrow from 'apache-arrow';
19-
const { BN } = Arrow.util;
19+
const { BN, bigNumToNumber } = Arrow.util;
2020

2121
describe(`BN`, () => {
2222
test(`to detect signed numbers, unsigned numbers and decimals`, () => {
@@ -98,4 +98,15 @@ describe(`BN`, () => {
9898
// const n6 = new BN(new Uint32Array([0x00000000, 0x00000000, 0x00000000, 0x80000000]), false);
9999
// expect(n6.valueOf(1)).toBe(1.7014118346046923e+37);
100100
});
101+
102+
test(`bigNumToNumber`, () => {
103+
const n1 = new BN(new Uint32Array([3, 2, 1, 0]));
104+
expect(() => bigNumToNumber(n1)).toThrow('18446744082299486211');
105+
/* eslint-disable @typescript-eslint/no-loss-of-precision */
106+
expect(bigNumToNumber(n1, 10)).toBeCloseTo(1844674408.2299486);
107+
expect(bigNumToNumber(n1, 15)).toBeCloseTo(18446.744082299486);
108+
expect(bigNumToNumber(n1, 20)).toBeCloseTo(0.18446744082299486);
109+
expect(bigNumToNumber(n1, 25)).toBeCloseTo(0.0000018446744082299486);
110+
/* eslint-enable @typescript-eslint/no-loss-of-precision */
111+
});
101112
});

0 commit comments

Comments
 (0)