-
Notifications
You must be signed in to change notification settings - Fork 1k
[Variant] Support strict casting for Decimals #8483
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
base: main
Are you sure you want to change the base?
[Variant] Support strict casting for Decimals #8483
Conversation
run_test_with_options(values, expected, false); | ||
} | ||
|
||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved above to group related tests together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one nit
|array| -> arrow::array::Decimal32Array { array.as_primitive() }, | ||
|value| decimal_to_variant_decimal!(value, scale, i32, VariantDecimal4) | ||
|value| -> Option<_> { | ||
decimal_to_variant_decimal!(value, scale, i32, VariantDecimal4).map(Variant::from) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need these Variant::from
calls?
The builder append_value
should take impl Into<Variant>
and we already define the needed impl From<DecimalXX> for Variant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Support strict casting for Decimal types, also fix the non-strict cast for temporal types where the results should be
Variant:Null
(Overflow) insteadNone
(missing value)Are these changes tested?
Yes
Are there any user-facing changes?
Yes, decimal casting now supports strict and non-strict modes