-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
ARROW-17176: [Rust] Activate generate_decimal256_case integration test for rust #13685
Conversation
|
cc @alamb |
🎉 🦜 |
@alamb Can you help merge this? If you are busy, please let me know how to merge this. Thanks. 😄 |
Hi @viirya -- I think we can just merge. Thanks! |
I will watch https://github.com/apache/arrow/runs/7585938940?check_suite_focus=true and https://github.com/apache/arrow/runs/7585938943?check_suite_focus=true to make sure they pass -- and if not I will revert this PR |
Thank you @alamb ! |
Ah, there was a failure:
Because we add value validation recently. Like Decimal128 case, we also need to disable validation for Decimal256 case for this integration. Proposed a fix at apache/arrow-rs#2232 cc @alamb |
Benchmark runs are scheduled for baseline = 778d574 and contender = f4c0cda. f4c0cda is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Thanks @viirya -- as I mentioned in apache/arrow-rs#2232 (review) something seems strange if the integration test is using invalid values for Decimal -- either the integration test should be fixed or maybe we have mis-interpreted what is valid in the Rust implementation 🤔 |
FYI @zeroshade |
@alamb Ah, sorry, after rechecking it, I made a mistake when filling maximum and minimum decimal values for precision > 38. So for |
Going to fix it at apache/arrow-rs#2245 |
Thank you @viirya ! |
arrow-rs has added decimal256 support recently. We should activate generate_decimal256_case integration test.