vismaytiwari commented on PR #23279: URL: https://github.com/apache/datafusion/pull/23279#issuecomment-4993469444
@Jefffrey yeah, I don't think other systems help here — but we've actually got the precedent in-tree. `get_result` hands `timestamp/date + interval` straight to arrow's `add_wrapping`, and arrow keeps the input unit: `timestamp(s) + interval '1 nanosecond'` already comes back as `timestamp(s)` with the sub-second part truncated. So the edge case you're worried about isn't new — it's exactly what timestamps do today. That gives us a consistent option: preserve the time unit and apply the interval at that resolution, so `time(s) + interval '1 nanosecond'` → `time(s)` unchanged, same as the timestamp case. The only cost is that silent truncation for the coarse time types. The current PR goes the other way (always widen to `time(ns)`), which keeps the precision but changes the column type. I lean toward preserving the unit — it matches timestamp/date arithmetic and keeps a `time(s)` column `time(s)`. It's contained: keep the time type in the coercion instead of forcing `Time64(ns)`, and branch `apply_time_interval` per unit (right now everything funnels through one ns wrap). Want me to go that way? -- 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]
