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

[Regression in 54.0.0]. Decimal cast to smaller precision gives invalid (off-by-one) result in some cases #7069

Closed
Blizzara opened this issue Feb 3, 2025 · 6 comments · Fixed by #7070
Labels
arrow Changes to the arrow crate bug

Comments

@Blizzara
Copy link
Contributor

Blizzara commented Feb 3, 2025

Describe the bug

See practical examples in apache/datafusion#14450.
Somehow a regression caused by #6833.

To Reproduce

Adding this into arrow-cast/src/cast/mod.rs:

    #[test]
    fn test_decimal_to_decimal() {
        let array = vec![Some(520)];
        let array = create_decimal_array(array, 4, 2).unwrap();
        let input_type = DataType::Decimal128(4, 2);
        let output_type = DataType::Decimal128(3, 2);
        assert!(can_cast_types(&input_type, &output_type));

        let options = CastOptions {
            safe: false,
            ..Default::default()
        };
        let result = cast_with_options(&array, &output_type, &options);
        assert_eq!(result.unwrap().as_primitive::<Decimal128Type>().value(0), 520);
    }

passes on 84dba34, but fails on eb7ab83 with

assertion `left == right` failed
  left: 521
 right: 520

Somehow something in the code adds an extra 1 to the value?

Expected behavior

Test passes

Additional context

Fyi @himadripal @andygrove @findepi @viirya @tustvold as you were involved in that earlier change. I tried to look at it but don't understand how it'd go wrong myself, maybe you know?

@himadripal
Copy link
Contributor

himadripal commented Feb 3, 2025

@Blizzara I'll take a look, as part of the previous PR, I added validation after creation like it was for float, wondering if validation message would add that extra 1?

@Blizzara
Copy link
Contributor Author

Blizzara commented Feb 3, 2025

Actually @himadripal I think the issue is this line: eb7ab83#diff-07a573af92ff5330f77407c559e89e06b02ef8c466a394ca86a710023f000593R170, the < should be probably <= to match the convert_to_bigger_or_equal_scale_decimal. If I do that change then my test passes. Does that make sense to you?

@himadripal
Copy link
Contributor

himadripal commented Feb 3, 2025

@Blizzara it make sense, when input scale = output scale it always goes to convert_to_smaller_scale_decimal instead it should go to convert_to_bigger_or_equal_scale_decimal , I think, tests added as part of the #6833 needs change . Let me know if you are going to make this change, I'll create a PR otherwise to fix this now.

@himadripal
Copy link
Contributor

@Blizzara Please check.

@alamb alamb changed the title Decimal cast to smaller precision gives invalid (off-by-one) result in some cases [Regression in 54.0.0]. Decimal cast to smaller precision gives invalid (off-by-one) result in some cases Feb 10, 2025
@alamb
Copy link
Contributor

alamb commented Feb 10, 2025

I double checked and it seems like the issue was introduced by this commit (which was released as part of 54.0.0):

@Blizzara has requested a release with this fix

Thus I have updated the schedule and will prepare to make a release later this weel:

@alamb alamb added the arrow Changes to the arrow crate label Feb 10, 2025
@alamb
Copy link
Contributor

alamb commented Feb 10, 2025

label_issue.py automatically added labels {'arrow'} from #7070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate bug
Projects
None yet
3 participants