GH-35067: [JavaScript] toString for signed `BigNum`s by aljazerzen · Pull Request #35067 · apache/arrow

aljazerzen · 2023-04-12T08:42:40Z

What changes are included in this PR?

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 apache/arrow-js#100.

Closes: GH-35067: [JavaScript] toString for signed BigNums #35067
GitHub Issue: GH-35067: [JavaScript] toString for signed BigNums #35067

github-actions · 2023-04-12T08:43:12Z

Closes: GH-35067: [JavaScript] toString for signed BigNums #35067

github-actions · 2023-04-12T08:43:14Z

⚠️ GitHub issue #35067 has been automatically assigned in GitHub to PR creator.

github-actions · 2023-04-12T08:43:15Z

⚠️ GitHub issue #35067 has no components, please add labels for components.

github-actions · 2023-04-12T08:44:59Z

⚠️ GitHub issue #35067 has no components, please add labels for components.

github-actions · 2023-04-12T08:45:39Z

⚠️ GitHub issue #35067 has no components, please add labels for components.

trxcllnt

Looks good to me, thanks @aljazerzen!

aljazerzen · 2023-04-13T16:24:08Z

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, 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.

trxcllnt · 2023-04-13T19:57:16Z

@aljazerzen 99% sure closure is mangling too aggressively, and we need to do if(!a['signed']) to defeat it.

Co-authored-by: Paul Taylor <178183+trxcllnt@users.noreply.github.com>

aljazerzen · 2023-04-21T14:31:20Z

@domoritz can you review this? @trxcllnt can it be merged with only one approval?

domoritz · 2023-04-21T15:26:41Z

One review is fine. We can merge this.

aljazerzen · 2023-04-22T04:07:08Z

Great. Please go ahead, as I don't have the permissions.

domoritz · 2023-04-22T07:43:14Z

Thank you. I had to get back to my computer to use the merge script.

ursabot · 2023-04-23T13:13:31Z

Benchmark runs are scheduled for baseline = 080a74c and contender = a1403d4. a1403d4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.39% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] a1403d46 ec2-t3-xlarge-us-east-2
[Failed] a1403d46 test-mac-arm
[Finished] a1403d46 ursa-i9-9960x
[Finished] a1403d46 ursa-thinkcentre-m75q
[Finished] 080a74c2 ec2-t3-xlarge-us-east-2
[Failed] 080a74c2 test-mac-arm
[Finished] 080a74c2 ursa-i9-9960x
[Finished] 080a74c2 ursa-thinkcentre-m75q
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

ursabot · 2023-04-23T13:16:05Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

) ### What changes are included in this PR? Closes apache#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. * Closes: apache#35067 Lead-authored-by: Aljaž Mur Eržen <aljaz.erzen@gmail.com> Co-authored-by: Aljaž Mur Eržen <aljazerzen@users.noreply.github.com> Co-authored-by: Paul Taylor <178183+trxcllnt@users.noreply.github.com> Signed-off-by: Dominik Moritz <domoritz@gmail.com>

aljazerzen added 3 commits April 12, 2023 10:26

refactor bn.ts

0f74e53

test bn.ts

dc7656c

toString for signed negative BigNum

17c07f1

aljazerzen requested review from domoritz and trxcllnt as code owners April 12, 2023 08:42

aljazerzen changed the title ~~GH- : [JS] toString for signed BigNums~~ GH-35067 : [JS] toString for signed BigNums Apr 12, 2023

github-actions Bot assigned aljazerzen Apr 12, 2023

github-actions Bot added the awaiting review Awaiting review label Apr 12, 2023

aljazerzen changed the title ~~GH-35067 : [JS] toString for signed BigNums~~ GH-35067: [JS] toString for signed BigNums Apr 12, 2023

aljazerzen changed the title ~~GH-35067: [JS] toString for signed BigNums~~ GH-35067: [JavaScript] toString for signed BigNums Apr 12, 2023

trxcllnt approved these changes Apr 12, 2023

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 12, 2023

aljazerzen added 2 commits April 13, 2023 12:18

fix: prevent mutating number in toString

cc7bd9c

try to fix tests for -t esnext -m umd

e3b27d4

github-actions Bot added the Component: JavaScript label Apr 13, 2023

trxcllnt reviewed Apr 13, 2023

View reviewed changes

Comment thread js/src/util/bn.ts Outdated

tweak

7fe2dd5

aljazerzen force-pushed the js-bignum-signed branch from 9a226cd to 7fe2dd5 Compare April 13, 2023 16:19

trxcllnt reviewed Apr 13, 2023

View reviewed changes

Comment thread js/src/util/bn.ts Outdated

Update js/src/util/bn.ts

440a78d

Co-authored-by: Paul Taylor <178183+trxcllnt@users.noreply.github.com>

domoritz merged commit a1403d4 into apache:main Apr 22, 2023

domoritz added this to the 13.0.0 milestone Apr 22, 2023

bluehat974 mentioned this pull request Jun 9, 2023

cast decimal to double option duckdb/duckdb-wasm#477

Closed

kravets-levko mentioned this pull request Jun 14, 2023

scientific notation negative numbers databricks/databricks-sql-nodejs#150

Closed

jraymakers mentioned this pull request Aug 31, 2023

upgrade to apache-arrow v13 duckdb/duckdb-wasm#1396

Merged

Conversation

aljazerzen commented Apr 12, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Apr 12, 2023

Uh oh!

github-actions Bot commented Apr 12, 2023

Uh oh!

github-actions Bot commented Apr 12, 2023

Uh oh!

github-actions Bot commented Apr 12, 2023

Uh oh!

github-actions Bot commented Apr 12, 2023

Uh oh!

trxcllnt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aljazerzen commented Apr 13, 2023

Uh oh!

trxcllnt commented Apr 13, 2023

Uh oh!

Uh oh!

aljazerzen commented Apr 21, 2023

Uh oh!

domoritz commented Apr 21, 2023

Uh oh!

aljazerzen commented Apr 22, 2023

Uh oh!

domoritz commented Apr 22, 2023

Uh oh!

ursabot commented Apr 23, 2023

Uh oh!

ursabot commented Apr 23, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aljazerzen commented Apr 12, 2023 •

edited by github-actions Bot

Loading