Add Decimal256Builder and Decimal256Array (generic approach) #2006
Conversation
viirya
force-pushed
the
decimal256array_generic
branch
from
96237fd to
1484a9e
Compare
| pub struct GenericDecimalArray<T: BasicDecimal, const VALUE_LENGTH: i32> { | ||
| data: ArrayData, | ||
| value_data: RawPtrBox<u8>, | ||
| precision: usize, | ||
| scale: usize, | ||
| phantom: PhantomData<T>, | ||
| } |
There was a problem hiding this comment.
Compared to #2000, which defines a trait and let DecimalArray and Decimal256Array extend the trait, this generic approach defines a struct GenericDecimalArray and generalize on Decimal type (Decimal128/Decimal256).
Honestly I prefer #2000's trait approach. It looks more clear to me. But one cons of #2000 is that it needs to use macro to reduce code duplication, though it is not complicated usage.
viirya
force-pushed
the
decimal256array_generic
branch
from
1484a9e to
53525f8
Compare
| .parse::<i128>() | ||
| .map_or_else(|_| false, |v| v == self.value(i).as_i128())) | ||
| } | ||
| JString(s) => self.is_valid(i) && (s == &self.value(i).to_string()), |
There was a problem hiding this comment.
This is one issue on generic approach. We cannot keep original JsonEqual behavior for DecimalArray. This causes some test failures.
| use crate::util::decimal::{BasicDecimal, Decimal128, Decimal256}; | ||
| use std::marker::PhantomData; | ||
|
|
||
| pub struct GenericDecimalArray<T: BasicDecimal, const VALUE_LENGTH: i32> { |
There was a problem hiding this comment.
One little concern is that we could remove T: BasicDecimal if we also do const generic on Decimal128 and Decimal256.
There was a problem hiding this comment.
Don't we need a type bound to Decimal<BYTE_LENGTH> even for const generic?
There was a problem hiding this comment.
Actually we need it. But just as I said in #2001, this is an unstable, which is the major disadvantage of using const generic.
There was a problem hiding this comment.
I tried to find some help from the rust-lang team: https://users.rust-lang.org/t/how-to-seal-the-const-generic/77947.
|
As this has some issues, I'd close this and prefer #2000 now. |
|
👍 Thank you for the trying @viirya . |
Which issue does this PR close?
Closes #1999.
This is alternative PR that implements Decimal256Builder and Decimal256Array in generic approach.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?