rok commented on a change in pull request #12528:
URL: https://github.com/apache/arrow/pull/12528#discussion_r817740457
##########
File path: cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
##########
@@ -709,94 +709,96 @@ year_month_day GetFlooredYmd(int64_t arg, int multiple,
Localizer localizer_) {
template <typename Duration, typename Unit, typename Localizer>
const Duration FloorTimePoint(const int64_t arg, const int64_t multiple,
- Localizer localizer_, Status* st) {
+ Localizer localizer_, const RoundTemporalOptions
options,
+ Status* st) {
const auto t = localizer_.template ConvertTimePoint<Duration>(arg);
const Unit d = floor<Unit>(t).time_since_epoch();
if (multiple == 1) {
return localizer_.template
ConvertLocalToSys<Duration>(duration_cast<Duration>(d),
- st);
+ options, st);
} else {
const Unit unit = Unit{multiple};
const Unit m =
(d.count() >= 0) ? d / unit * unit : (d - unit + Unit{1}) / unit *
unit;
return localizer_.template
ConvertLocalToSys<Duration>(duration_cast<Duration>(m),
- st);
+ options, st);
}
}
template <typename Duration, typename Localizer>
const Duration FloorWeekTimePoint(const int64_t arg, const int64_t multiple,
Localizer localizer_, const Duration
weekday_offset,
- Status* st) {
+ const RoundTemporalOptions options, Status*
st) {
const auto t = localizer_.template ConvertTimePoint<Duration>(arg) +
weekday_offset;
const weeks d = floor<weeks>(t).time_since_epoch();
if (multiple == 1) {
return localizer_.template
ConvertLocalToSys<Duration>(duration_cast<Duration>(d),
- st) -
+ options, st) -
weekday_offset;
} else {
const weeks unit = weeks{multiple};
const weeks m =
(d.count() >= 0) ? d / unit * unit : (d - unit + weeks{1}) / unit *
unit;
return localizer_.template
ConvertLocalToSys<Duration>(duration_cast<Duration>(m),
- st) -
+ options, st) -
weekday_offset;
}
}
template <typename Duration, typename Unit, typename Localizer>
Duration CeilTimePoint(const int64_t arg, const int64_t multiple, Localizer
localizer_,
- Status* st) {
+ const RoundTemporalOptions options, Status* st) {
Review comment:
Would it make sense to have `arrow_vendored::date::choose` as a template
parameter rather than passing options every time the kernel is called? (I'm not
certain just asking)
Also note that we could probably do more templating for the
[ceil/floor/round kernels](https://issues.apache.org/jira/browse/ARROW-15787),
but that's out of scope here.
##########
File path: cpp/src/arrow/compute/kernels/temporal_internal.h
##########
@@ -120,18 +121,57 @@ struct ZonedLocalizer {
}
template <typename Duration>
- Duration ConvertLocalToSys(Duration t, Status* st) const {
+ Duration get_local_time(Duration arg) const {
+ return zoned_time<Duration>(tz, local_time<Duration>(arg))
+ .get_sys_time()
+ .time_since_epoch();
+ }
+
+ template <typename Duration>
+ Duration get_local_time(Duration arg, const arrow_vendored::date::choose
choose) const {
+ return zoned_time<Duration>(tz, local_time<Duration>(arg), choose)
+ .get_sys_time()
+ .time_since_epoch();
+ }
+
+ template <typename Duration>
+ Duration ConvertLocalToSys(Duration t, const RoundTemporalOptions options,
+ Status* st) const {
try {
return zoned_time<Duration>{tz, local_time<Duration>(t)}
.get_sys_time()
.time_since_epoch();
} catch (const arrow_vendored::date::nonexistent_local_time& e) {
- *st = Status::Invalid("Local time does not exist: ", e.what());
- return Duration{0};
+ switch (options.nonexistent) {
+ case NonexistentTime::NONEXISTENT_RAISE: {
+ *st = Status::Invalid("Timestamp doesn't exist in timezone '", tz,
+ "': ", e.what());
+ return t;
+ }
+ case NonexistentTime::NONEXISTENT_EARLIEST: {
+ return get_local_time<Duration>(t,
arrow_vendored::date::choose::latest) -
+ Duration{1};
+ }
+ case NonexistentTime::NONEXISTENT_LATEST: {
+ return get_local_time<Duration>(t,
arrow_vendored::date::choose::latest);
+ }
+ }
} catch (const arrow_vendored::date::ambiguous_local_time& e) {
- *st = Status::Invalid("Local time is ambiguous: ", e.what());
- return Duration{0};
+ switch (options.ambiguous) {
+ case AmbiguousTime::AMBIGUOUS_RAISE: {
+ *st = Status::Invalid("Timestamp is ambiguous in timezone '", tz,
+ "': ", e.what());
+ return t;
+ }
+ case AmbiguousTime::AMBIGUOUS_EARLIEST: {
+ return get_local_time<Duration>(t,
arrow_vendored::date::choose::earliest);
+ }
+ case AmbiguousTime::AMBIGUOUS_LATEST: {
+ return get_local_time<Duration>(t,
arrow_vendored::date::choose::latest);
+ }
+ }
}
+ return Duration{0};
}
Review comment:
Perhaps [`AssumeTimezone`
kernel](https://github.com/apache/arrow/blob/4aaf1f2e7e741fae5a8b02a514dcc891083d57b7/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc#L1181-L1216)
could reuse this now that you have it nicely factored out?
--
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]
