This is an automated email from the ASF dual-hosted git repository.
ptaylor pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new f8f17a58ad GH-45250: [JS] Fix denominator precision loss and remove
unnecessary safe integer check for fractional part (#45251)
f8f17a58ad is described below
commit f8f17a58adb6b5fe4fed6c13d7d3b2e64b4a9acd
Author: Kent Yao <y...@apache.org>
AuthorDate: Wed Jan 22 11:07:50 2025 +0800
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 <178183+trxcl...@users.noreply.github.com>
---
js/src/util/bn.ts | 11 +++++++----
js/test/unit/bn-tests.ts | 13 ++++++++++++-
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/js/src/util/bn.ts b/js/src/util/bn.ts
index 8f6dfe258f..8eed390924 100644
--- a/js/src/util/bn.ts
+++ b/js/src/util/bn.ts
@@ -91,11 +91,14 @@ export function bigNumToNumber<T extends
BN<BigNumArray>>(bn: T, scale?: number)
number |= word * (BigInt(1) << BigInt(64 * i++));
}
}
- if (typeof scale === 'number') {
- const denominator = BigInt(Math.pow(10, scale));
+ if (typeof scale === 'number' && scale > 0) {
+ const denominator = BigInt('1'.padEnd(scale + 1, '0'));
const quotient = number / denominator;
- const remainder = number % denominator;
- return bigIntToNumber(quotient) + (bigIntToNumber(remainder) /
bigIntToNumber(denominator));
+ const remainder = negative? -(number % denominator) : number %
denominator;
+ const integerPart = bigIntToNumber(quotient);
+ const fractionPart = `${remainder}`.padStart(scale, '0');
+ const sign = negative && integerPart === 0 ? '-' : '';
+ return +`${sign}${integerPart}.${fractionPart}`;
}
return bigIntToNumber(number);
}
diff --git a/js/test/unit/bn-tests.ts b/js/test/unit/bn-tests.ts
index 2ea8f6055d..510b8c2cd6 100644
--- a/js/test/unit/bn-tests.ts
+++ b/js/test/unit/bn-tests.ts
@@ -16,7 +16,7 @@
// under the License.
import * as Arrow from 'apache-arrow';
-const { BN } = Arrow.util;
+const { BN, bigNumToNumber } = Arrow.util;
describe(`BN`, () => {
test(`to detect signed numbers, unsigned numbers and decimals`, () => {
@@ -98,4 +98,15 @@ describe(`BN`, () => {
// const n6 = new BN(new Uint32Array([0x00000000, 0x00000000,
0x00000000, 0x80000000]), false);
// expect(n6.valueOf(1)).toBe(1.7014118346046923e+37);
});
+
+ test(`bigNumToNumber`, () => {
+ const n1 = new BN(new Uint32Array([3, 2, 1, 0]));
+ expect(() => bigNumToNumber(n1)).toThrow('18446744082299486211');
+ /* eslint-disable @typescript-eslint/no-loss-of-precision */
+ expect(bigNumToNumber(n1, 10)).toBeCloseTo(1844674408.2299486);
+ expect(bigNumToNumber(n1, 15)).toBeCloseTo(18446.744082299486);
+ expect(bigNumToNumber(n1, 20)).toBeCloseTo(0.18446744082299486);
+ expect(bigNumToNumber(n1, 25)).toBeCloseTo(0.0000018446744082299486);
+ /* eslint-enable @typescript-eslint/no-loss-of-precision */
+ });
});
