zeroshade commented on code in PR #1127:
URL: https://github.com/apache/arrow-go/pull/1127#discussion_r3865557818
##########
arrow/compute/internal/kernels/rounding.go:
##########
@@ -1187,22 +1485,42 @@ func roundTimestampCalendar(tsNanos int64, inputUnit
arrow.TimeUnit, tz *time.Lo
rounded = startOfDay
case RoundUp:
if opts.CeilIsStrictlyGreater || !t.Equal(startOfDay) {
- rounded = startOfDay.AddDate(0, 0, 1)
+ rounded, err =
checkedCalendarAddDays(startOfDay, 1)
} else {
rounded = startOfDay
}
default:
- nextDay := startOfDay.AddDate(0, 0, 1)
- rounded = halfRoundPeriod(t, startOfDay, nextDay)
+ nextDay, dateErr := checkedCalendarAddDays(startOfDay,
1)
+ if dateErr != nil {
+ return 0, dateErr
+ }
+ rounded, err = halfRoundPeriod(t, startOfDay, nextDay)
+ if err != nil {
+ return 0, err
+ }
}
default:
return 0, fmt.Errorf("%w: unsupported calendar unit",
arrow.ErrNotImplemented)
}
+ if err != nil {
+ return 0, err
+ }
+
+ // Convert back to the input unit, validating only the selected result.
+ roundedTimestamp, err := arrow.TimestampFromTime(rounded, inputUnit)
Review Comment:
**Blocking:** This does not actually validate second-resolution results:
`arrow.TimestampFromTime` returns `val.Unix()` without a range check for
`arrow.Second`. Ceiling `math.MaxInt64` seconds to the next year therefore
succeeds with wrapped value `-9223372036852412416` instead of returning
overflow. Please explicitly validate second-unit conversion (or make
`TimestampFromTime` do so) and add a boundary test.
##########
arrow/compute/internal/kernels/rounding.go:
##########
@@ -1160,23 +1423,58 @@ func roundTimestampCalendar(tsNanos int64, inputUnit
arrow.TimeUnit, tz *time.Lo
epochWeekStart := epochInTz.AddDate(0, 0, -epochWeekday)
epochWeekStart = time.Date(epochWeekStart.Year(),
epochWeekStart.Month(), epochWeekStart.Day(), 0, 0, 0, 0, tz)
- daysSinceEpochWeek :=
int(startOfWeek.Sub(epochWeekStart).Hours() / 24)
+ daysSinceEpochWeek :=
int64(startOfWeek.Sub(epochWeekStart).Hours() / 24)
Review Comment:
**Blocking:** This reintroduces the same approximately 292-year saturation
through `time.Time.Sub`. Flooring a second-resolution `1500-06-15 12:34:56`
timestamp to one week returns `1677-09-19` instead of `1500-06-10` because the
day distance saturates before division. Please calculate calendar-day/week
distance without `time.Duration`, with wide-range week regression coverage.
##########
arrow/compute/internal/kernels/rounding.go:
##########
@@ -912,43 +916,55 @@ func roundTimestamp(ts int64, inputUnit arrow.TimeUnit,
tz *time.Location, opts
// Calendar units with variable duration (year, quarter, month, week)
require date arithmetic
if !opts.isSubDay {
- tsNanos := convertToNanos(ts, inputUnit)
- return roundTimestampCalendar(tsNanos, inputUnit, tz, opts)
+ return roundTimestampCalendar(ts, inputUnit, tz, opts)
}
// Day rounding with timezone requires calendar arithmetic (days vary:
23/24/25 hours due to DST)
isUTC := tz == time.UTC || tz.String() == "UTC"
if !isUTC && opts.Unit == RoundTemporalDay {
- tsNanos := convertToNanos(ts, inputUnit)
- return roundTimestampCalendar(tsNanos, inputUnit, tz, opts)
+ return roundTimestampCalendar(ts, inputUnit, tz, opts)
}
// Sub-day units (hour, minute, second, etc.) use fixed-duration
arithmetic
// Fast path: round directly in input unit if possible (no origin,
compatible units)
if canRoundInInputUnit(inputUnit, opts.unitNanos) &&
!opts.useCalendarOrigin {
intervalInInputUnit := opts.roundingInterval /
int64(inputUnit.Multiplier())
- rounded := roundToMultipleInt64(ts, intervalInInputUnit,
opts.mode, opts.CeilIsStrictlyGreater)
- return rounded, nil
+ return roundToMultipleInt64(ts, intervalInInputUnit, opts.mode,
opts.CeilIsStrictlyGreater)
}
// Slow path: convert to nanoseconds for calendar origin or
incompatible units
- tsNanos := convertToNanos(ts, inputUnit)
+ tsNanos, err := convertToNanos(ts, inputUnit)
Review Comment:
**Blocking:** The fixed-duration slow path still rejects representable
wider-unit values when the requested rounding is finer than the input
resolution. For example, flooring a second-resolution `1500-06-15 12:34:56`
timestamp to one nanosecond is an exact no-op, but this conversion returns
`invalid: temporal rounding overflow`. Please avoid narrowing when the input is
already aligned to the requested interval, and add coverage outside the
timestamp-nanosecond window.
##########
arrow/compute/internal/kernels/rounding.go:
##########
@@ -1005,142 +1102,308 @@ func roundToMultipleInt64(value, multiple int64, mode
RoundMode, strictCeil bool
// a remainder of 1 when rounding to multiples of 3 is closer
to 0
// than to 3, so it must not be treated as a tie.
if absRemainder < half || (multiple%2 != 0 && absRemainder ==
half) {
- return quotient * multiple
+ return checkedMulInt64(quotient, multiple)
} else if absRemainder > half {
if remainder > 0 {
- return (quotient + 1) * multiple
+ quotient, err := checkedAddInt64(quotient, 1)
+ if err != nil {
+ return 0, err
+ }
+ return checkedMulInt64(quotient, multiple)
+ }
+ quotient, err := checkedSubInt64(quotient, 1)
+ if err != nil {
+ return 0, err
}
- return (quotient - 1) * multiple
+ return checkedMulInt64(quotient, multiple)
} else {
// Exactly on the halfway point
switch mode {
case HalfDown:
if remainder > 0 {
- return quotient * multiple
+ return checkedMulInt64(quotient,
multiple)
}
- return (quotient - 1) * multiple
+ quotient, err := checkedSubInt64(quotient, 1)
+ if err != nil {
+ return 0, err
+ }
+ return checkedMulInt64(quotient, multiple)
case HalfUp:
if remainder > 0 {
- return (quotient + 1) * multiple
+ quotient, err :=
checkedAddInt64(quotient, 1)
+ if err != nil {
+ return 0, err
+ }
+ return checkedMulInt64(quotient,
multiple)
}
- return quotient * multiple
+ return checkedMulInt64(quotient, multiple)
case HalfToEven:
if quotient%2 == 0 {
- return quotient * multiple
+ return checkedMulInt64(quotient,
multiple)
}
if remainder > 0 {
- return (quotient + 1) * multiple
+ quotient, err :=
checkedAddInt64(quotient, 1)
+ if err != nil {
+ return 0, err
+ }
+ return checkedMulInt64(quotient,
multiple)
+ }
+ quotient, err := checkedSubInt64(quotient, 1)
+ if err != nil {
+ return 0, err
}
- return (quotient - 1) * multiple
+ return checkedMulInt64(quotient, multiple)
}
}
}
- return quotient * multiple
+ return checkedMulInt64(quotient, multiple)
}
-// halfRoundPeriod performs half-rounding by finding the midpoint between
period start and end
-func halfRoundPeriod(t, periodStart, periodEnd time.Time) time.Time {
- midPoint := periodStart.Add(periodEnd.Sub(periodStart) / 2)
+// halfRoundPeriod performs half-rounding by finding the midpoint between
period start and end.
+// It does not use time.Time.Sub because that method saturates for periods
longer than
+// approximately 292 years.
+func halfRoundPeriod(t, periodStart, periodEnd time.Time) (time.Time, error) {
+ if periodEnd.Before(periodStart) {
+ return time.Time{}, overflowError()
+ }
+
+ startSeconds := periodStart.Unix()
Review Comment:
**Blocking:** `time.Time.Unix()` is undefined once a boundary lies outside
the `int64` seconds range, so this still rejects a representable selected
result at the edge of `timestamp[s]`. With an input of January 2 in the year
containing `math.MaxInt64` seconds, half-up rounding to one year should select
that year's representable January 1; the next-year boundary exceeds `int64`,
these calls wrap, and `checkedSubInt64` returns overflow. Please compute the
midpoint without requiring both boundaries to fit Unix seconds and cover this
edge.
--
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]
