comphead commented on PR #22134:
URL: https://github.com/apache/datafusion/pull/22134#issuecomment-5111426508
I made an initial review with assistance of LLM and it comes up with some
high severity issues that needed to be checked
### `LEAD ... IGNORE NULLS` with expression default returns default for
every row
`datafusion/functions-window/src/lead_lag.rs`, new
`evaluate_all_with_ignore_null_and_array_default` (diff lines 220-262).
`shift_offset` for `LEAD` is negative `i64`. Both arms do
`pos.checked_add(offset as usize)`. Casting a negative `i64` to `usize` wraps
to `usize::MAX`, `checked_add` overflows to `None`, and every row falls
back to `default_values[id]`.
The `Err(pos)` arm is also off-by-one for `LEAD` independent of the sign
bug: it needs `pos + k - 1` (the null's insertion point already points at the
first at-or-after valid), while `Ok(pos)` needs `pos +
k`.
```sql
SELECT lead(a, 1, b) IGNORE NULLS OVER (ORDER BY id)
FROM (VALUES (1,1,100),(2,NULL,200),(3,3,300)) AS t(id,a,b);
-- returns [100, 200, 300] instead of [3, 3, 300]
```
---
### `.unwrap()` in `WindowUDFImpl::expressions()` can panic
`datafusion/functions-window/src/lead_lag.rs`, diff lines 112-117.
```rust
let main_expr = parse_expr(...).unwrap();
let main_expr_nullable =
expr_args.input_fields().first().unwrap().data_type().is_null();
```
`parse_expr` returns `Result` and `.first()` returns `Option`. A caller
with empty `input_fields` (optimizer rule rebuilding the plan) panics the
executor thread.
---
### `expressions()` materializes literal defaults on every batch
`datafusion/functions-window/src/lead_lag.rs`, diff lines 118-141.
`expressions()` pushes `input_exprs[2]` (or a `CastExpr` wrapping it)
regardless of whether it is a `Literal`. Downstream
`evaluate_expressions_to_arrays` calls `into_array_of_size(num_rows)` for every
expression. In `evaluate_all`, the `DefaultValue::Literal` arm then
ignores `values[1]`.
`lag(x, 1, 0)` over a 10M-row partition allocates an extra 10M-element
array on every batch and discards it. Regression on the most common form of
`LAG`/`LEAD`.
---
### Silent fallbacks mask invariant breaks
`datafusion/functions-window/src/lead_lag.rs`, diff lines 305-316 and
353-356.
```rust
// evaluate_all
let default_array = values.get(1).cloned()
.unwrap_or_else(||
Arc::new(arrow::array::NullArray::new(value.len())));
// evaluate
values.get(1).map(|defaults| { ... })
.unwrap_or_else(|| ScalarValue::try_from(array.data_type()))
```
Both paths silently emit NULLs when `DefaultValue::Expression` disagrees
with `values.len()`. Wrong answer instead of an error.
---
### Test coverage gaps
`datafusion/sqllogictest/test_files/window.slt`, diff lines 415-742.
Missing cases:
- `LAG`/`LEAD ... IGNORE NULLS` with a per-row expression default (would
immediately expose finding #1).
- Per-row default that is `NULL` for some rows.
- Main and default with different `DataType`s (exercises `CastExpr`
insertion at diff line 134-138).
- Reversed execution (`ORDER BY <col> DESC`).
- Empty partition.
--
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]
