[Variant] add strict mode to cast_to_variant #8233
Conversation
Signed-off-by: codephage2020 <[email protected]>
Signed-off-by: codephage2020 <[email protected]>
Signed-off-by: codephage2020 <[email protected]>
Signed-off-by: codephage2020 <[email protected]>
|
Thanks for your reviews @liamzwbao . All feedback implemented - ready for re-review! 🔍 CC @alamb . |
codephage2020
changed the title
| /// * `strict` - If true, return error on conversion failure. If false, insert null for failed conversions. | ||
| pub fn cast_to_variant_with_options( | ||
| input: &dyn Array, | ||
| strict: bool, |
There was a problem hiding this comment.
Is there any possibility/risk that we may need additional options in the future? If so, it might be better to pass a struct? Or maybe we just deal with that if/when it comes?
There was a problem hiding this comment.
Yes, very good discovery. I have thought about this implementation. We can refactor this part in the next PR.
Done.
| run_test_non_strict( | ||
| values, | ||
| vec![ | ||
| None, // Invalid timestamp becomes null |
There was a problem hiding this comment.
Just realized that for overflow value, we may need to convert it to Variant::Null instead of None like what we did for Decimal, see this comment for context. However, I don't think it should be in this PR, we could discuss and make them consistent later.
cc @alamb
Signed-off-by: codephage2020 <[email protected]> Co-authored-by: Ryan Johnson <[email protected]>
Signed-off-by: codephage2020 <[email protected]>
Signed-off-by: codephage2020 <[email protected]>
| fn convert_timestamp( | ||
| /// Options for controlling the behavior of `cast_to_variant_with_options`. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct CastOptions { |
There was a problem hiding this comment.
This looks quite similar to https://docs.rs/arrow/latest/arrow/compute/struct.CastOptions.html
However that seems to be defined in arrow-compute
There was a problem hiding this comment.
I thought more about this and I think a dedicated CastOptions struct makes sense for variant
There was a problem hiding this comment.
Specifically they are different kernels and so some options like FormatOptions for the normal cast kernel may not be relevant for this one
There was a problem hiding this comment.
Thanks @codephage2020 -- this looks good to me. Sorry for the delay in reviewing this PR.
It looks like this PR has a few merge conflicts, but once those are resolved this will be ready to go
|
Thank you @scovich and @liamzwbao for the reviews |
Thanks @alamb , these conflicts have been resolved. |
|
Thanks again everyone |
Which issue does this PR close?
Date64or Timestamp Values values #8155 .Rationale for this change
cast_to_variant will panic for values of Date64 / Timestamp that can not be converted to NaiveDate
What changes are included in this PR?
pub fn cast_to_variant_with_options(input: &dyn Array, strict: bool) -> Result<VariantArray, ArrowError>Are these changes tested?
Yes.
Are there any user-facing changes?
no.