You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "aljazerzen (via GitHub)" <gi...@apache.org> on 2023/04/12 08:42:43 UTC
[GitHub] [arrow] aljazerzen opened a new pull request, #35067: GH- : [JS] toString for signed `BigNum`s
aljazerzen opened a new pull request, #35067:
URL: https://github.com/apache/arrow/pull/35067
### What changes are included in this PR?
Closes #22932
Basically, signed negative numbers were displayed as very large positive numbers.
### Are these changes tested?
Yes, I've also added tests for `bn.ts` (BigNum) that was not tested before.
### Are there any user-facing changes?
Negative numbers stored in BigNum will now be displayed as negative numbers instead of very large positive numbers.
Note that this change also affects decimals, because they are stored in BigNum as signed numbers. Decimals still don't convert to string correctly, because inserting the decimal dot is not implemented. Ref #28804.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1519065823
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/8c2fab6457064b1ba4aa85178426317a...470beeecca5d465f96a9e9059306b5d6/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1518549673
Thank you. I had to get back to my computer to use the merge script.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a diff in pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #35067:
URL: https://github.com/apache/arrow/pull/35067#discussion_r1165737563
##########
js/src/util/bn.ts:
##########
@@ -90,12 +90,52 @@ function bignumToNumber<T extends BN<BigNumArray>>(bn: T) {
}
/** @ignore */
-export const bignumToString: { <T extends BN<BigNumArray>>(a: T): string } = (<T extends BN<BigNumArray>>(a: T) => a.byteLength === 8 ? `${new a['BigIntArray'](a.buffer, a.byteOffset, 1)[0]}` : decimalToString(a));
+export const bigNumToString: { <T extends BN<BigNumArray>>(a: T): string } = (<T extends BN<BigNumArray>>(a: T) => {
+ // use BigInt native implementation
+ if (a.byteLength === 8) {
+ const bigIntArray = new a['BigIntArray'](a.buffer, a.byteOffset, 1);
+ return `${bigIntArray[0]}`;
+ }
+
+ // unsigned numbers
+ if (!a.signed) {
+ return unsignedBigNumToString(a);
+ }
+
+ let array = new Uint16Array(a.buffer, a.byteOffset, a.byteLength / 2);
+
+ // detect positive numbers
+ const highOrderWord = new Int16Array([array[array.length - 1]])[0];
+ if (highOrderWord >= 0) {
+ return unsignedBigNumToString(a);
+ }
+
+ // flip the negative value
+ array = array.slice();
+ let carry = 1;
+ for (let i = 0; i < array.length; i++) {
+ const elem = array[i];
+ const updated = ~elem + carry;
+ array.set([updated], i);
Review Comment:
```suggestion
array[i] = updated;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] aljazerzen commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "aljazerzen (via GitHub)" <gi...@apache.org>.
aljazerzen commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1518500915
Great. Please go ahead, as I don't have the permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] aljazerzen commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "aljazerzen (via GitHub)" <gi...@apache.org>.
aljazerzen commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1507250762
I've spent a little time trying to get tests to pass, but something strange is happening:
the `bn-tests.ts` are failing only on `-t es5 -m umd`. I've debugged it to the finding that in [this line](https://github.com/apache/arrow/pull/35067/files#diff-f0dbeeadc965880cf706bd8b8776f24dacf74f260b1faa5c60e4d62dbb76ecd7R101), `a.signed` is false, even though I've constructed a signed number or a decimal (which is signed).
It may be a problem with how these prototypes are assigning to each other, but that is beyond me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35067: GH-35067 : [JS] toString for signed `BigNum`s
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1504891746
:warning: GitHub issue #35067 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] aljazerzen commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "aljazerzen (via GitHub)" <gi...@apache.org>.
aljazerzen commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1517924572
@domoritz can you review this? @trxcllnt can it be merged with only one approval?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1519065276
Benchmark runs are scheduled for baseline = 080a74c2c9c5b5b1f8b6252be4aff8697a3c7172 and contender = a1403d46916ac6daa419cc688c5fb19980a59e60. a1403d46916ac6daa419cc688c5fb19980a59e60 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/a2ba7eba7d114cac911d609d4f932b00...4b22d4cc09b94339b55bee670ca642ab/)
[Failed] [test-mac-arm](https://conbench.ursa.dev/compare/runs/306376a7be6747e9a9d2b2e5e8772391...4d5a6d41005f45a496b996c23555f782/)
[Finished :arrow_down:0.77% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/8c2fab6457064b1ba4aa85178426317a...470beeecca5d465f96a9e9059306b5d6/)
[Finished :arrow_down:0.39% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/e473b0eceb3148ea947c6b4b288c546f...c5c0ecb509604fe390a272665ae59d01/)
Buildkite builds:
[Finished] [`a1403d46` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2769)
[Failed] [`a1403d46` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2803)
[Finished] [`a1403d46` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2767)
[Finished] [`a1403d46` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2794)
[Finished] [`080a74c2` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2768)
[Failed] [`080a74c2` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2802)
[Finished] [`080a74c2` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2766)
[Finished] [`080a74c2` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2793)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1507535376
@aljazerzen 99% sure closure is mangling too aggressively, and we need to do `if(!a['signed'])` to defeat it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1517999213
One review is fine. We can merge this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz merged pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz merged PR #35067:
URL: https://github.com/apache/arrow/pull/35067
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1504894915
:warning: GitHub issue #35067 **has no components**, please add labels for components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35067: GH-35067 : [JS] toString for signed `BigNum`s
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1504891763
:warning: GitHub issue #35067 **has no components**, please add labels for components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35067: GH-35067: [JS] toString for signed `BigNum`s
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1504894038
:warning: GitHub issue #35067 **has no components**, please add labels for components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #35067: GH-35067 : [JS] toString for signed `BigNum`s
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35067:
URL: https://github.com/apache/arrow/pull/35067#issuecomment-1504891700
* Closes: #35067
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a diff in pull request #35067: GH-35067: [JavaScript] toString for signed `BigNum`s
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #35067:
URL: https://github.com/apache/arrow/pull/35067#discussion_r1165970302
##########
js/src/util/bn.ts:
##########
@@ -90,12 +90,52 @@ function bignumToNumber<T extends BN<BigNumArray>>(bn: T) {
}
/** @ignore */
-export const bignumToString: { <T extends BN<BigNumArray>>(a: T): string } = (<T extends BN<BigNumArray>>(a: T) => a.byteLength === 8 ? `${new a['BigIntArray'](a.buffer, a.byteOffset, 1)[0]}` : decimalToString(a));
+export const bigNumToString: { <T extends BN<BigNumArray>>(a: T): string } = (<T extends BN<BigNumArray>>(a: T) => {
+ // use BigInt native implementation
+ if (a.byteLength === 8) {
+ const bigIntArray = new a['BigIntArray'](a.buffer, a.byteOffset, 1);
+ return `${bigIntArray[0]}`;
+ }
+
+ // unsigned numbers
+ if (!a.signed) {
Review Comment:
```suggestion
if (!a['signed']) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org