[Variant] Implement DataType::{Date32,Date64} => Variant::Date#8081
[Variant] Implement DataType::{Date32,Date64} => Variant::Date#8081alamb merged 1 commit intoapache:mainfrom
DataType::{Date32,Date64} => Variant::Date#8081Conversation
There was a problem hiding this comment.
New tweak to the macro here. Made cast_fn fallible so that an ArrowError::CastError can be returned if something goes wrong.
There was a problem hiding this comment.
A potential downside to this tweak is that it forces the caller's closure to handle a Result even if the cast is known to be infallible, like the case of f16 -> f32.
There was a problem hiding this comment.
If we don't want to tweak the macro, we could call expect or unwrap here.
There was a problem hiding this comment.
Another behavior which might be better is writing Variant::Null when the value can not be converted 🤔
There was a problem hiding this comment.
Could potentially add a negative test here, although it's difficult to construct an invalid Date64Type.
superserious-dev
force-pushed
the
date-cast-to-variant
branch
from
7379a75 to
bd710e1
Compare
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @superserious-dev -- sorry for the delay in reviewing. I left a comment about returning Variant::Null vs an error on conversion failure -- let me know what you think
There was a problem hiding this comment.
Another behavior which might be better is writing Variant::Null when the value can not be converted 🤔
superserious-dev
force-pushed
the
date-cast-to-variant
branch
from
bd710e1 to
ae6d851
Compare
superserious-dev
force-pushed
the
date-cast-to-variant
branch
from
ae6d851 to
df1a65b
Compare
That's an interesting idea. One issue could be that a Null value in the Output Variant would represent 2 things: either a) an error in the casting process or b) a Null value in the input. For now, I undid the macro change and did |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @superserious-dev
| generic_conversion!( | ||
| Date64Type, | ||
| as_primitive, | ||
| |v: i64| { Date64Type::to_naive_date_opt(v).unwrap() }, |
There was a problem hiding this comment.
this is probably good for now. I'll file a follow on ticket to track handling this case
There was a problem hiding this comment.
|
Thank you @superserious-dev |
alamb
changed the title
DataType::{Date32,Date64} => Variant::DateDataType::{Date32,Date64} => Variant::Date
2 participants
Which issue does this PR close?
DataType::Date32 / DataType::Date64support forcast_to_variantkernel #8054Rationale for this change
Adds Date32, Date64 conversions to the cast_to_variant kernel
What changes are included in this PR?
Are these changes tested?
Yes, additional unit tests have been added.
Currently there is no negative test for a Date64Array with a date element out-of-range.
Are there any user-facing changes?
Yes, adds new type conversions to kernel