AmirF194 opened a new issue, #25462:
URL: https://github.com/apache/datafusion/issues/25462
### Describe the bug
`Expr::WindowFunction` and `Expr::AggregateFunction` in
`datafusion/sql/src/unparser/expr.rs` both hardcode `null_treatment: None` on
the `ast::Function` they build, even though `WindowFunctionParams` and
`AggregateFunctionParams` (`datafusion/expr/src/expr.rs`) carry a real
`Option<NullTreatment>`. So a call like `LAST_VALUE(v IGNORE NULLS) OVER (...)`
round-trips through `unparse()` / `transform_sql()` into `LAST_VALUE(v) OVER
(...)`, silently, with no error. It runs fine and can return a different result
whenever the ordered column has NULLs, since the two forms mean different
things.
Checked both spots:
- window arm: the `WindowFunctionParams` destructure drops `null_treatment`
with `..`, then the built `Function` sets `null_treatment: None` directly.
- aggregate arm: same pattern, same field name.
Confirmed on current `main` and on the `53.0.0` tag. `git log -G` on the
line shows it was never wired up, not regressed.
### To reproduce
```rust
// datafusion/sql/src/unparser/expr.rs, inside `mod tests`
#[test]
fn null_treatment_lost_on_unparse() -> Result<()> {
let dialect: Arc<dyn Dialect> =
Arc::new(CustomDialectBuilder::new().build());
let unparser = Unparser::new(dialect.as_ref());
let func = WindowFunctionDefinition::WindowUDF(
datafusion_functions_window::nth_value::last_value_udwf(),
);
let mut window_func = WindowFunction::new(func, vec![col("a")]);
window_func.params.order_by = vec![Sort::new(col("b"), true, true)];
window_func.params.null_treatment = Some(NullTreatment::IgnoreNulls);
let ast = unparser.expr_to_sql(&Expr::from(window_func))?;
println!("{}", ast.to_string());
Ok(())
}
```
Ran with `cargo test -p datafusion-sql --features unparser
null_treatment_lost_on_unparse` (test binary flag `--nocapture`) at HEAD
`3b16a3d0eef765b81ce5aad20997b193a42ad02a`. Printed:
```
last_value(a) OVER (ORDER BY b ASC NULLS FIRST ROWS BETWEEN UNBOUNDED
PRECEDING AND UNBOUNDED FOLLOWING)
```
### Expected behavior
The unparsed SQL should keep the null treatment, something like
`last_value(a) IGNORE NULLS OVER (...)`.
### Additional context
Same shape on the aggregate side (`Expr::AggregateFunction` arm, same file).
Fix looks like a small one: read `null_treatment` out of the destructure
instead of dropping it with `..`, map DataFusion's `NullTreatment` to
`sqlparser::ast::NullTreatment`, and pass that through instead of the hardcoded
`None`, in both arms. Happy to send a PR for this if it's useful.
--
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]
