Unparser: Support `ORDER BY` in window function definition #10370

yyy1000 · 2024-05-03T16:50:09Z

Which issue does this PR close?

Closes #10256 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

yyy1000 · 2024-05-03T16:56:20Z

datafusion/sql/src/unparser/expr.rs

                    null_treatment: None,
                }),
-                r#"COUNT(*) OVER (RANGE BETWEEN 6 PRECEDING AND 2 FOLLOWING)"#,
+                r#"COUNT(* ORDER BY "a" DESC NULLS FIRST) OVER (RANGE BETWEEN 6 PRECEDING AND 2 FOLLOWING)"#,


This is a bit out of my expectation. I don't know whether the two expressions are the same.🤔
From

datafusion/datafusion/core/tests/dataframe/mod.rs

Lines 171 to 194 in 8190cb9

let sql_results = ctx

.sql("select COUNT(*) OVER(ORDER BY a DESC RANGE BETWEEN 6 PRECEDING AND 2 FOLLOWING) from t1")

.await?

.explain(false, false)?

.collect()

.await?;

let df_results = ctx

.table("t1")

.await?

.select(vec![Expr::WindowFunction(expr::WindowFunction::new(

WindowFunctionDefinition::AggregateFunction(AggregateFunction::Count),

vec![wildcard()],

vec![],

vec![Expr::Sort(Sort::new(Box::new(col("a")), false, true))],

WindowFrame::new_bounds(

WindowFrameUnits::Range,

WindowFrameBound::Preceding(ScalarValue::UInt32(Some(6))),

WindowFrameBound::Following(ScalarValue::UInt32(Some(2))),

),

None,

))])?

.explain(false, false)?

.collect()

.await?;

,
it should be COUNT(*) OVER(ORDER BY a DESC RANGE BETWEEN 6 PRECEDING AND 2 FOLLOWING),
I doubt it should be an issue from the fmt for Function in sql-parser, if there is an over and order_by, it should put the order by in Over https://github.com/sqlparser-rs/sqlparser-rs/blob/71a7262e38e1c10a46fd50ea7c5610091e1aca3c/src/ast/mod.rs#L4869-L4904

It looks like somehow the ordering was assciated with the aggregate function rather than the window frame -- see comment above

Oh, got it!

alamb

Thank you @yyy1000 for this contribution -- this looks pretty sweet indeed.

I left a few comments but I think it is quite close

cc @devinjdangelo

datafusion/sql/src/unparser/expr.rs

alamb · 2024-05-03T20:34:45Z

datafusion/sql/src/unparser/expr.rs

                asc: _,
                nulls_first: _,
-            }) => self.expr_to_sql(expr),
+            }) => internal_err!("Sort expression should be handled by expr_to_unparsed"),


Since this is a public API I don't think this should be an internal_err as it isn't a bug in datafusion (it could be a bug in the input that was passed in)-- perhaps we can make it a plan_err?

Ohh, great. Now I know better when to use these types of error.

alamb · 2024-05-03T20:36:27Z

datafusion/sql/src/unparser/expr.rs

+}
+
+impl Unparsed {
+    fn into_order_by_expr(self) -> Result<ast::OrderByExpr> {


Maybe we can call this try_into_order_by_expr to reflect the fact it is fallable

Perhaps it should also be marked pub 🤔

alamb · 2024-05-03T20:38:32Z

datafusion/sql/src/unparser/expr.rs

+                let order_by_expr_vec: Vec<ast::OrderByExpr> = order_by
+                    .iter()
+                    .flat_map(|expr| expr_to_unparsed(expr)?.into_order_by_expr())
+                    .collect();


I think this should be just map (flat map I think will discard Result`s 🤔

Suggested change

let order_by_expr_vec: Vec<ast::OrderByExpr> = order_by

.iter()

.flat_map(|expr| expr_to_unparsed(expr)?.into_order_by_expr())

.collect();

let order_by: Vec<ast::OrderByExpr> = order_by

.iter()

.map(|expr| expr_to_unparsed(expr)?.into_order_by_expr())

.collect()?;

datafusion/sql/src/unparser/expr.rs

alamb · 2024-05-03T20:40:41Z

datafusion/sql/src/unparser/expr.rs

                    null_treatment: None,
                }),
-                r#"COUNT(*) OVER (RANGE BETWEEN 6 PRECEDING AND 2 FOLLOWING)"#,
+                r#"COUNT(* ORDER BY "a" DESC NULLS FIRST) OVER (RANGE BETWEEN 6 PRECEDING AND 2 FOLLOWING)"#,


It looks like somehow the ordering was assciated with the aggregate function rather than the window frame -- see comment above

yyy1000 · 2024-05-03T23:51:18Z

Thanks for your review! @alamb
I resolved your comment and you can review it again when you are available. :)

alamb

This is great -- thank you @yyy1000 🚀

alamb · 2024-05-04T11:13:30Z

datafusion/sql/src/unparser/expr.rs


+/// Convert a DataFusion [`Expr`] to [`Unparsed`]
+///
+/// This function is similar to expr_to_sql, but it supports converting more [`Expr`] types like


alamb · 2024-05-04T11:13:47Z

datafusion/sql/src/unparser/expr.rs

                        .map(|e| self.expr_to_sql(e))
                        .collect::<Result<Vec<_>>>()?,
-                    order_by: vec![],
+                    order_by,


support OrderBy in Unparser

e1c5b7c

github-actions bot added the sql SQL Planner label May 3, 2024

yyy1000 commented May 3, 2024

View reviewed changes

alamb reviewed May 3, 2024

View reviewed changes

resolve comment

06ea156

alamb approved these changes May 4, 2024

View reviewed changes

alamb changed the title ~~Support OrderByExpr in Unparsed~~ Unparser: Support ORDER BY in window function definition May 4, 2024

alamb merged commit 5d3bbaa into apache:main May 4, 2024

yyy1000 deleted the orderby branch May 4, 2024 15:23

	let sql_results = ctx
	.sql("select COUNT(*) OVER(ORDER BY a DESC RANGE BETWEEN 6 PRECEDING AND 2 FOLLOWING) from t1")
	.await?
	.explain(false, false)?
	.collect()
	.await?;
	let df_results = ctx
	.table("t1")
	.await?
	.select(vec![Expr::WindowFunction(expr::WindowFunction::new(
	WindowFunctionDefinition::AggregateFunction(AggregateFunction::Count),
	vec![wildcard()],
	vec![],
	vec![Expr::Sort(Sort::new(Box::new(col("a")), false, true))],
	WindowFrame::new_bounds(
	WindowFrameUnits::Range,
	WindowFrameBound::Preceding(ScalarValue::UInt32(Some(6))),
	WindowFrameBound::Following(ScalarValue::UInt32(Some(2))),
	),
	None,
	))])?
	.explain(false, false)?
	.collect()
	.await?;

Unparser: Support ORDER BY in window function definition #10370

Unparser: Support ORDER BY in window function definition #10370

Uh oh!

Conversation

yyy1000 commented May 3, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyy1000 commented May 3, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Unparser: Support `ORDER BY` in window function definition #10370

Unparser: Support `ORDER BY` in window function definition #10370