andygrove commented on PR #5081:
URL:
https://github.com/apache/datafusion-comet/pull/5081#issuecomment-5133832236
Thanks @comphead. Items 2 and 3 were real gaps and are fixed in 63f174b7.
Item 1 was written against the previous revision — details below.
### 1. Branchy scalar pre-scan — no longer present, but there is a related
finding
`scan_float_array` and the pre-scan it belonged to were removed in 6c815fea
(addressing @mbutrovich's earlier comment), before this review landed. The
current code has no pre-scan at all: it computes the remainder with
`try_binary` + `ArrowNativeTypeOp::mod_checked`, so the zero check rides along
inside the single pass that produces the result.
Your underlying concern is still worth measuring though, so I benchmarked
all three strategies on 8192 `f64` rows with **no** zero divisors (the common
case you called out), against the infallible `rem` kernel as baseline:
| strategy | no nulls | 1-in-17 nulls |
|---|---|---|
| `rem` (non-ANSI baseline) | 63.1 µs | 57.8 µs |
| `try_binary` + `mod_checked` (current) | 65.1 µs (+3%) | 72.1 µs
(**+25%**) |
| `eq` pre-scan + `rem` (your suggestion) | 62.6 µs (~0%) | 60.2 µs (+4%) |
Two things fall out of this:
- The absolute cost is ~7.7 ns/row, i.e. ~25 cycles. Float remainder is
`fmod`-bound, not throughput-bound — `mod_wrapping` compiles to a libm call per
element, so it never vectorized in the first place. That's why the branch in
`mod_checked` costs only 3% when there are no nulls, rather than the ~2x a
blocked-autovectorization argument would predict.
- **You're right about the null case.** When either operand has nulls,
`try_binary` takes its `try_for_each_valid_idx` path (arrow-arith
`arity.rs:283`), which walks valid indices through the bitmap one at a time
instead of computing straight through and applying the null buffer afterwards.
That's the +25%.
So there's a genuine ~25% regression on ANSI float modulo over nullable
columns, and your `eq` + `count_set_bits` suggestion does avoid it. I have not
switched to it, because it directly reverses the change @mbutrovich asked for
and re-adds the dividend-validity intersection logic that removal simplified
away — I would rather you two agree on the tradeoff than have me flip-flop the
implementation between reviews. Happy to make the switch immediately if you
want the null-case regression closed; the measurement above is the whole
argument for it.
(I did not commit the benchmark — it was a throwaway. Glad to add a
permanent `modulo` bench if that would be useful, since there isn't one today.)
### 2. NaN / Inf / 0.0 dividend with a zero divisor — fixed
Good catch, and the reasoning checks out in Spark 4.1's `DivModLike.eval`
(`arithmetic.scala:636`):
```scala
val input2 = right.eval(input)
if (input2 == null || (!failOnError && isZero(input2))) null
else {
val input1 = left.eval(input)
if (input1 == null) null
else { if (isZero(input2)) throw
QueryExecutionErrors.divideByZeroError(...) ... }
}
```
`isZero` only ever looks at the divisor, so any non-null dividend — NaN,
±Infinity, ±0.0 — throws. Added, with one adjustment to your examples: `SELECT
double('NaN') % 0.0` is fully literal and gets constant-folded before it
reaches Comet, so I kept the dividend as a column:
```sql
SELECT a % CAST(0.0 AS DOUBLE) FROM ansi_float_special WHERE isnan(a)
SELECT a % CAST(0.0 AS DOUBLE) FROM ansi_float_special WHERE a =
double('Infinity')
SELECT a % CAST(0.0 AS DOUBLE) FROM ansi_float_special WHERE a =
double('-Infinity')
SELECT a % CAST(0.0 AS DOUBLE) FROM ansi_float_special WHERE a = 0.0
SELECT a % CAST(2.0 AS DOUBLE) FROM ansi_float_special ORDER BY 1 -- must
not throw
```
Matching Rust tests cover the same dividends for both `Array/Array` and
`Array/Scalar` operands, and I added null-divisor and NaN-divisor cases
confirming neither is mistaken for zero.
### 3. Mixed-row batch — fixed
Agreed, and I used your suggested shape almost verbatim:
```sql
INSERT INTO ansi_float_mixed VALUES (1.0, 2.0), (3.0, 0.0), (NULL, 0.0),
(5.0, 1.5), (NULL, 4.0)
SELECT a % b FROM ansi_float_mixed -- throws,
because of (3.0, 0.0)
SELECT a % b FROM ansi_float_mixed WHERE a IS NULL ORDER BY 1 -- null, must
NOT throw
SELECT a % b FROM ansi_float_mixed WHERE b <> 0.0 ORDER BY 1 -- no zero
divisors at all
```
The middle one is the important negative case: per the `eval` snippet above,
a null dividend returns null even under ANSI mode with a zero divisor, so a
batch-wide `any(b == 0.0)` check would incorrectly raise there.
### Verifying these actually reach the native path
Rather than assume, I isolated each new SQL error case into its own file and
reran with the float branch disabled. All four error cases failed independently
(so each one genuinely exercises the fix), and the `WHERE a IS NULL` case
passed with the branch disabled, which is correct — it's the negative control
and null propagation handles it either way. Re-enabled, all five pass.
Also green: 25 tests in `modulo_expr`, `cargo test -p
datafusion-comet-spark-expr` (559), clippy `-D warnings`, `cargo fmt --check`.
--
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]
