Skip to content
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

Jump to bottom

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

Merged
merged 7 commits into from
Apr 22, 2023
Merged

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

merged 7 commits into from
Apr 22, 2023

Conversation

aljazerzen
Copy link
Contributor

@aljazerzen aljazerzen commented Apr 12, 2023 •
edited by github-actions bot
Loading

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.

@aljazerzen aljazerzen changed the title GH- : [JS] toString for signed BigNums GH-35067 : [JS] toString for signed BigNums Apr 12, 2023
@github-actions
Copy link

@github-actions
Copy link

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

@github-actions
Copy link

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

@github-actions github-actions bot added the awaiting review Awaiting review label Apr 12, 2023
@aljazerzen aljazerzen changed the title GH-35067 : [JS] toString for signed BigNums GH-35067: [JS] toString for signed BigNums Apr 12, 2023
@github-actions
Copy link

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

@aljazerzen aljazerzen changed the title GH-35067: [JS] toString for signed BigNums GH-35067: [JavaScript] toString for signed BigNums Apr 12, 2023
@github-actions
Copy link

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

trxcllnt
trxcllnt approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@trxcllnt trxcllnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @aljazerzen!

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 12, 2023
trxcllnt
trxcllnt reviewed Apr 13, 2023
View reviewed changes
js/src/util/bn.ts Outdated Show resolved Hide resolved
@aljazerzen
Copy link
Contributor Author

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
Copy link
Contributor

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

trxcllnt
trxcllnt reviewed Apr 13, 2023
View reviewed changes
js/src/util/bn.ts Outdated Show resolved Hide resolved
@aljazerzen @trxcllnt
Update js/src/util/bn.ts
440a78d 
Co-authored-by: Paul Taylor <178183+trxcllnt@users.noreply.github.com>
@aljazerzen
Copy link
Contributor Author

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

@domoritz
Copy link
Member

One review is fine. We can merge this.

@aljazerzen
Copy link
Contributor Author

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

@domoritz domoritz merged commit a1403d4 into apache:main Apr 22, 2023
@domoritz domoritz added this to the 13.0.0 milestone Apr 22, 2023
@domoritz
Copy link
Member

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

@ursabot
Copy link

ursabot commented Apr 23, 2023

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
Copy link

ursabot commented Apr 23, 2023

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

liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
@aljazerzen @trxcllnt @liujiacheng777
apacheGH-35067: [JavaScript] toString for signed BigNums (apache#35067
f9dd209 
)

### 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 apache#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>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@aljazerzen @trxcllnt @ArgusLi
apacheGH-35067: [JavaScript] toString for signed BigNums (apache#35067
ccd4d03 
)

### 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 apache#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>
rtpsw pushed a commit to rtpsw/arrow that referenced this pull request May 16, 2023
@aljazerzen @trxcllnt @rtpsw
apacheGH-35067: [JavaScript] toString for signed BigNums (apache#35067
b7647c8 
)

### 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 apache#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>
@bluehat974 bluehat974 mentioned this pull request Jun 9, 2023
Closed
@kravets-levko kravets-levko mentioned this pull request Jun 14, 2023
Closed
@jraymakers jraymakers mentioned this pull request Aug 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trxcllnt trxcllnt trxcllnt approved these changes

@domoritz domoritz Awaiting requested review from domoritz domoritz is a code owner

Assignees

@aljazerzen aljazerzen

Labels
awaiting committer review Awaiting committer review Component: JavaScript
Projects
None yet
Milestone
13.0.0
Development

Successfully merging this pull request may close these issues.

[JS] decimal toString does not support negative values
4 participants
@aljazerzen @trxcllnt @domoritz @ursabot