andygrove commented on PR #5367:
URL:
https://github.com/apache/datafusion-comet/pull/5367#issuecomment-5441704828
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
This is a model PR. Per-shape benchmark tables with a stated baseline
method, a section documenting the two refactors that were tried and reverted
with the reason and the numbers, and a test added for each newly reached path.
The `date_trunc` finding about `NaiveDate::from_ymd_opt` not being `const` is a
genuinely useful thing to have written down.
One thing I want to check, plus two smaller notes.
**`spark_cast_decimal_to_boolean` now fails on a non-all-null precision-0
array**
```rust
if decimal_array.null_count() == decimal_array.len() {
return Ok(Arc::new(BooleanArray::new_null(decimal_array.len())));
}
let zero = Scalar::new(
Decimal128Array::from(vec![0i128])
.with_precision_and_scale(decimal_array.precision(),
decimal_array.scale())?,
);
```
The comment says `Decimal128(0, 0)` is reachable through Spark's RDD
row-to-Arrow path, and the all-null fast path handles it. But if such an array
ever contains a non-null value, `with_precision_and_scale(0, 0)` returns `Err`
and the whole cast fails, where the old `BooleanBuilder` loop handled it fine.
Is a non-all-null precision-0 array reachable? Spark's `DecimalType`
requires precision at least 1, so my guess is no and the all-null case is a
degenerate placeholder. If that is right, the comment should say so, because as
written it explains why the fast path exists without saying why the slow path
is safe. If it is not right, the zero scalar needs to be built at a clamped
precision instead.
**`is_not_null` changes the result length**
The old code built a `BooleanArray` of exactly `batch.num_rows()` by
indexing `src_value.is_valid(row)`. `is_not_null(&src_value)` returns an array
of `src_value.len()`. Those are the same whenever the evaluated column is
materialized to the batch length, which I expect is always. But if a scalar
ever came through without expansion, the old code would have produced a
full-length mask and the new one produces a length-1 mask, and the subsequent
`and` would fail with a length mismatch rather than silently misbehaving.
That is arguably an improvement, but it is a behavior change worth a
sentence. Is there a guarantee upstream that `src_array_expr.evaluate` always
yields a batch-length array here?
**One question on `spark_pow`**
The kernel comment says scalar/array uses `unary` so the scalar is not
broadcast, with an explicit null-scalar short-circuit because `unary` preserves
the input array's null buffer rather than the scalar's. What about
array/scalar, with the scalar on the right? The comment only describes the
left-scalar case. If both directions are handled the wording could say so, and
if `test_spark_pow_null_scalar` only covers one direction it would be worth
covering the other.
--
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]
