kumarUjjawal commented on code in PR #25985:
URL: https://github.com/apache/datafusion/pull/25985#discussion_r4172092359
##########
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:
When an integer divisor ends at zero, the helpers divide by that 0 endpoint,
and div_bounds turns it into an unbounded side. The bounds stay safe but get
much wider than needed. Since integer division by zero always fails at runtime,
the divisor could be shrunk first ([-3,0] -> [-3,-1], [0,3] -> [1,3]) before
choosing the sign case.
Failure scenario: Int64 [-3,-1] / [-3,0] now gives [0, +inf) instead of [0,
3], and [-6,6] / [-3,0] gives (-inf, +inf) instead of [-6, 6]. Filter
statistics for `a / b > k` then cannot rule out any rows. Shrinking the divisor
once in `div()` would fix the sign choice and keep the bounds tight, with no
special cases in each helper.
##########
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:
The new tests only use Int64, but the change also affects unsigned types. A
UInt lhs of [0,0] now goes to the 'non-positive' match arms, and no test checks
this.
Failure scenario: UInt32 [0,0] / [0,5] used to give [0, +inf) and now gives
[0, 0]. This is correct, but no test checks it, so a later change could break
the unsigned path without any test failing. Please add a few UInt cases, for
example [0,0]/[0,5] and [0,0]/[0,0], to test_div or the exhaustive loop.
##########
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:
The new comment explains the old bug ('not the negative endpoint of the
integer zero neighborhood [-1, 1]') instead of the current rule. It is also
only in one of the two helpers that use the same comparison.
Failure scenario: The comment talks about code that no longer exists, and
div_helper_lhs_zero_inclusive uses the same `rhs.upper <= *zero` check with no
comment. A short shared note, for example 'an interval whose upper bound is <=
0 is treated as non-positive', in both places (or on `div()`) would be clearer.
##########
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:
No exact-bound test covers the changed branch in
div_helper_lhs_zero_inclusive (lhs contains zero, divisor ends at zero). The
exhaustive test only checks that values are contained, so a loose result would
still pass.
Failure scenario: Before this PR, [-6,6] / [-3,0] went to the positive
branch and built an inverted interval [2,-2]. Now it gives an unbounded result.
No test pins this result, so the branch could later get wider, or wrongly move
to another sign arm, without any test noticing. Please add exact cases such as
[-6,6]/[-3,0] and [-6,6]/[0,3] to test_div_zero_endpoints.
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -3674,6 +3668,55 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_div_zero_endpoints() -> Result<()> {
Review Comment:
test_div_zero_endpoints is a separate test with its own loop, but test_div
already has a table of (lhs, rhs, expected) cases checked with assert_eq for
integers.
Failure scenario: Two places now hold exact division expectations, so future
div cases may go into either one. The four new tuples could just be rows in the
test_div `cases` vec, which removes about 15 lines of duplicate loop and assert
code.
--
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]
