0lai0 opened a new pull request, #5367:
URL: https://github.com/apache/datafusion-comet/pull/5367
## Which issue does this PR close?
Part of #5091.
## Rationale for this change
Each site had a hand-rolled per-row loop that an existing Arrow kernel
already covers. Behaviour is unchanged and the kernel versions are faster on
the shapes measured.
## What changes are included in this PR?
| File | Before | After |
| --- | --- | --- |
| `conversion_funcs/numeric.rs::spark_cast_decimal_to_boolean` |
`BooleanBuilder` loop of `value.is_zero()` | `neq` against a `Scalar` decimal
zero at the source `(precision, scale)` |
| `array_funcs/array_insert.rs::ArrayInsert::evaluate` | Two
`(0..num_rows).map(is_valid).collect()` loops materialising `BooleanArray`s |
`is_not_null(src)` and `and(evaluate_pos, is_not_null(pos))` |
| `math_funcs/pow.rs::spark_pow` | 4-way `match` on array/scalar shapes with
per-row iterators | `apply` from `datafusion::physical_expr_common::datum`,
dispatching to `binary` and `unary`. Null-scalar short-circuit kept explicit
because `unary` only preserves the input array's null buffer. |
`spark_cast_decimal_to_boolean` also plumbs the source `(precision, scale)`
through the zero scalar so `Decimal128(38, 0)` compares against a
matching-scale zero.
### Also included
Three criterion benches under `native/spark-expr/benches/` wired in
`Cargo.toml`.
### Attempted and reverted
**`temporal.rs::days_to_date` → `Date32Type::to_naive_date_opt`.**
`date_trunc` regressed +15–29% on three of four shapes because
`chrono::NaiveDate::from_ymd_opt(1970, 1, 1).unwrap()` is not `const`, so the
epoch is reconstructed per row. The old `const i32` offset let LLVM fold it.
Re-runnable once upstream makes the epoch `const`.
**`covariance.rs::{update_batch,retract_batch}` → `and(is_not_null(a),
is_not_null(b))` + `filter`.** Would have matched `CorrelationAccumulator`, but
`filter` allocates two new `Float64Array`s per batch and regressed sparse-null
shapes by +18–33%. The no-null path was −12%; not enough to justify a per-batch
heuristic.
## How are these changes tested?
Existing tests in each file pass unchanged. Three tests were added for the
paths the refactors newly reach:
- `numeric.rs`: `test_spark_cast_decimal_to_boolean` extended with
`Decimal128(38, 0)`, pinning the zero-scalar precision/scale wiring.
- `array_insert.rs`: `test_array_insert_evaluate_cross_null_patterns` drives
`ArrayInsert::evaluate` with four rows (src NULL, pos NULL, item NULL,
all-non-null), pinning the Spark evaluation-order contract.
- `pow.rs`: `test_spark_pow_null_scalar` covers the null-scalar
short-circuit.
SQL-level coverage already exists in `pow.sql`,
`cast_decimal_to_primitive.sql`, and `array_insert*.sql`.
### Benchmarks
Baseline captured on `main`'s versions of the three files (`git stash push
-- <files>`, `cargo bench --save-baseline main`), refactors restored, `cargo
bench --baseline main` re-run on the same machine. 8192 rows per shape. All `p
< 0.05`.
#### `spark_cast_decimal_to_boolean`
| shape | before | after | change |
| --- | --- | --- | --- |
| no nulls | 16.89 µs | 1.95 µs | −88.5% |
| sparse nulls | 18.24 µs | 1.91 µs | −89.5% |
| dense nulls | 18.39 µs | 1.92 µs | −89.6% |
#### `ArrayInsert::evaluate`
| shape | before | after | change |
| --- | --- | --- | --- |
| no nulls | 168.3 µs | 125.5 µs | −25.2% |
| sparse src nulls | 281.5 µs | 238.2 µs | −14.6% |
| dense src nulls | 216.7 µs | 187.5 µs | −13.3% |
| mixed src+pos nulls | 293.8 µs | 243.6 µs | −16.9% |
Gain comes from dropping the two `BooleanArray::from(Vec<bool>)` allocations
per batch.
#### `spark_pow`
| shape | before | after | change |
| --- | --- | --- | --- |
| array/array no nulls | 39.17 µs | 27.62 µs | −30.1% |
| array/array sparse nulls | 44.56 µs | 26.88 µs | −38.7% |
| array/array dense nulls | 32.91 µs | 16.64 µs | −50.0% |
| scalar/array no nulls | 42.35 µs | 34.21 µs | −18.5% |
| scalar/array sparse nulls | 43.66 µs | 30.96 µs | −29.4% |
| scalar/array dense nulls | 29.58 µs | 20.26 µs | −31.7% |
| array/scalar no nulls | 34.93 µs | 25.35 µs | −27.3% |
| array/scalar sparse nulls | 39.04 µs | 25.83 µs | −34.4% |
| array/scalar dense nulls | 28.38 µs | 17.02 µs | −40.4% |
| null scalar short-circuit | 1.46 µs | 1.39 µs | −5.0% |
Dense-null shapes gain the most because `unary`/`binary` skip null slots
that the old `iter().zip().map().collect()` still visited via `Option` matching.
--
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]
