kosiew commented on code in PR #22027:
URL: https://github.com/apache/datafusion/pull/22027#discussion_r3193864775
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -754,22 +740,16 @@ impl Interval {
/// disjoint with `other` by returning `[true, true]`, `[false, true]` or
/// `[false, false]` respectively.
///
- /// NOTE: This function only works with intervals of the same data type.
- /// Attempting to compare intervals of different data types will lead
- /// to an error.
+ /// If the two intervals have different data types, both are coerced to a
+ /// common comparison type via [`comparison_coercion`] before checking
+ /// containment.
pub fn contains<T: Borrow<Self>>(&self, other: T) -> Result<Self> {
Review Comment:
Nice improvement overall. Since this PR explicitly relaxes `intersect` /
`union` / `contains`, it could be helpful to add a couple of focused unit tests
that assert the actual bounds and boolean outcomes for mismatched Decimal128
precision/scale intervals.
Right now the new direct unit test only exercises `mul` / `div` and checks
result types. Adding small cases for `contains` returning TRUE / FALSE /
TRUE_OR_FALSE would help lock in the comparison coercion behavior at the API
layer too, not just through the SQL regression tests.
--
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]
