haohuaijin commented on code in PR #25985:
URL: https://github.com/apache/datafusion/pull/25985#discussion_r4172461788
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -1640,10 +1632,10 @@ fn div_helper_lhs_zero_inclusive(
dt: &DataType,
lhs: &Interval,
rhs: &Interval,
- zero_point: &Interval,
+ zero: &ScalarValue,
) -> Interval {
// With the following interval bounds, there is no possibility to create
an invalid interval.
- if rhs.upper <= zero_point.lower && !rhs.upper.is_null() {
+ if rhs.upper <= *zero && !rhs.upper.is_null() {
Review Comment:
Added exact-bound cases for `[-6,6] / [-3,0]` and `[-6,6] / [0,3]` to
`test_div`. Both assert the current conservative unbounded result, since
divisor endpoint shrinking is being kept out of this PR.
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -3674,6 +3668,55 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_div_zero_endpoints() -> Result<()> {
Review Comment:
Merged the four zero-endpoint cases into the existing `test_div` table and
removed `test_div_zero_endpoints` and its duplicate assertion loop.
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -3674,6 +3668,55 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_div_zero_endpoints() -> Result<()> {
+ let cases = [
+ ((-3_i64, 0_i64), (-2_i64, -1_i64), (0_i64, 3_i64)),
+ ((-3, 0), (1, 2), (-3, 0)),
+ ((0, 3), (-2, -1), (-3, 0)),
+ ((0, 3), (1, 2), (0, 3)),
+ ];
+ for ((lo, hi), (rlo, rhi), (expected_lo, expected_hi)) in cases {
+ let lhs = Interval::make(Some(lo), Some(hi))?;
+ let rhs = Interval::make(Some(rlo), Some(rhi))?;
+ assert_eq!(
+ lhs.div(&rhs)?,
+ Interval::make(Some(expected_lo), Some(expected_hi))?,
+ "{lhs:?} / {rhs:?}",
+ );
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_div_small_integer_intervals_contain_runtime_results() ->
Result<()> {
Review Comment:
Added `UInt32` and `UInt64` cases for `[0,0] / [0,5]` and `[0,0] / [0,0]` to
`test_div`, covering both a zero numerator and a singleton-zero divisor.
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -861,19 +863,9 @@ impl Interval {
else if lhs_ref.contains(&zero_point)? == Self::TRUE
&& !dt.is_unsigned_integer()
{
- Ok(div_helper_lhs_zero_inclusive(
- &dt,
- lhs_ref,
- rhs_ref,
- &zero_point,
- ))
+ Ok(div_helper_lhs_zero_inclusive(&dt, lhs_ref, rhs_ref, &zero))
} else {
- Ok(div_helper_zero_exclusive(
- &dt,
- lhs_ref,
- rhs_ref,
- &zero_point,
- ))
+ Ok(div_helper_zero_exclusive(&dt, lhs_ref, rhs_ref, &zero))
Review Comment:
Thanks! I tested both examples:
| Division | main | This PR | With endpoint shrinking |
|---|---|---|---|
| `[-3,-1] / [-3,0]` | `[1,+inf)` | `[0,+inf)` | `[0,3]` |
| `[-6,6] / [-3,0]` | `[2,-2]` (inverted) | `(-inf,+inf)` | `[-6,6]` |
This PR fixes incorrect bounds: the first case on main excludes valid zero
quotients, and the second produces an inverted interval.
Agreed, this would tighten the bounds. Would you be okay with handling it in
a follow-up, keeping this PR focused on the bug fix?
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -1693,11 +1685,13 @@ fn div_helper_zero_exclusive(
dt: &DataType,
lhs: &Interval,
rhs: &Interval,
- zero_point: &Interval,
+ zero: &ScalarValue,
) -> Interval {
+ // An interval ending at zero is non-positive. Compare against zero itself,
Review Comment:
Replaced the historical explanation with a shared note in `div()`: intervals
with a finite upper bound <= 0 are treated as non-positive. This documents the
rule used by both helpers.
--
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]
