mbutrovich commented on code in PR #11199:
URL: https://github.com/apache/arrow-rs/pull/11199#discussion_r4098305747
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -645,11 +731,16 @@ fn timestamp_to_date32<T: ArrowTimestampType>(
.map(|d| Date32Type::from_naive_date(d.date_naive()))
})?
}
- None => array.try_unary(|x| {
- as_datetime::<T>(x)
- .ok_or_else(|| err(x))
- .map(|d| Date32Type::from_naive_date(d.date()))
- })?,
+ None => {
+ // Date32 stores days since the epoch. Round down so that a
timestamp
+ // just before the epoch belongs to the preceding day.
+ let days = |x: i64| x.div_euclid(SECONDS_IN_DAY *
time_unit_multiple(&T::UNIT));
+ match T::UNIT {
+ // Every microsecond or nanosecond timestamp lies within the
Date32 range.
+ TimeUnit::Microsecond | TimeUnit::Nanosecond =>
array.unary(|x| days(x) as i32),
+ _ => array.try_unary(|x| i32::try_from(days(x)).map_err(|_|
err(x)))?,
Review Comment:
With `safe: true`, should a day count that doesn't fit in `i32` produce
`NULL` instead of an error? That's what `CastOptions::safe` documents, and the
`Timestamp(Second)` to `Date64` cast right below this one handles overflow that
way with `unary_opt`. The new doc line on 770 describes the current behavior
accurately, but once it's documented it becomes harder to change later. The
timezone branch of `timestamp_to_date32` ignores `safe` too, and that behavior
predates this PR. If you'd rather keep this PR focused on performance, could
you open an issue for honoring `safe` in both branches and link it here?
##########
arrow-cast/src/cast/mod.rs:
##########
@@ -674,6 +765,9 @@ fn timestamp_to_date32<T: ArrowTimestampType>(
/// * `Date32` and `Date64`: precision lost when going to higher interval
/// * `Time32` and `Time64`: precision lost when going to higher interval
/// * `Timestamp` and `Date{32|64}`: precision lost when going to higher
interval
+/// * `Timestamp` without a timezone to `Date32`, `Time32`, or `Time64`:
supports
+/// timestamps outside Chrono's date range. A `Date32` day count that does
not fit
+/// in `i32` returns an error, regardless of [`CastOptions::safe`].
Review Comment:
This is the same question I raised on #11187, and I think it would be good
to settle it once for both PRs. I'm not sure which behavior is better. After
this PR, `i64::MAX` seconds casts to `Time64` without a timezone, but the same
value with `+00:00` returns an error, as
`test_cast_timestamp_date_time_timezone_validation` checks.
If we'd like to keep the current semantics, the range check I suggested on
#11187 would work here too: one branch-free pass over `values()` against
Chrono's bounds, skipped for nanoseconds, with a fallback to the existing path.
On `date_part` it cost about 0.7 us per 8192 values. I haven't measured it on
the casts. If we keep the new behavior instead, the `api-change` label would
make sure it shows up in the changelog. What do you and the other maintainers
think?
--
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]
