Add Decimal128 and Decimal256 to downcast_primitive #3056
Conversation
viirya
changed the title
|
Interesting, there are a few tests failed due to this change. I will take a deep look. |
arrow/src/row/mod.rs
Outdated
| @@ -726,6 +697,21 @@ unsafe fn decode_column( | |||
| ))) | |||
| } | |||
| }; | |||
| let array: ArrayRef = match &field.data_type { | |||
There was a problem hiding this comment.
I think we should probably be passing the data type into decode_primitive so that it can correctly construct the array with the correct data type. I suspect the current logic will lose the timezone for timestamps 🤔
There was a problem hiding this comment.
As the change doesn't touch timestamp array, I think that you didn't mean the issue is caused by this change, but suggested that we should also fix the current logic together, right?
There was a problem hiding this comment.
Yeah, they both have the same problem
arrow/src/row/fixed.rs
Outdated
| @@ -326,9 +326,20 @@ fn decode_fixed<T: FixedLengthEncoding + ToByteSlice>( | |||
| pub fn decode_primitive<T: ArrowPrimitiveType>( | |||
| rows: &mut [&[u8]], | |||
| options: SortOptions, | |||
| data_type: &DataType, | |||
There was a problem hiding this comment.
This is technically unsafe as it could allow for transmute from decimal128 to decimal256, etc... I have a PR in the works to fix this
viirya
force-pushed
the
add_decimal_to_downcast_primitive
branch
from
74f2552
to
1af7263
Compare
|
I've added an API change, as this may lead to downstream breakage (likely a good thing as it will remove unnecessary special-case handling for decimals) |
|
I will rebase once #3064 is merged. |
|
Benchmark runs are scheduled for baseline = e4e15f8 and contender = f596209. f596209 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3055.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?