Support ORDER BY an aliased column #5067

alamb · 2023-01-25T19:36:20Z

~~Draft:~~

Which issue does this PR close?

Closes #4854

Rationale for this change

Column resolution for ORDER BY expressions are incorrect

What changes are included in this PR?

Make the output rewrite general (they were incorrect for projections above aggregates)
Add comments
Add tests

Are these changes tested?

Yes,

Are there any user-facing changes?

bug fix

alamb · 2023-01-27T18:49:43Z

datafusion/sql/tests/integration_test.rs

@@ -36,6 +36,12 @@ use datafusion_sql::planner::{ContextProvider, ParserOptions, SqlToRel};

 use rstest::rstest;

+#[cfg(test)]


this is to enable debug logging in the sql tests

alamb · 2023-01-27T18:50:38Z

datafusion/expr/src/expr_rewriter/order_by.rs

+/// Remember that:
+/// 1. given a projection with exprs: [a, b + c]
+/// 2. t produces an output schema with two columns "a", "b + c"
+fn rewrite_in_terms_of_projection(expr: Expr, projection: &Projection) -> Result<Expr> {


this is the fix -- it has slightly different semantics than the raw aggregate case.

alamb · 2023-01-27T19:08:54Z

datafusion/expr/src/expr_rewriter/order_by.rs

+/// Note The SQL planner always puts a `Projection` at the output of
+/// an aggregate, the other paths such as LogicalPlanBuilder can
+/// create a Sort directly above an Aggregate
+fn rewrite_in_terms_of_aggregate(


this is the same logic, just extracted into a function and using rewrite_expr to make it easier to read

alamb · 2023-01-27T19:09:39Z

Ok -- I think this PR is ready to review once the other PRs it depends on are ready

alamb · 2023-01-30T21:43:16Z

datafusion/expr/src/expr_rewriter/order_by.rs

-                aggr_expr,
-                distinct_group_exprs: &distinct_group_exprs,
-            })
+/// Rewrites a sort expression in terms of the output of the previous [`LogicalPlan`]


This is the new more general algorithm that works for LogicalPlan::Projection and LogicalPlan::Aggregation

alamb · 2023-01-30T21:43:50Z

datafusion/expr/src/expr_rewriter/order_by.rs

+        // if that doesn't work, try to match the expression as an
+        // output column -- however first it must be "normalized"
+        // (e.g. "c1" --> "t.c1") because that normalization is done
+        // at the input of the aggregate.


I don't fully understand why this is needed, but several tests fail if this logic is removed and the previous logic did it as well

Maybe(Just to offer a possibility) it's related with qualifier.

In the comment:

/// 1. given a projection with exprs: [a, b + c] /// 2. t produces an output schema with two columns "a", "b + c"

But in fact, in the plan, [a, b + c] can be [table1.a, "table2.b+c"].
output schema also may be with qualifier like above.
Due to qualifier, some equations are not true, like 'a' == 't1.a' will be false.

But in fact, in the plan, [a, b + c] can be [table1.a, "table2.b+c"].

Yes I think you are correct. I think what I found was confusing is that the qualifier is added "on demand" in several places and there isn't a clear cut line between "as written" and "qualified"

alamb · 2023-01-30T21:47:20Z

datafusion/expr/src/expr_rewriter/order_by.rs

