Jefffrey commented on code in PR #9613:
URL: https://github.com/apache/arrow-datafusion/pull/9613#discussion_r1529225827
##########
datafusion-examples/Cargo.toml:
##########
@@ -75,6 +75,6 @@ serde_json = { workspace = true }
tempfile = { workspace = true }
tokio = { workspace = true, features = ["rt-multi-thread", "parking_lot"] }
# 0.10 and 0.11 are incompatible. Need to upgrade tonic to 0.11 when upgrading
to arrow 51
Review Comment:
```suggestion
```
No longer need this comment
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -187,83 +144,46 @@ impl ScalarUDFImpl for DatePartFunc {
}
}
-fn to_ticks<T>(array: &PrimitiveArray<T>, frac: i32) -> Result<Float64Array>
-where
- T: ArrowTemporalType + ArrowNumericType,
- i64: From<T::Native>,
-{
- let zipped = temporal::second(array)?
- .values()
- .iter()
- .zip(temporal::nanosecond(array)?.values().iter())
- .map(|o| (*o.0 as f64 + (*o.1 as f64) / 1_000_000_000.0) * (frac as
f64))
- .collect::<Vec<f64>>();
-
- Ok(Float64Array::from(zipped))
+/// Invoke [`date_part`] and cast the result to Float64
+fn date_part_f64(array: &dyn Array, part: DatePart) -> Result<ArrayRef> {
+ Ok(cast(date_part(array, part)?.as_ref(), &Float64)?)
}
-fn seconds<T>(array: &PrimitiveArray<T>) -> Result<Float64Array>
-where
- T: ArrowTemporalType + ArrowNumericType,
- i64: From<T::Native>,
-{
- to_ticks(array, 1)
-}
-
-fn millis<T>(array: &PrimitiveArray<T>) -> Result<Float64Array>
-where
- T: ArrowTemporalType + ArrowNumericType,
- i64: From<T::Native>,
-{
- to_ticks(array, 1_000)
-}
-
-fn micros<T>(array: &PrimitiveArray<T>) -> Result<Float64Array>
-where
- T: ArrowTemporalType + ArrowNumericType,
- i64: From<T::Native>,
-{
- to_ticks(array, 1_000_000)
+fn seconds(array: &dyn Array, unit: TimeUnit) -> Result<ArrayRef> {
+ let sf = match unit {
+ Second => 1_f64,
+ Millisecond => 1_000_f64,
+ Microsecond => 1_000_000_f64,
+ Nanosecond => 1_000_000_000_f64,
+ };
+ let secs = date_part(array, DatePart::Second)?;
+ let secs = as_int32_array(secs.as_ref())?;
Review Comment:
Is it worth making a note somewhere here that `array` must be a
`PrimitiveArray`? Otherwise the `as_int32_array()` can panic if it is a
dictionary, and maybe want to make this clear to anyone viewing the code hoping
to make it work for dictionary in the future?
(Previous code encoded this by having `array` be a `&PrimitiveArray<T>`)
--
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]
