Smallfu666 commented on PR #5162:
URL:
https://github.com/apache/datafusion-comet/pull/5162#issuecomment-5143315086
Thanks for the thorough review — all addressed in `f1acb5e50`:
- collapsed the duplicated checked-negation branches (and renamed
`map_neg_error`'s param to `from_type`)
- fixed the scalar Int8/Int16 messages to match the array path
- updated the stale comment, cut the test copy-paste
- added the null-slot regressions that were actually missing:
`IntervalYearMonth(i32::MIN)` and the whole `IntervalDayTime::MIN`
On the `DayTime` change in your item 1 — I checked this on `main` and I
don't think the behavior widened. `neg_wrapping` is `downcast_integer! { ..., _
=> neg(array) }`, so every non-integer type, intervals included, already went
through the checked kernel. `(i32::MIN, 0)` and `(0, i32::MIN)` did fail before
this PR:
```
DayTime valid (MIN,0) => ArrowError(ArithmeticOverflow("Overflow happened
on: - -2147483648"))
```
The failure set is the same; what changes is that these now map to
`SparkError::ArithmeticOverflow` like every other overflow here. I've reframed
that test accordingly and rewritten the description.
That also means the `MonthDayNano` follow-up doesn't hold — nothing silently
wraps there, since `neg_wrapping` is `neg` for that type too. So I dropped it.
The workflows are still waiting on a maintainer to approve the run.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
