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

Error order for comparing Decimal128 or Decimal256 #2256

Closed
liukun4515 opened this issue Aug 1, 2022 · 2 comments · Fixed by #2275
Closed

Error order for comparing Decimal128 or Decimal256 #2256

liukun4515 opened this issue Aug 1, 2022 · 2 comments · Fixed by #2275
Assignees
Labels
arrow Changes to the arrow crate bug

Comments

@liukun4515
Copy link
Contributor

Describe the bug
Decimal128 as a example, we use the [u8;16] to store the value of the decimal.

In order to compare the decimal128, we should use the signed compare order.

But the current implementation just compare the [u8;16] to get the order.

For example:
Decimal128(-1, 10,0).cmp(Decimal128(129,10,0) will get the result of Greater.

To Reproduce

    #[test]
    fn compare_i128() {
        let v: i128 = -1;
        let b1: [u8; 16] = v.to_le_bytes();
        let b2: [u8; 16] = 129_i128.to_le_bytes();
        println!("{:?}", b1);
        println!("{:?}", b2);
        if b1.eq(&b2) {
            println!("=")
        }
        println!("{:?}", b1.cmp(&b2));
    }

Expected behavior

Additional context

@liukun4515 liukun4515 added the bug label Aug 1, 2022
@liukun4515
Copy link
Contributor Author

liukun4515 commented Aug 1, 2022

FYI @viirya
I will fix it.

@viirya
Copy link
Member

viirya commented Aug 1, 2022

Good catch!

@alamb alamb added the arrow Changes to the arrow crate label Aug 5, 2022
@alamb alamb changed the title Error order for comparing the decimal128 or decimal256 Error order for comparing Decimal128 or Decimal256 Aug 5, 2022
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
Development

Successfully merging a pull request may close this issue.

3 participants