scovich commented on code in PR #9491:
URL: https://github.com/apache/arrow-rs/pull/9491#discussion_r2867946603
##########
arrow-array/src/types.rs:
##########
@@ -350,14 +351,15 @@ pub trait ArrowTimestampType: ArrowTemporalType<Native =
i64> {
chrono::offset::LocalResult::Ambiguous(dt1, _) =>
Self::from_datetime(dt1),
chrono::offset::LocalResult::None => None,
},
- None => Self::make_value(naive),
+ None => Self::from_datetime(naive.and_utc()),
}
}
}
impl ArrowTimestampType for TimestampSecondType {
const UNIT: TimeUnit = TimeUnit::Second;
+ #[allow(deprecated)]
Review Comment:
aside: a bit weird that you have to allow implementing a required trait
method?
##########
arrow-array/src/types.rs:
##########
@@ -324,6 +324,7 @@ pub trait ArrowTimestampType: ArrowTemporalType<Native =
i64> {
/// Creates a ArrowTimestampType::Native from the provided
[`NaiveDateTime`]
///
/// See [`DataType::Timestamp`] for more information on timezone handling
+ #[deprecated(since = "59.0.0", note = "Use from_naive_datetime instead")]
Review Comment:
Will there not be any 58.x minor releases? Or is does PR need
`next-major-release` label?
##########
arrow-array/src/types.rs:
##########
@@ -417,7 +422,7 @@ fn add_year_months<T: ArrowTimestampType>(
let res = as_datetime_with_timezone::<T>(timestamp, tz)?;
let res = add_months_datetime(res, months)?;
let res = res.naive_utc();
- T::make_value(res)
+ T::from_naive_datetime(res, None)
Review Comment:
nit: single-use val considered annoying when it doesn't help save space...
```rust
T::from_naive_datetime(res.naive_utc(), None)
```
(several more below, if you choose to change this)
--
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]