@@ -136,24 +162,24 @@ mod test {

        let cases = vec![
            TestCase {
-                desc: "c1 --> t.c1",


I am not sure why direct aggregate creation doesn't need this rewrite anymore. All the other tests in DataFusion CI pass and I wrote the unit tests last week to document the existing behavior in #5088

If reviewers prefer to keep the old behavior here, that is easy as the first commit in this PR, 0c16ef1 actually keeps all these tests passing with the existing "aggregate" code.

alamb · 2023-01-30T21:48:22Z

datafusion/sql/tests/integration_test.rs

+    // expect that this is not an ambiguous reference
+    let expected =
+        "Sort: birth_date ASC NULLS LAST\
+         \n  Projection: AVG(person.age) AS value, datetrunc(Utf8(\"month\"), person.birth_date) AS birth_date\


this shows the plan is using the expression datetrunc rather than the original birth_date grouping column

alamb · 2023-01-31T10:51:31Z

This PR is now ready for review.

I don't know if @houqp or @andygrove have some time to review it too as I think they were involved with the order by / group by rewrites

…by_orderby

jackwener · 2023-02-02T07:53:38Z

datafusion/sql/tests/integration_test.rs

+fn select_groupby_orderby() {
+    let sql = r#"SELECT
+  avg(age) AS "value",
+  date_trunc('month', birth_date) AS "birth_date"
+  FROM person GROUP BY birth_date ORDER BY birth_date;


I suggest add more test about qualified column, I'm worried that it may cause some problem.
Because I also try to fix this problem, but meet some barrier about qualified column.

such as use date_trunc('month', person.birth_date) AS "birth_date"
instead of
date_trunc('month', birth_date) AS "birth_date"

It is a good idea -- Added in ba563ad

…by_orderby

alamb

Thank you for the review @jackwener

alamb · 2023-02-02T18:31:17Z

datafusion/sql/tests/integration_test.rs

+fn select_groupby_orderby() {
+    let sql = r#"SELECT
+  avg(age) AS "value",
+  date_trunc('month', birth_date) AS "birth_date"
+  FROM person GROUP BY birth_date ORDER BY birth_date;


It is a good idea -- Added in ba563ad

jackwener

A very nice job🚀 I reviewed this PR intermittently for a few days.
Look great to me.
cc @mingmwang @liukun4515

alamb · 2023-02-05T16:55:50Z

Thank you for the review @jackwener

I will plan to merge this PR tomorrow (in about 24 hours) unless anyone else would like time to review

Dandandan

Nice 🥳

ursabot · 2023-02-06T10:21:53Z

Benchmark runs are scheduled for baseline = b09c1ee and contender = 6eb0e36. 6eb0e36 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Jan 25, 2023

alamb force-pushed the alamb/support_groupby_orderby branch 3 times, most recently from 2cd0a0c to dbbbc25 Compare January 27, 2023 19:08

alamb commented Jan 27, 2023

View reviewed changes

alamb mentioned this pull request Jan 27, 2023

Can not ORDER BY an aliased group column #4854

Closed

jackwener self-requested a review January 29, 2023 10:23

alamb force-pushed the alamb/support_groupby_orderby branch from dbbbc25 to e11a032 Compare January 30, 2023 20:19

alamb added 2 commits January 30, 2023 16:33

Support grouping aliases in the order by clause

0c16ef1

simplify the code

6d85c1f

alamb force-pushed the alamb/support_groupby_orderby branch from 907964b to 6d85c1f Compare January 30, 2023 21:33

alamb commented Jan 30, 2023

View reviewed changes

alamb marked this pull request as ready for review January 31, 2023 10:50

Merge remote-tracking branch 'apache/master' into alamb/support_group…

3affe92

…by_orderby

jackwener reviewed Feb 2, 2023

View reviewed changes

alamb added 2 commits February 2, 2023 13:25

Merge remote-tracking branch 'apache/master' into alamb/support_group…

b7bfbbf

…by_orderby

Add more test coverage

ba563ad

alamb commented Feb 2, 2023

View reviewed changes

alamb mentioned this pull request Feb 2, 2023

aliasing a field renders it missing in the order by clause #669

Closed

jackwener approved these changes Feb 5, 2023

View reviewed changes

Dandandan approved these changes Feb 6, 2023

View reviewed changes

Dandandan merged commit 6eb0e36 into apache:master Feb 6, 2023

alamb deleted the alamb/support_groupby_orderby branch February 6, 2023 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ORDER BY an aliased column #5067

Support ORDER BY an aliased column #5067

alamb commented Jan 25, 2023 •

edited

Loading

alamb Jan 27, 2023

alamb Jan 27, 2023

alamb Jan 27, 2023

alamb commented Jan 27, 2023

alamb Jan 30, 2023

alamb Jan 30, 2023

jackwener Feb 2, 2023 •

edited

Loading

alamb Feb 2, 2023

alamb Jan 30, 2023

alamb Jan 30, 2023

alamb commented Jan 31, 2023

jackwener Feb 2, 2023 •

edited

Loading

alamb Feb 2, 2023

alamb left a comment

alamb Feb 2, 2023

jackwener left a comment •

edited

Loading

alamb commented Feb 5, 2023

Dandandan left a comment

ursabot commented Feb 6, 2023

		@@ -36,6 +36,12 @@ use datafusion_sql::planner::{ContextProvider, ParserOptions, SqlToRel};

		use rstest::rstest;

		#[cfg(test)]

Support ORDER BY an aliased column #5067

Support ORDER BY an aliased column #5067

Conversation

alamb commented Jan 25, 2023 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackwener Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 31, 2023

jackwener Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackwener left a comment • edited Loading

Choose a reason for hiding this comment

alamb commented Feb 5, 2023

Dandandan left a comment

Choose a reason for hiding this comment

ursabot commented Feb 6, 2023

alamb commented Jan 25, 2023 •

edited

Loading

jackwener Feb 2, 2023 •

edited

Loading

jackwener Feb 2, 2023 •

edited

Loading

jackwener left a comment •

edited

Loading