fix: preserve null treatment when unparsing window and aggregate functions

[Shoryamishra61]

Which issue does this PR close?

• Closes SQL unparser drops IGNORE NULLS / RESPECT NULLS on window and aggregate functions #25462

Rationale for this change

When converting DataFusion logical expressions back to SQL AST in datafusion-sql, both Expr::WindowFunction and Expr::AggregateFunction previously dropped the null_treatment field stored in WindowFunctionParams and AggregateFunctionParams and hardcoded null_treatment: None on the generated sqlparser::ast::Function.

As a result, expressions using IGNORE NULLS or RESPECT NULLS (such as FIRST_VALUE(v IGNORE NULLS) OVER (...) or aggregate FIRST_VALUE(v IGNORE NULLS)) lost their explicit NULL treatment clause during unparsing, altering query semantics upon re-execution.

What changes are included in this PR?

• Extracted null_treatment from WindowFunctionParams in Expr::WindowFunction and forwarded it to ast::Function.null_treatment.
• Extracted null_treatment from AggregateFunctionParams in Expr::AggregateFunction and forwarded it to ast::Function.null_treatment.
• Added helper function null_treatment_to_sql mapping datafusion_expr::expr::NullTreatment to sqlparser::ast::NullTreatment.
• Added unit tests covering IGNORE NULLS, RESPECT NULLS, and None (control) for both window and aggregate functions.

What is the testing strategy for this PR?

• Added test_unparse_null_treatment_window_and_aggregate in datafusion/sql/src/unparser/expr.rs covering:
  • Window functions with IGNORE NULLS
  • Window functions with RESPECT NULLS
  • Window functions without explicit NULL treatment (control case ensuring no clause is emitted)
  • Aggregate functions with IGNORE NULLS
  • Aggregate functions with RESPECT NULLS
  • Aggregate functions without explicit NULL treatment (control case)
• Verified test fails on pre-fix code and passes with the fix.
• Ran all unparser unit tests: cargo test -p datafusion-sql --lib unparser (38 passed, 0 failed).
• Verified lints and formatting: cargo fmt --all -- --check and cargo clippy -p datafusion-sql --all-targets --all-features -- -D warnings.

Are there any user-facing changes?

Bug fix: SQL unparsing now preserves explicit IGNORE NULLS and RESPECT NULLS clauses on window and aggregate functions rather than omitting them.

[Copilot]

🟢 Approval recommended

The change correctly forwards existing logical-expression null-treatment into the SQL AST and includes focused unit tests that cover both window and aggregate cases.

Pull request overview

Fixes a SQL round-trip correctness bug in datafusion-sql where explicit IGNORE NULLS / RESPECT NULLS clauses on window and aggregate functions were dropped during expression → SQL AST unparsing, potentially changing query semantics when re-executed.

Changes:

• Plumbs null_treatment from WindowFunctionParams into sqlparser::ast::Function.null_treatment.
• Plumbs null_treatment from AggregateFunctionParams into sqlparser::ast::Function.null_treatment.
• Adds a null_treatment_to_sql helper plus unit tests validating IGNORE NULLS, RESPECT NULLS, and absence of the clause.

File summaries

File	Description
datafusion/sql/src/unparser/expr.rs	Preserves null-treatment during unparsing for window/aggregate functions and adds targeted regression tests.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.10345% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.73%. Comparing base (b7c7bc2) to head (b69ee3c).
⚠️ Report is 34 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/sql/src/unparser/expr.rs	93.10%	0 Missing and 6 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #25475   +/-   ##
=======================================
  Coverage   82.73%   82.73%           
=======================================
  Files        1147     1147           
  Lines      449213   449298   +85     
  Branches   449213   449298   +85     
=======================================
+ Hits       371634   371706   +72     
- Misses      54929    54931    +2     
- Partials    22650    22661   +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kosiew]

@Shoryamishra61,

Thanks for working on this. The null treatment propagation looks correct, and the current tests cover the mapping fix well. I left one optional suggestion for broader round-trip coverage.

[kosiew]

Optional follow-up: could you add plan-to-SQL round-trip coverage for IGNORE NULLS and RESPECT NULLS on both window and aggregate FIRST_VALUE? It would be useful to include an argument-clause form such as SELECT FIRST_VALUE(id IGNORE NULLS) OVER (ORDER BY id) AS v FROM person, then reparse the emitted SQL text before replanning so the test exercises the full serialization path.

[kosiew]

@Shoryamishra61,

Thanks for fixing this. The change preserves explicit IGNORE NULLS and RESPECT NULLS treatment when unparsing both window and aggregate functions. The shared conversion helper handles both variants, and the regression tests cover explicit and absent NULL treatment.

The implementation addresses the reported issue. Approved.

[kosiew]

🚀
@Shoryamishra61
Thank you for your contribution.

[Shoryamishra61]

Thank you, @kosiew , for the thorough review and approval! I appreciate your feedback and the opportunity to contribute to Apache DataFusion. Glad we could get the NULL treatment handling and regression coverage into good shape. Looking forward to contributing more!