Re: [PR] fix missing window expressions when unparsing plans without outer projections [datafusion]

2026-05-15 Thread via GitHub 


kosiew merged PR #21801:
URL: https://github.com/apache/datafusion/pull/21801


-- 
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]

Re: [PR] fix missing window expressions when unparsing plans without outer projections [datafusion]

2026-05-15 Thread via GitHub 


PiotrSrebrny commented on code in PR #21801:
URL: https://github.com/apache/datafusion/pull/21801#discussion_r3248290566


##
datafusion/sql/tests/cases/plan_to_sql.rs:
##
@@ -2848,6 +2849,253 @@ fn test_unparse_window() -> Result<()> {
 Ok(())
 }
 
+#[test]
+fn test_unparse_window_over_aggregate_without_projection() -> Result<()> {
+let schema = Schema::new(vec![
+Field::new("time", DataType::Int64, false),
+Field::new("value", DataType::Float64, true),
+]);
+let window_expr = Expr::WindowFunction(Box::new(WindowFunction {
+fun: WindowFunctionDefinition::WindowUDF(row_number_udwf()),
+params: WindowFunctionParams {
+args: vec![],
+partition_by: vec![],
+order_by: vec![col("time").sort(true, true)],
+window_frame: WindowFrame::new(None),
+null_treatment: None,
+distinct: false,
+filter: None,
+},
+}))
+.alias("row_idx");
+let plan = table_scan(Some("gas"), &schema, None)?
+.aggregate(vec![col("time")], vec![sum(col("value")).alias("sum_n")])?
+.window(vec![window_expr])?

Review Comment:
   Yes, I tested on my side and it fixes the issue. Thank you so much!



-- 
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]

Re: [PR] fix missing window expressions when unparsing plans without outer projections [datafusion]

2026-05-14 Thread via GitHub 


kosiew commented on code in PR #21801:
URL: https://github.com/apache/datafusion/pull/21801#discussion_r3245642125


##
datafusion/sql/tests/cases/plan_to_sql.rs:
##
@@ -2848,6 +2849,253 @@ fn test_unparse_window() -> Result<()> {
 Ok(())
 }
 
+#[test]
+fn test_unparse_window_over_aggregate_without_projection() -> Result<()> {
+let schema = Schema::new(vec![
+Field::new("time", DataType::Int64, false),
+Field::new("value", DataType::Float64, true),
+]);
+let window_expr = Expr::WindowFunction(Box::new(WindowFunction {
+fun: WindowFunctionDefinition::WindowUDF(row_number_udwf()),
+params: WindowFunctionParams {
+args: vec![],
+partition_by: vec![],
+order_by: vec![col("time").sort(true, true)],
+window_frame: WindowFrame::new(None),
+null_treatment: None,
+distinct: false,
+filter: None,
+},
+}))
+.alias("row_idx");
+let plan = table_scan(Some("gas"), &schema, None)?
+.aggregate(vec![col("time")], vec![sum(col("value")).alias("sum_n")])?
+.window(vec![window_expr])?

Review Comment:
   @PiotrSrebrny 
   Does 
https://github.com/apache/datafusion/pull/21801/commits/5e8b64ac32c7f7810dd01d69434b310035f54616
 fix your issue?



-- 
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]

Re: [PR] fix missing window expressions when unparsing plans without outer projections [datafusion]

2026-05-14 Thread via GitHub 


nathanb9 commented on code in PR #21801:
URL: https://github.com/apache/datafusion/pull/21801#discussion_r3245109874


##
datafusion/sql/tests/cases/plan_to_sql.rs:
##
@@ -2848,6 +2849,253 @@ fn test_unparse_window() -> Result<()> {
 Ok(())
 }
 
+#[test]
+fn test_unparse_window_over_aggregate_without_projection() -> Result<()> {
+let schema = Schema::new(vec![
+Field::new("time", DataType::Int64, false),
+Field::new("value", DataType::Float64, true),
+]);
+let window_expr = Expr::WindowFunction(Box::new(WindowFunction {
+fun: WindowFunctionDefinition::WindowUDF(row_number_udwf()),
+params: WindowFunctionParams {
+args: vec![],
+partition_by: vec![],
+order_by: vec![col("time").sort(true, true)],
+window_frame: WindowFrame::new(None),
+null_treatment: None,
+distinct: false,
+filter: None,
+},
+}))
+.alias("row_idx");
+let plan = table_scan(Some("gas"), &schema, None)?
+.aggregate(vec![col("time")], vec![sum(col("value")).alias("sum_n")])?
+.window(vec![window_expr])?

Review Comment:
   yea makes sense just fixed:
   * For Filter nodes, the unparser now checks for windows first when the 
dialect supports QUALIFY.
   * If there is also an aggregate underneath, aggregate aliases inside the 
QUALIFY expression are still unprojected correctly.



-- 
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]

Re: [PR] fix missing window expressions when unparsing plans without outer projections [datafusion]

2026-05-12 Thread via GitHub 


PiotrSrebrny commented on code in PR #21801:
URL: https://github.com/apache/datafusion/pull/21801#discussion_r3226941605


##
datafusion/sql/tests/cases/plan_to_sql.rs:
##
@@ -2848,6 +2849,253 @@ fn test_unparse_window() -> Result<()> {
 Ok(())
 }
 
+#[test]
+fn test_unparse_window_over_aggregate_without_projection() -> Result<()> {
+let schema = Schema::new(vec![
+Field::new("time", DataType::Int64, false),
+Field::new("value", DataType::Float64, true),
+]);
+let window_expr = Expr::WindowFunction(Box::new(WindowFunction {
+fun: WindowFunctionDefinition::WindowUDF(row_number_udwf()),
+params: WindowFunctionParams {
+args: vec![],
+partition_by: vec![],
+order_by: vec![col("time").sort(true, true)],
+window_frame: WindowFrame::new(None),
+null_treatment: None,
+distinct: false,
+filter: None,
+},
+}))
+.alias("row_idx");
+let plan = table_scan(Some("gas"), &schema, None)?
+.aggregate(vec![col("time")], vec![sum(col("value")).alias("sum_n")])?
+.window(vec![window_expr])?

Review Comment:
   I tried to access the row_idx column here in a filter just by inserting this 
expression:
   ```
   .filter(col("row_idx").eq(lit(0i64)))?
   ```
   and the unparser fails with this error: Error: Internal("Tried to unproject 
agg expr for column 'row_idx' that was not found in the provided Aggregate!")
   
   when accessing the "sum_n" column there is no problem. 



-- 
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]