zeroshade commented on code in PR #1127:
URL: https://github.com/apache/arrow-go/pull/1127#discussion_r3855670209
##########
arrow/compute/internal/kernels/rounding.go:
##########
@@ -1056,90 +1198,185 @@ func roundTimestampCalendar(tsNanos int64, inputUnit
arrow.TimeUnit, tz *time.Lo
t := time.Unix(secs, nanos).In(tz)
var rounded time.Time
+ var err error
+ multiple := opts.Multiple
switch opts.Unit {
case RoundTemporalYear:
- year := t.Year()
- roundedYear := (year / int(opts.Multiple)) * int(opts.Multiple)
+ year := int64(t.Year())
+ roundedYear, err := checkedMulInt64(year/multiple, multiple)
+ if err != nil {
+ return 0, err
+ }
switch opts.mode {
case RoundDown:
- rounded = time.Date(roundedYear, 1, 1, 0, 0, 0, 0, tz)
+ var dateErr error
+ rounded, dateErr = checkedCalendarDate(roundedYear, 1,
tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
case RoundUp:
- periodStart := time.Date(roundedYear, 1, 1, 0, 0, 0, 0,
tz)
+ periodStart, dateErr :=
checkedCalendarDate(roundedYear, 1, tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
if opts.CeilIsStrictlyGreater || !t.Equal(periodStart) {
- roundedYear += int(opts.Multiple)
- rounded = time.Date(roundedYear, 1, 1, 0, 0, 0,
0, tz)
+ roundedYear, err = checkedAddInt64(roundedYear,
multiple)
+ if err != nil {
+ return 0, err
+ }
+ rounded, dateErr =
checkedCalendarDate(roundedYear, 1, tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
} else {
rounded = periodStart
}
default:
- yearStart := time.Date(roundedYear, 1, 1, 0, 0, 0, 0,
tz)
- nextYear := roundedYear + int(opts.Multiple)
- yearEnd := time.Date(nextYear, 1, 1, 0, 0, 0, 0, tz)
+ yearStart, dateErr := checkedCalendarDate(roundedYear,
1, tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
+ nextYear, dateErr := checkedAddInt64(roundedYear,
multiple)
+ if dateErr != nil {
+ return 0, dateErr
+ }
+ yearEnd, dateErr := checkedCalendarDate(nextYear, 1, tz)
Review Comment:
**Blocking:** Restricting this intermediate period boundary to the
nanosecond timestamp window rejects inputs whose selected result is
representable. For `timestamp[ns]` `2262-01-05` rounded half-up to one year,
the correct result is `2262-01-01`, but this checks the unused `2263-01-01`
boundary and returns overflow. Likewise, `2201-01-01` rounded to 100 years
should return `2200-01-01`, but checking the 2300 boundary fails. Main succeeds
in both cases. Please permit calendar boundaries outside the output window and
validate only the selected result with `timeToNanos`; cover year, quarter, and
month boundaries.
##########
arrow/compute/internal/kernels/rounding.go:
##########
@@ -959,22 +983,104 @@ func canRoundInInputUnit(inputUnit arrow.TimeUnit,
roundingIntervalNanos int64)
return roundingIntervalNanos%int64(inputUnit.Multiplier()) == 0
}
-// convertToNanos converts a timestamp value to nanoseconds
-func convertToNanos(ts int64, unit arrow.TimeUnit) int64 {
- return ts * int64(unit.Multiplier())
+func overflowError() error {
+ return fmt.Errorf("%w: temporal rounding overflow", arrow.ErrInvalid)
+}
+
+func checkedAddInt64(left, right int64) (int64, error) {
+ if (right > 0 && left > math.MaxInt64-right) || (right < 0 && left <
math.MinInt64-right) {
+ return 0, overflowError()
+ }
+ return left + right, nil
+}
+
+func checkedSubInt64(left, right int64) (int64, error) {
+ if (right > 0 && left < math.MinInt64+right) || (right < 0 && left >
math.MaxInt64+right) {
+ return 0, overflowError()
+ }
+ return left - right, nil
+}
+
+func checkedMulInt64(left, right int64) (int64, error) {
+ if left == 0 || right == 0 {
+ return 0, nil
+ }
+ if (left == math.MinInt64 && right == -1) || (right == math.MinInt64 &&
left == -1) {
+ return 0, overflowError()
+ }
+
+ result := left * right
+ if result/right != left {
+ return 0, overflowError()
+ }
+ return result, nil
+}
+
+func checkedInt64ToInt(value int64) (int, error) {
+ result := int(value)
+ if int64(result) != value {
+ return 0, overflowError()
+ }
+ return result, nil
+}
+
+// Nanosecond timestamps can only represent dates in this year range. Keep
+// calendar arithmetic inside it before constructing a time.Time so large
+// options cannot wrap inside the time package.
+const (
+ minNanosecondTimestampYear = 1677
+ maxNanosecondTimestampYear = 2262
+ maxCalendarDayOffset = (maxNanosecondTimestampYear -
minNanosecondTimestampYear + 1) * 366
+)
+
+func checkedCalendarDate(year, month int64, tz *time.Location) (time.Time,
error) {
+ if year < minNanosecondTimestampYear || year >
maxNanosecondTimestampYear {
+ return time.Time{}, overflowError()
+ }
+
+ dateYear, err := checkedInt64ToInt(year)
+ if err != nil {
+ return time.Time{}, err
+ }
+ dateMonth, err := checkedInt64ToInt(month)
+ if err != nil {
+ return time.Time{}, err
+ }
+ return time.Date(dateYear, time.Month(dateMonth), 1, 0, 0, 0, 0, tz),
nil
+}
+
+func checkedCalendarAddDays(value time.Time, days int64) (time.Time, error) {
+ if days < -maxCalendarDayOffset || days > maxCalendarDayOffset {
+ return time.Time{}, overflowError()
+ }
+
+ dayOffset, err := checkedInt64ToInt(days)
+ if err != nil {
+ return time.Time{}, err
+ }
+ result := value.AddDate(0, 0, dayOffset)
+ if result.Year() < minNanosecondTimestampYear || result.Year() >
maxNanosecondTimestampYear {
+ return time.Time{}, overflowError()
+ }
+ return result, nil
+}
+
+// convertToNanos converts a timestamp value to nanoseconds.
+func convertToNanos(ts int64, unit arrow.TimeUnit) (int64, error) {
Review Comment:
**Blocking:** Every calendar path is still narrowed to an `int64` nanosecond
intermediate, even when the input and correct result are representable in their
original type. For example, flooring `Date32(1500-06-15)` to one year should
return `1500-01-01`, but this PR returns overflow. The same affects Date64 and
second/millisecond/microsecond timestamps, whose ranges are much wider than
timestamp nanoseconds. Please perform calendar arithmetic without narrowing to
nanoseconds and range-check the final result in the input unit.
##########
arrow/compute/internal/kernels/rounding.go:
##########
@@ -1056,90 +1198,185 @@ func roundTimestampCalendar(tsNanos int64, inputUnit
arrow.TimeUnit, tz *time.Lo
t := time.Unix(secs, nanos).In(tz)
var rounded time.Time
+ var err error
+ multiple := opts.Multiple
switch opts.Unit {
case RoundTemporalYear:
- year := t.Year()
- roundedYear := (year / int(opts.Multiple)) * int(opts.Multiple)
+ year := int64(t.Year())
+ roundedYear, err := checkedMulInt64(year/multiple, multiple)
+ if err != nil {
+ return 0, err
+ }
switch opts.mode {
case RoundDown:
- rounded = time.Date(roundedYear, 1, 1, 0, 0, 0, 0, tz)
+ var dateErr error
+ rounded, dateErr = checkedCalendarDate(roundedYear, 1,
tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
case RoundUp:
- periodStart := time.Date(roundedYear, 1, 1, 0, 0, 0, 0,
tz)
+ periodStart, dateErr :=
checkedCalendarDate(roundedYear, 1, tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
if opts.CeilIsStrictlyGreater || !t.Equal(periodStart) {
- roundedYear += int(opts.Multiple)
- rounded = time.Date(roundedYear, 1, 1, 0, 0, 0,
0, tz)
+ roundedYear, err = checkedAddInt64(roundedYear,
multiple)
+ if err != nil {
+ return 0, err
+ }
+ rounded, dateErr =
checkedCalendarDate(roundedYear, 1, tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
} else {
rounded = periodStart
}
default:
- yearStart := time.Date(roundedYear, 1, 1, 0, 0, 0, 0,
tz)
- nextYear := roundedYear + int(opts.Multiple)
- yearEnd := time.Date(nextYear, 1, 1, 0, 0, 0, 0, tz)
+ yearStart, dateErr := checkedCalendarDate(roundedYear,
1, tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
+ nextYear, dateErr := checkedAddInt64(roundedYear,
multiple)
+ if dateErr != nil {
+ return 0, dateErr
+ }
+ yearEnd, dateErr := checkedCalendarDate(nextYear, 1, tz)
+ if dateErr != nil {
+ return 0, dateErr
+ }
rounded = halfRoundPeriod(t, yearStart, yearEnd)
Review Comment:
**Blocking:** `halfRoundPeriod` computes `periodEnd.Sub(periodStart)/2`;
`time.Time.Sub` saturates at `math.MaxInt64` for periods longer than about 292
years. Consequently, rounding `1948-01-01` to a 300-year multiple uses a
midpoint in 1946 instead of the true 1950 midpoint and returns 2100 rather than
1800. This is pre-existing, but it is an unchecked calendar-rounding overflow
directly within this PR’s scope. Please compute calendar midpoints without
converting the entire period to `time.Duration`, with a regression test for a
period over 292 years.
--
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]
