Skip to content

fix: preserve null treatment when unparsing window and aggregate functions #25475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kosiew merged 1 commit into from
Oct 9, 2026
Merged

fix: preserve null treatment when unparsing window and aggregate functions #25475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 119 additions & 5 deletions datafusion/sql/src/unparser/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

use datafusion_common::datatype::DataTypeExt;
use datafusion_expr::expr::{
AggregateFunctionParams, HigherOrderFunction, WindowFunctionParams,
AggregateFunctionParams, HigherOrderFunction, NullTreatment, WindowFunctionParams,
};
use datafusion_expr::expr::{Lambda, Unnest};
use sqlparser::ast::Value::SingleQuotedString;
Expand Down Expand Up @@ -92,6 +92,13 @@ const LOWEST: &BinaryOperator = &BinaryOperator::Or;
// (https://www.postgresql.org/docs/7.2/sql-precedence.html)
const IS: &BinaryOperator = &BinaryOperator::BitwiseAnd;

fn null_treatment_to_sql(null_treatment: NullTreatment) -> ast::NullTreatment {
match null_treatment {
NullTreatment::IgnoreNulls => ast::NullTreatment::IgnoreNulls,
NullTreatment::RespectNulls => ast::NullTreatment::RespectNulls,
}
}

impl Unparser<'_> {
pub fn expr_to_sql(&self, expr: &Expr) -> Result<ast::Expr> {
// Unparsing recurses once per nesting level. The function-argument and
Expand Down Expand Up @@ -276,7 +283,7 @@ impl Unparser<'_> {
window_frame,
filter,
distinct,
..
null_treatment,
},
} = window_fun.as_ref();
let func_name = fun.name();
Expand Down Expand Up @@ -343,7 +350,7 @@ impl Unparser<'_> {
.as_ref()
.map(|f| self.expr_to_sql_inner(f).map(Box::new))
.transpose()?,
null_treatment: None,
null_treatment: null_treatment.map(null_treatment_to_sql),
over,
within_group: vec![],
parameters: ast::FunctionArguments::None,
Expand Down Expand Up @@ -405,7 +412,7 @@ impl Unparser<'_> {
args,
filter,
order_by,
..
null_treatment,
} = &agg.params;

// An aggregate's `order_by` is spelled one of two ways in SQL:
Expand Down Expand Up @@ -449,7 +456,7 @@ impl Unparser<'_> {
clauses,
}),
filter,
null_treatment: None,
null_treatment: null_treatment.map(null_treatment_to_sql),
over: None,
within_group,
parameters: ast::FunctionArguments::None,
Expand Down Expand Up @@ -3966,4 +3973,111 @@ mod tests {
"NULL"
);
}

#[test]
fn test_unparse_null_treatment_window_and_aggregate() -> Result<()> {
use datafusion_expr::expr::NullTreatment;
use datafusion_functions_aggregate::first_last::first_value_udaf;
use datafusion_functions_window::nth_value::first_value_udwf;

// Window function with IgnoreNulls
let window_ignore = Expr::from(WindowFunction {
fun: WindowFunctionDefinition::WindowUDF(first_value_udwf()),
params: WindowFunctionParams {
args: vec![col("a")],
partition_by: vec![],
order_by: vec![Sort::new(col("b"), true, true)],
window_frame: WindowFrame::new(None),
null_treatment: Some(NullTreatment::IgnoreNulls),
distinct: false,
filter: None,
},
});

// Window function with RespectNulls
let window_respect = Expr::from(WindowFunction {
fun: WindowFunctionDefinition::WindowUDF(first_value_udwf()),
params: WindowFunctionParams {
args: vec![col("a")],
partition_by: vec![],
order_by: vec![Sort::new(col("b"), true, true)],
window_frame: WindowFrame::new(None),
null_treatment: Some(NullTreatment::RespectNulls),
distinct: false,
filter: None,
},
});

// Window function with None (Control)
let window_none = Expr::from(WindowFunction {
fun: WindowFunctionDefinition::WindowUDF(first_value_udwf()),
params: WindowFunctionParams {
args: vec![col("a")],
partition_by: vec![],
order_by: vec![Sort::new(col("b"), true, true)],
window_frame: WindowFrame::new(None),
null_treatment: None,
distinct: false,
filter: None,
},
});

// Aggregate function with IgnoreNulls
let agg_ignore =
Expr::AggregateFunction(datafusion_expr::expr::AggregateFunction::new_udf(
first_value_udaf(),
vec![col("a")],
false,
None,
vec![],
Some(NullTreatment::IgnoreNulls),
));

// Aggregate function with RespectNulls
let agg_respect =
Expr::AggregateFunction(datafusion_expr::expr::AggregateFunction::new_udf(
first_value_udaf(),
vec![col("a")],
false,
None,
vec![],
Some(NullTreatment::RespectNulls),
));

// Aggregate function with None (Control)
let agg_none =
Expr::AggregateFunction(datafusion_expr::expr::AggregateFunction::new_udf(
first_value_udaf(),
vec![col("a")],
false,
None,
vec![],
None,
));

let actual_window_ignore = expr_to_sql(&window_ignore)?.to_string();
let actual_window_respect = expr_to_sql(&window_respect)?.to_string();
let actual_window_none = expr_to_sql(&window_none)?.to_string();
let actual_agg_ignore = expr_to_sql(&agg_ignore)?.to_string();
let actual_agg_respect = expr_to_sql(&agg_respect)?.to_string();
let actual_agg_none = expr_to_sql(&agg_none)?.to_string();

assert_eq!(
actual_window_ignore,
"first_value(a) IGNORE NULLS OVER (ORDER BY b ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)"
);
assert_eq!(
actual_window_respect,
"first_value(a) RESPECT NULLS OVER (ORDER BY b ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)"
);
assert_eq!(
actual_window_none,
"first_value(a) OVER (ORDER BY b ASC NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING)"
);

@kosiew kosiew Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert_eq!(actual_agg_ignore, "first_value(a) IGNORE NULLS");
assert_eq!(actual_agg_respect, "first_value(a) RESPECT NULLS");
assert_eq!(actual_agg_none, "first_value(a)");

Ok(())
}
}
Loading