andygrove commented on PR #66:
URL: https://github.com/apache/datafusion-spark/pull/66#issuecomment-6824112714
Thanks for the PR. The semantics look right to me: casting both sides to
`Float64`, nulling zero divisors (including `-0.0`) before the kernel runs, and
raising in ANSI mode only for non-NULL dividends. Reusing `is_zero` from
`modulus.rs` is a nice touch. A few suggestions, none of them blocking.
**1. Document the `sql` feature.** Without `sql`, `SELECT 1 / 2` still
returns `0`. The Cargo feature comment, crate docs or README should say that
Spark-correct `/` needs `features = ["sql"]`. It's also worth noting that this
only applies to SQL parsing. A DataFrame `col("a") / col("b")` still uses
DataFusion's operator.
**2. Cache the UDF.** `plan_binary_op` creates a new `ScalarUDF` for every
`/` it plans. The other functions expose a shared instance (for example
`date_part()` and `substring()`). A `LazyLock<Arc<ScalarUDF>>` would be
consistent and avoid the repeated allocation.
**3. Test coverage.** The slt cases only use integer or literal inputs, and
the unit tests call `spark_divide` directly with `Float64` arrays. These are
missing:
- a Float32 or mixed-type column case
- a `NULL` dividend with a zero divisor at the SQL level, in both modes
- a check that decimal division is unchanged, for example
`arrow_typeof(CAST(1 AS DECIMAL(10,2)) / 2)`
- a check that intervals are left alone
The description says decimals and intervals keep their behavior, but nothing
pins it.
**4. Edge cases (optional).** Operands that fail `is_numeric()` fall through
to the default operator. That includes a `NULL` literal and string operands,
such as `1 / NULL` or `'4' / 2`, which Spark divides as DOUBLE. This is
probably fine for now, but a code comment or follow-up issue would help.
**Nits**
- The unit test `divides_decimal_digits_as_double` has a misleading name. It
divides `Float64` values and has nothing to do with decimals.
- The function is named `spark_divide`, but the error messages say `divide
expects exactly two arguments`.
--
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]
