Add decimal <-> float16 cast support #9198
Conversation
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Can we add some tests too?
| ) => true, | ||
|
|
||
| // decimal to null | ||
| (Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _) | Decimal256(_, _), Null) => true, |
There was a problem hiding this comment.
These changes don't seem necessary?
| pub fn is_floating(&self) -> bool { | ||
| use DataType::*; | ||
| matches!(self, Float16 | Float32 | Float64) | ||
| matches!( |
|
Thanks for the review! I've added unit tests for decimal ↔ float16 cast and reverted the unrelated changes. Please take another look. |
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
and reverted the unrelated changes
These don't seem to have taken effect?
| use std::sync::Arc; | ||
|
|
||
| #[test] | ||
| fn test_decimal128_to_float16_cast() { |
There was a problem hiding this comment.
We need more testing:
- Cover all decimal types
- Actually check the output of the cast (right now this only checks casting succeeds and null value
I would suggest looking at the other test cases in this file for how to structure test casts
|
"Thanks for the review! I have updated the tests to cover all decimal types (Decimal32, Decimal64, Decimal128, and Decimal256) casting to Float16 and added assertions to verify that the actual casted values match the expected results, ensuring correctness beyond just successful execution. Ready for re-review!" |
|
@UtkarshSahay123 I would strongly suggest self-reviewing PRs before asking for reviews. I've mentioned twice now that unrelated changes are being made and despite being assured they were reverted, they are still there. And I see a new unrelated comment in the code. |
2 participants
Which issue does this PR close?
Rationale for this change
Apache Arrow currently supports casting between decimal and float32/float64 types, but does not support float16.
This PR adds support for casting between decimal and float16 types to provide feature parity with other floating point types.
What changes are included in this PR?
Are these changes tested?
Yes. Existing cast tests pass locally:
cargo test -p arrow-cast
Are there any user-facing changes?
Yes. Users can now cast between decimal and float16 types using the standard Arrow cast APIs.