Enhance temporal kernels for Time32/64 types #5262
Conversation
| /// Extracts the time fraction of a given temporal datetime array as an array of integers. | ||
| /// | ||
| /// Does not support Time32/Time64, e.g. in cases when trying to extract month. | ||
| fn time_fraction_dyn_datetime<F>( | ||
| array: &dyn Array, | ||
| name: &str, | ||
| op: F, | ||
| ) -> Result<ArrayRef, ArrowError> | ||
| where | ||
| F: Fn(NaiveDateTime) -> i32, | ||
| { | ||
| match array.data_type().clone() { | ||
| match array.data_type() { | ||
| DataType::Dictionary(_, _) => { | ||
| downcast_dictionary_array!( | ||
| array => { | ||
| let values = time_fraction_dyn_datetime(array.values(), name, op)?; | ||
| Ok(Arc::new(array.with_values(values))) | ||
| } | ||
| dt => return_compute_error_with!(format!("{name} does not support"), dt), | ||
| ) | ||
| } | ||
| _ => { | ||
| downcast_temporal_array!( | ||
| array => { | ||
| time_fraction_internal_datetime(array, name, op) | ||
| .map(|a| Arc::new(a) as ArrayRef) | ||
| } | ||
| dt => return_compute_error_with!(format!("{name} does not support"), dt), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Extracts the time fraction of a given temporal time array as an array of integers. | ||
| /// | ||
| /// Supports Time32/Time64 types. | ||
| fn time_fraction_dyn_time<F>(array: &dyn Array, name: &str, op: F) -> Result<ArrayRef, ArrowError> | ||
| where | ||
| F: Fn(NaiveTime) -> i32, | ||
| { | ||
| match array.data_type() { |
There was a problem hiding this comment.
Duplication ugly here, but wasn't sure how to reduce without falling back to something like macros
Figured since it's only duplicated once and is internal code, might be ok for now?
|
I'm happy to review this in more detail if you would like me to, but I would like to float the possibility of taking the opportunity to revisit how these kernels are implemented from a more holistic perspective, to make them faster and easier to extend/maintain. I note that you've also commented on issues related to supporting these kernels for IntervalArray, which would worsen the code duplication that this PR already introduces. I filed #5266 with a high-level overview of what this might look like, in case this is something you would be interested in? |
|
I agree with the approach you described in #5266 Where that leaves this PR, I'm not too particularly fussed. But when considering review capacity, I could close this PR to focus efforts on #5266 instead, as this enhancement was something I raised because I noticed the gap on DataFusion's side: apache/datafusion#8692 And doesn't seem to be a pressing requirement |
I think lets mark this PR as a draft for now and we can come back to it if #5266 turns out to be a dead-end. On an unrelated note, I have something I would like to discuss with you if you could perhaps reach out to me on Discord/Slack or email, as I currently lack a means of getting in touch with you. You can reach me on my email [email protected] |
|
I'll rework this as structure drastically changed with #5319 |
Which issue does this PR close?
Closes #5261
Rationale for this change
Want to be able to extract hour/minute/second/etc. from Time32/Time64 types via temporal kernels
What changes are included in this PR?
Refactor temporal kernel handling to have special case for Time types
Are there any user-facing changes?