Jefffrey commented on code in PR #5319:
URL: https://github.com/apache/arrow-rs/pull/5319#discussion_r1463933933
##########
arrow-arith/src/temporal.rs:
##########
@@ -228,14 +372,14 @@ where
T: ArrowTemporalType + ArrowNumericType,
i64: From<T::Native>,
{
- time_fraction_internal(array, "year", |t| t.year())
+ date_part_primitive(array, DatePart::Year)
}
/// Extracts the quarter of a given temporal array as an array of integersa
within
/// the range of [1, 4]. If the given array isn't temporal primitive or
dictionary array,
/// an `Err` will be returned.
pub fn quarter_dyn(array: &dyn Array) -> Result<ArrayRef, ArrowError> {
Review Comment:
Probably for the best, as if in the future we want to support more parts
like `century` or `decade` in arrow-rs natively, we wouldn't have to add these
shim functions for consistency if they are already deprecated.
That does raise a question that comes to mind, that if we add more of these
parts, by extending the `DatePart` enum, would that constitute a breaking API
change? Since we're introducing a new enum variant. Maybe mark it as
non-exhaustive, or would this be unnecessary since its mainly used as an input
to the `date_part()` function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
