Skip to content
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

Jump to bottom

Add support for ordering sensitive aggregation #6332

Merged
merged 30 commits into from
May 15, 2023
Merged

Add support for ordering sensitive aggregation #6332

merged 30 commits into from
May 15, 2023
+979 −112

Conversation

mustafasrepo
Copy link
Contributor

@mustafasrepo mustafasrepo commented May 11, 2023 •
edited
Loading

Which issue does this PR close?

Closes #6189.

Rationale for this change

Currently datafusion has support for queries as below

SELECT (ARRAY_AGG(s.amount)) AS amounts
            FROM sales_global AS s
            GROUP BY s.ts

However, when aggregation queries expect their input to have an ordering, queries fail. As an example see the query below.

SELECT (ARRAY_AGG(s.amount ORDER BY s.sn DESC)) AS amounts
            FROM sales_global AS s
            GROUP BY s.ts

Above query can be parsed successfully, however it cannot be executed. With this PR we add support for queries in the form above, where aggregate function requires an ordering for its input

What changes are included in this PR?

In this PR

  • We add support for ordering sensitive aggregate functions.
  • We refactored create_physical_sort_expr function.

Are these changes tested?

Yes new tests are added to the aggregate.slt file.

Are there any user-facing changes?

mustafasrepo and others added 22 commits May 3, 2023 14:46
@mustafasrepo
Naive test pass
7411d6b 
i
@mustafasrepo
Add new tests and simplifications
87c397d
@mustafasrepo
move tests to the .slt file
ebd0e5f
@mustafasrepo
update requirement
ecc5b0d
@mustafasrepo
update tests
7093fa7
@mustafasrepo
Add support for partiallyOrdered aggregation sensitive.
e377ea9
@mustafasrepo
Resolve linter errors
e0ce989
@mustafasrepo
Merge branch 'main' into feature/ordering_sensitive_aggregation
e3edf7c 
# Conflicts:
#	datafusion/core/tests/sqllogictests/test_files/aggregate.slt
@mustafasrepo
update comments
550416e
@mustafasrepo
minor changes
d72cc8f
@mustafasrepo
Merge branch 'main' into feature/ordering_sensitive_aggregation
713b56b
@mustafasrepo
retract changes in generated
3dcf079
@mustafasrepo
update proto files
a817887
@ozankabak
Simplifications
740a605
@mustafasrepo
Make types consistent in schema, and data
8209c46
@mustafasrepo
Update todos
d93b187
@mustafasrepo
Convert API to vector
5bb8dde
@mustafasrepo
Convert get_finest to handle Vector inputs
72985f8
@mustafasrepo
simplifications, update comment
4ae5c25
@mustafasrepo
Merge branch 'main' into feature/ordering_sensitive_aggregation
0a52150 
# Conflicts:
#	datafusion/core/src/physical_plan/planner.rs
#	datafusion/core/tests/sqllogictests/test_files/aggregate.slt
#	datafusion/expr/src/expr.rs
#	datafusion/expr/src/tree_node/expr.rs
#	datafusion/expr/src/udaf.rs
#	datafusion/optimizer/src/analyzer/type_coercion.rs
#	datafusion/optimizer/src/common_subexpr_eliminate.rs
#	datafusion/proto/src/logical_plan/from_proto.rs
#	datafusion/proto/src/logical_plan/mod.rs
#	datafusion/proto/src/logical_plan/to_proto.rs
#	datafusion/sql/src/expr/function.rs
#	datafusion/sql/src/utils.rs
@ozankabak
Minor code simplifications
c59ce9b
@mustafasrepo
Update comment
90aa195
@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate labels May 11, 2023
alamb
alamb reviewed May 11, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @mustafasrepo

I will try and review this PR over the next day or two.

I think we should document this feature as well in https://arrow.apache.org/datafusion/user-guide/sql/select.html#group-by-clause if possible

I also noticed that window functions are not actually documented yet, so I filed #6338 to track that

@mustafasrepo
Copy link
Contributor Author

Thank you @mustafasrepo

I will try and review this PR over the next day or two.

I think we should document this feature as well in https://arrow.apache.org/datafusion/user-guide/sql/select.html#group-by-clause if possible

I also noticed that window functions are not actually documented yet, so I filed #6338 to track that

Thanks @alamb, I wasn't aware of the documentation part. I have updated the documentation at https://arrow.apache.org/datafusion/user-guide/sql/select.html#group-by-clause and https://arrow.apache.org/datafusion/user-guide/sql/aggregate_functions.html#array-agg

@alamb alamb added the api change Changes the API exposed to users of the crate label May 12, 2023
alamb
alamb approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @mustafasrepo -- this looks really nice

I especially like how ordering is expressed as an updated "required_input_ordering" and then the rest of the datafusion planning machinery handles generating and optimizing the actual sorting. Very nice 👏

datafusion/core/tests/sqllogictests/test_files/groupby.slt
TUR [75.0, 100.0]

# test_ordering_sensitive_aggregation2
# We should be able to satisfy the finest requirement among all aggregators, when we have multiple aggregators.
Copy link
Contributor

Choose a reason for hiding this comment

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

This test verifies that an aggregate with no sort and a single expr works

I think it would be valuable to also test more than one column, something like:

ARRAY_AGG(s.amount ORDER BY s.amount DESC, ts ASC) AS amounts,
SUM(s.amount ORDER BY s.amount DESC) AS sum1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, we cannot support these kind of queries (because we cannot parse multiple requirement, PR in the sqlparser fixes the problem. However, it is not merged yet, I am not sure when it will be available to datafusion.). However, once this support is added. This case should be fine. I have added a test for this use case, we currently generate an error, once this is added. We can change the test.

@mustafasrepo
Simplifications, Address reviews
1d8d4ba
@mustafasrepo
Update comment
01c8da8
@mustafasrepo
Merge branch 'main' into feature/ordering_sensitive_aggregation
c5be504
@mustafasrepo
Resolve linter errors
c23945d
@mustafasrepo
Merge branch 'main' into feature/ordering_sensitive_aggregation
7c955eb 
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@alamb alamb merged commit d8a92be into apache:main May 15, 2023
@alamb
Copy link
Contributor

alamb commented May 15, 2023

Thanks again @mustafasrepo

@mustafasrepo mustafasrepo mentioned this pull request May 25, 2023
Closed
@mustafasrepo mustafasrepo deleted the feature/ordering_sensitive_aggregation branch July 21, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for ordered aggregation
3 participants
@mustafasrepo @alamb @ozankabak