Support ORDER BY an aliased column by alamb · Pull Request #5067 · apache/datafusion

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


 use rstest::rstest;

+#[cfg(test)]


this is to enable debug logging in the sql tests

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

+/// 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

+/// 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

-                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

+        // 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


        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

+    // 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

+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

+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

Conversation

alamb commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Jan 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackwener Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alamb commented Jan 31, 2023

Uh oh!

jackwener Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

jackwener left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Feb 5, 2023

Uh oh!

Dandandan left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Feb 6, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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