mbutrovich opened a new issue, #100: URL: https://github.com/apache/datafusion-spark/issues/100
## Why this matters arrow-rs divides an array by a scalar with a precomputed divisor for `Int32`, `Int64`, `UInt32`, and `UInt64` (apache/arrow-rs#12076, tracked in apache/arrow-rs#11421). The kernel picks that path only when the right-hand `Datum` is a scalar. Spark `mod` and `pmod` with a literal divisor, such as `pmod(c, 13)`, don't give it one. ## Where the scalar is lost `spark_mod` and `spark_pmod` call `ColumnarValue::values_to_arrays(args)`, which turns the literal into a full-length array ([`modulus.rs` L90-L91](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/modulus.rs#L90-L91) and [L121](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/modulus.rs#L121)). `try_rem` then builds `safe_right` with `zip(&divisor_is_zero, &null, right)`, another array, and passes it to `rem` ([`modulus.rs` L57-L76](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/modulus.rs#L57-L76)). `pmod` calls `try_rem` twice ([L156-L160](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/modulus.rs#L156-L160)), so both calls miss the scalar path. The arrow-rs side is [`arithmetic_op`](https://github.com/apache/arrow-rs/blob/9c9486188744e80ec86c3b2bcdd649b89945ffe5/arrow-arith/src/numeric.rs#L228-L236), which requires `!l_scalar && r_scalar`. ## Proposed change Keep a scalar divisor a `Scalar` through `try_rem`. Check it for zero once. In legacy mode a zero scalar divisor makes the whole result null. In ANSI mode it raises unless every dividend is null, which matches the per-row rule in `try_rem` today. Pass `Scalar::new(...)` to `rem` for a non-zero divisor. Array divisors keep today's code. ## Overlap with open issues apache/datafusion-spark#27 changes the divide-by-zero error that `try_rem` raises, and apache/datafusion-spark#26 changes how `pmod` coerces its arguments. Both touch the code this change restructures, so the zero check for a scalar divisor should land on top of whichever merges first. ## Checks before a PR - Add sqllogictest cases for a literal divisor of 0, a null literal, -1, a power of two, and a nonzero literal, in legacy and ANSI mode, with and without nulls in the dividend, so the behavior matches the current array path. - Benchmark `invoke_with_args` on an 8192-row `Int32` and `Int64` column with a literal divisor, before and after, with Comet's batch size and build profile. Include a case that has no scalar divisor, because it shows the noise floor. I haven't run a benchmark or a test for this. The finding is from reading the code at the linked commit. -- 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]
