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.

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

fix: bad cast to Decimal128 for negative precision #6413

Closed
wants to merge 1 commit into from

Conversation

ByteBaker
Copy link
Contributor

Which issue does this PR close?

Closes #5793.

Rationale for this change

Refer to issue.

What changes are included in this PR?

  • Logic change
  • Test case fix

Are there any user-facing changes?

No.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Sep 18, 2024
@alamb
Copy link
Contributor

alamb commented Sep 18, 2024

I am depressed about the large review backlog in this crate. We are looking for more help from the community reviewing PRs -- see #6418 for more

Comment on lines -8124 to +8114
assert_eq!("1123450", decimal_arr.value_as_string(0));
assert_eq!("2123450", decimal_arr.value_as_string(1));
assert_eq!("3123450", decimal_arr.value_as_string(2));
assert_eq!("11234560", decimal_arr.value_as_string(0));
assert_eq!("21234560", decimal_arr.value_as_string(1));
assert_eq!("31234560", decimal_arr.value_as_string(2));
Copy link
Member

@wjones127 wjones127 Sep 19, 2024

Choose a reason for hiding this comment

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

I'm not sure this behavior change is actually what we want 🤔

See my comment in this issue: #5793 (comment)

If we do commit this, we should mark this as breaking-change.

@wjones127 wjones127 added the api-change Changes to the arrow API label Sep 19, 2024
@tustvold tustvold added the next-major-release the PR has API changes and it waiting on the next major version label Sep 19, 2024
@ByteBaker
Copy link
Contributor Author

@alamb @jayzhan211 I'm closing this as this is inconsistent with Postgres' behavior on fixed-point decimal casting. In fact, what actually needs to change is the documentation to reflect the proper behavior. Its parent issue is already closed.

I'll make the change in the docs and open a separate PR for that.

@ByteBaker ByteBaker closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate next-major-release the PR has API changes and it waiting on the next major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cast int to decimal has an unexpected result
4 participants