Skip to content

GH-35310: [Go] Incorrect value decimal128 from string#35311

Merged
zeroshade merged 2 commits into
apache:mainfrom
zeroshade:decimals-rounding
Apr 25, 2023
Merged

GH-35310: [Go] Incorrect value decimal128 from string#35311
zeroshade merged 2 commits into
apache:mainfrom
zeroshade:decimals-rounding

Conversation

@zeroshade
Copy link
Copy Markdown
Member

@zeroshade zeroshade commented Apr 24, 2023

Rationale for this change

Fix edge case of rounding when parsing a decimal128 value from string with a high precision.

What changes are included in this PR?

Adding a simple 0.5 rounding based on the signbit before we truncate the value to an integer.

Are these changes tested?

Yes, an edge case test case was added.

Are there any user-facing changes?

Some values that were previously incorrect will now be correct.

@zeroshade zeroshade requested a review from lidavidm April 24, 2023 20:55
@github-actions
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Can we also add a test with a negative value?

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 24, 2023
@zeroshade
Copy link
Copy Markdown
Member Author

@lidavidm added same test but with negative version of the number

@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 24, 2023
@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 25, 2023
@zeroshade zeroshade merged commit 966a804 into apache:main Apr 25, 2023
@zeroshade zeroshade deleted the decimals-rounding branch April 25, 2023 00:36
@ursabot
Copy link
Copy Markdown

ursabot commented Apr 25, 2023

Benchmark runs are scheduled for baseline = 5de5692 and contender = 966a804. 966a804 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.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.6% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 966a8040 ec2-t3-xlarge-us-east-2
[Failed] 966a8040 test-mac-arm
[Finished] 966a8040 ursa-i9-9960x
[Finished] 966a8040 ursa-thinkcentre-m75q
[Finished] 5de56928 ec2-t3-xlarge-us-east-2
[Failed] 5de56928 test-mac-arm
[Finished] 5de56928 ursa-i9-9960x
[Finished] 5de56928 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

liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
…5311)

### Rationale for this change
Fix edge case of rounding when parsing a decimal128 value from string with a high precision.

### What changes are included in this PR?
Adding a simple 0.5 rounding based on the signbit before we truncate the value to an integer.

### Are these changes tested?
Yes, an edge case test case was added.

### Are there any user-facing changes?
Some values that were previously incorrect will now be correct.

* Closes: apache#35310

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
…5311)

### Rationale for this change
Fix edge case of rounding when parsing a decimal128 value from string with a high precision.

### What changes are included in this PR?
Adding a simple 0.5 rounding based on the signbit before we truncate the value to an integer.

### Are these changes tested?
Yes, an edge case test case was added.

### Are there any user-facing changes?
Some values that were previously incorrect will now be correct.

* Closes: apache#35310

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Go] Incorrect value when creating decimal128 from string

3 participants