Toby1009 opened a new issue, #25925:
URL: https://github.com/apache/datafusion/issues/25925

   ### Is your feature request related to a problem or challenge?
   
   Follow-up to the [review of 
#25668](https://github.com/apache/datafusion/pull/25668#discussion_r4147165778).
   
   The `time ± interval` SQL tests added there still pass when the central 
type-recovery fallback is removed. SQL-planned arithmetic uses 
`fail_on_overflow = false`, and the unbounded result range already makes the 
overflow rule return `Unordered`, whether the function's range type is `Time64` 
or `Null`.
   
   With checked arithmetic, however, the recovered type is needed to recognize 
that time-of-day arithmetic wraps across midnight. A local ablation experiment 
confirmed that removing the fallback makes checked `date_trunc('hour', t) + 
INTERVAL '2 hours'` incorrectly report `Ordered`.
   
   The merged fallback handles this correctly; this issue tracks stronger 
regression coverage and more precise documentation.
   
   ### Describe the solution you'd like
   
   - Add a unit test constructing `date_trunc('hour', t) + INTERVAL '2 hours'` 
as a `BinaryExpr` with `with_fail_on_overflow(true)` over an ordered 
`Time64(Nanosecond)` child, and assert `Unordered`.
   - Verify that the test fails when the central fallback is removed. Keep the 
existing SQL result and plan tests.
   - Clarify that type recovery applies to any unbounded `Null` interval, 
including one returned by an override, while other intervals are preserved. 
Retain the original interval if the resolved return type cannot be represented 
by `Interval::make_unbounded`.
   - Describe the unchanged path as direct calls to 
`PhysicalExpr::evaluate_bounds`, rather than implying that scalar UDFs 
participate in the normal constraint-solver path. `check_support` currently 
excludes them.
   - Consider separating the existing bounds-preservation and error-propagation 
test cases for readability, as suggested in review.
   
   ### Describe alternatives you've considered
   
   Relying only on the existing SQL tests does not catch this regression: 
removing the fallback leaves their results and sorts unchanged. Checked 
arithmetic exposes the type-dependent time wrapping rule.
   
   ### Additional context
   
   - #25668: central scalar UDF type recovery.
   - [Documentation 
review](https://github.com/apache/datafusion/pull/25668#discussion_r4147165785).
   - [Test readability 
review](https://github.com/apache/datafusion/pull/25668#discussion_r4147165795).
   


-- 
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]

Reply via email to