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

[53.0.0_maintenance] Backport: fix: issue introduced in #6833 - less than equal check for scale in … #7079

Open
wants to merge 2 commits into
base: 53.0.0_maintenance
Choose a base branch
from
Open

[53.0.0_maintenance] Backport: fix: issue introduced in #6833 - less than equal check for scale in … #7079

wants to merge 2 commits into from
+23 −10

Conversation

himadripal
Copy link
Contributor

@himadripal himadripal commented Feb 5, 2025 •
edited
Loading

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Feb 5, 2025
@himadripal
Copy link
Contributor Author

@alamb @andygrove @findepi - backport fix.

andygrove
andygrove approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

Thanks @himadripal

@himadripal
Copy link
Contributor Author

DO NOT Merge it yet. I'm trying to figure out why this test_decimal_to_decimal_throw_error_on_precision_overflow_lower_scale failed in 53.0.0 maintenance branch but passes in main branch. Any idea why this would happen would be appreciated.

andygrove
andygrove requested changes Feb 5, 2025
View reviewed changes
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

Removing approval for now to avoid accidental merge

himadripal and others added 2 commits February 5, 2025 09:17
@himadripal
remove println (apache#6935)
2d4e87f
@himadripal @Blizzara
fix: issue introduced in apache#6833 - less than equal check for scal…
efa7e8d 
…e in decimal conversion (apache#7070)

* fix <= check for scale in decimal conversion

* Update arrow-cast/src/cast/mod.rs

name change

Co-authored-by: Arttu <[email protected]>

* remove incorrect comment

---------

Co-authored-by: Arttu <[email protected]>
@himadripal
Copy link
Contributor Author

This is good now. test fix was required to be back ported along with this one.

@findepi
Copy link
Member

findepi commented Feb 5, 2025

this PR has much more changes than #7070 had
is it intentional?

@himadripal
Copy link
Contributor Author

himadripal commented Feb 5, 2025 •
edited
Loading

Yes @findepi . The back porting of test fix #6935 is also required with this, otherwise the test fails, this additional change make it same as main with respect to this functionality. Changes for #6935 are remove println and fix a test.

findepi
findepi approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

thanks!

if you want, you can also include #7078 test case in the backport. Optional

@findepi
Copy link
Member

findepi commented Feb 5, 2025

cc @felipecrv

@alamb
Copy link
Contributor

alamb commented Feb 6, 2025

I don't think we have any releases planned for the 53.0.0 line -- it would be fine to merge this PR but unless someone is going to make a release it won't be available to users

Release schedule is here: https://github.com/apache/arrow-rs?tab=readme-ov-file#release-versioning-and-schedule

I also filed a ticket to track the next planned release

@alamb alamb changed the title Backport: fix: issue introduced in #6833 - less than equal check for scale in … [53.0.0_maintenance] Backport: fix: issue introduced in #6833 - less than equal check for scale in … Feb 6, 2025
@himadripal
Copy link
Contributor Author

@alamb should there exist a 54.0.0 maintenance branch?

@alamb
Copy link
Contributor

alamb commented Feb 6, 2025

@alamb should there exist a 54.0.0 maintenance branch?

Update: we will release 54.2.0 from main, so I don't think there is any need for a maintenance branch at this time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andygrove andygrove andygrove requested changes

@findepi findepi findepi approved these changes

Assignees
No one assigned
Labels
arrow Changes to the arrow crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@himadripal @findepi @alamb @andygrove