[Variant] impl [Try]From for VariantDecimalXX types #7809
Conversation
|
Attn @alamb and @friendlymatthew |
parquet-variant/src/variant.rs
Outdated
| Variant::Int64(i) => Some(i), | ||
| Variant::Decimal4(d) if d.scale == 0 => Some(d.integer.into()), | ||
| Variant::Decimal8(d) if d.scale == 0 => Some(d.integer), | ||
| Variant::Decimal16(d) if d.scale == 0 => d.integer.try_into().ok(), |
There was a problem hiding this comment.
One minor comment is that the ok() here discards an ArowError which has an owned string -- so in othr words, this call is pretty expensive. It might be worth a method that returns a Option rather than Err for testing conversion
The same general rule applies below
One potential solution woudl be to add methods like
struct VariantDecimal8 {
// returns Some if this variant can be represented as Decimal4, None otherwise
fn as_decimal_4(&self) -> Option<VariantDecimal4> { ... }There was a problem hiding this comment.
This is always tricky -- if we go for Option instead, and convert None to Err, then the resulting error message contains little or no information about why the conversion failed. And I don't think we want to duplicate code, to do both?
There was a problem hiding this comment.
Yeah, let's go with this for now and we can optimize if it shows up in traces
parquet-variant/src/variant.rs
Outdated
| Variant::Int64(i) => Some(i), | ||
| Variant::Decimal4(d) if d.scale == 0 => Some(d.integer.into()), | ||
| Variant::Decimal8(d) if d.scale == 0 => Some(d.integer), | ||
| Variant::Decimal16(d) if d.scale == 0 => d.integer.try_into().ok(), |
There was a problem hiding this comment.
Yeah, let's go with this for now and we can optimize if it shows up in traces
|
I merged this PR up to resolve a conflict and hope to merge it shortly |
|
@scovich I noticed this PR is marked as Draft. Is it ok to mark ready for review and merge? |
Go for it! |
|
Thanks again @scovich |
Which issue does this PR close?
Rationale for this change
The existing
Variant::as_decimal_XXmethods were actually incorrect, failing to validate scale when converting from a wider decimal to a narrower one. Fix it, while also improving ergonomics of the decimal code to reduce the chances of future issues of this type.What changes are included in this PR?
Add proper [Try]From for converting to decimal types from other decimals or their underlying integer type.
Add missing conversions in the
Variant::as_int_xxandVariant::as_decimal_xxhelpers.Are these changes tested?
TODO - need more tests for the new conversions
Are there any user-facing changes?
The
Variant:as_decimal_xxmethods have been renamed and now return actual decimal types.New conversions available.