Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions #6445

mustafasrepo · 2023-05-25T15:25:02Z

Which issue does this PR close?

Closes #6444.

Rationale for this change

What changes are included in this PR?

This PR adds support for FIRST_VALUE, LAST_VALUE ordering sensitive aggregation functions. With the changes in this PR we now can run query below

SELECT a, FIRST_VALUE(c ORDER BY b DESC) as first_c
FROM table
GROUP BY a

This query will return the first c value, according to the reverse b order, for every a group.

Are these changes tested?

Yes, new tests are added to the groupby.slt file for new aggregate functions.

Are there any user-facing changes?

i

# Conflicts: # datafusion/core/tests/sqllogictests/test_files/aggregate.slt

# 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

…rst_last_aggregate # Conflicts: # datafusion/expr/Cargo.toml

# Conflicts: # datafusion-cli/Cargo.lock # datafusion/common/Cargo.toml # datafusion/core/Cargo.toml # datafusion/expr/Cargo.toml # datafusion/sql/Cargo.toml

ozankabak · 2023-05-25T18:04:55Z

Note that this change makes FIRST_VALUE and LAST_VALUE functions available in an aggregation context, which were previously only available as window functions. With this change, one can choose first/last values within a group according to a given ordering. In aggregation contexts, these functions are sometimes called FIRST and LAST in other query engines/DBs, but we chose to use the same with their window function counterparts (to be consistent with the case of SUM, COUNT etc.)

Having said that, there are few minor issues here that we will fix shortly. I think the example in the PR body should read something like:

SELECT a, FIRST_VALUE(c ORDER BY b DESC) as first_c
FROM table
GROUP BY a

This will return the first c value, according to the reverse b order, for every a group.

Also some tests are failing. We will fix the tests, make the PR body clearer and possibly add another test that looks like this.

alamb · 2023-05-25T20:03:42Z

Marking as draft while the PR is worked on

# 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.

mustafasrepo · 2023-05-26T10:33:15Z

datafusion/core/tests/dataframe_functions.rs

-        "+----------------------+",
-        "| 10                   |",
-        "+----------------------+",
+        "+-----------------------+",


As part of this PR, I have added convert_camel_to_upper_snake util for displaying Aggregate functions. Their display name is automatically, calculated from struct name. Hence display name of some of the existing aggregators is changed(such as APPROXMEDIAN became APPROX_MEDIAN, ARRAYAGG became ARRAY_AGG, etc).

mustafasrepo · 2023-05-26T15:03:57Z

Marking as draft while the PR is worked on

I have resolved the CI issue, also I have updated Pr body to reflect intent better. This PR is ready for further review.

alamb

Thank you @mustafasrepo -- I think the feature looks great and really nicely coded 👌 .

I think a different approach for display name that is not so tightly bound to the Rust struct name would be better, but that is a stylistic opinion and something we can fix in a follow on PR. I left some more specific suggestions inline

alamb · 2023-05-26T14:53:03Z

datafusion/core/tests/dataframe_functions.rs

-        "+-------------------------------------------+",
-        "| 10                                        |",
-        "+-------------------------------------------+",
+        "+---------------------------------------------+",


I think this is nice -- it makes the aggregates consistent with the (very) recent change from @2010YOUY01 for scalar functions: #6448

datafusion/expr/src/aggregate_function.rs

docs/source/user-guide/sql/aggregate_functions.md

datafusion/core/tests/sqllogictests/test_files/window.slt

datafusion/core/tests/sqllogictests/test_files/groupby.slt

alamb · 2023-05-26T21:23:18Z

Thanks again @mustafasrepo !

alamb · 2023-05-26T21:22:58Z

datafusion/expr/src/aggregate_function.rs

    BoolOr,
 }

+impl AggregateFunction {


mustafasrepo and others added 30 commits May 3, 2023 14:46

Naive test pass

7411d6b

i

Add new tests and simplifications

87c397d

move tests to the .slt file

ebd0e5f

update requirement

ecc5b0d

update tests

7093fa7

Add support for partiallyOrdered aggregation sensitive.

e377ea9

Resolve linter errors

e0ce989

Merge branch 'main' into feature/ordering_sensitive_aggregation

e3edf7c

# Conflicts: # datafusion/core/tests/sqllogictests/test_files/aggregate.slt

update comments

550416e

minor changes

d72cc8f

Merge branch 'main' into feature/ordering_sensitive_aggregation

713b56b

retract changes in generated

3dcf079

update proto files

a817887

Simplifications

740a605

Make types consistent in schema, and data

8209c46

Update todos

d93b187

Convert API to vector

5bb8dde

Convert get_finest to handle Vector inputs

72985f8

simplifications, update comment

4ae5c25

initial commit, add test

6fd96da

Add support for FIRST Aggregate function.

61cc109

Add support for last aggregate

682b1f4

Update cargo.lock

670a327

Remove distinct, and limit from First and last aggregate.

4c27b85

Add reverse for First and Last Aggregator

e9535bd

Update cargo lock

2143b72

Merge branch 'feature/ordering_sensitive_aggregation' into feature/fi…

10e7811

…rst_last_aggregate # Conflicts: # datafusion/expr/Cargo.toml

Minor code simplifications

c59ce9b

Update comment

90aa195

mustafasrepo and others added 9 commits May 23, 2023 13:00

simplifications

4ce1038

Merge branch 'main' into feature/first_last_aggregate2

148dec2

Fix cargo lock file

1bdda38

Merge branch 'main' into feature/first_last_aggregate2

fab0dcd

# Conflicts: # datafusion-cli/Cargo.lock # datafusion/common/Cargo.toml # datafusion/core/Cargo.toml # datafusion/expr/Cargo.toml # datafusion/sql/Cargo.toml

Update comment

94fe486

Rename aggregator first and last

13ee806

minor change

1cd494f

Comment improvements

f2a0a68

Remove count from First,Last accumulators

c4a47e2

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

alamb marked this pull request as draft May 25, 2023 20:03

Merge branch 'main' into feature/first_last_aggregate2

c20ca75

# 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.

mustafasrepo marked this pull request as ready for review May 26, 2023 07:22

Merge branch 'main' into feature/first_last_aggregate2

d140e97

# 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.

mustafasrepo commented May 26, 2023

View reviewed changes

alamb approved these changes May 26, 2023

View reviewed changes

mustafasrepo added 3 commits May 26, 2023 21:31

Address reviews

18935b2

Remove camel to upper snake util, make aggregate function names explicit

48057cc

update the test

5cc149a

alamb merged commit f54f514 into apache:main May 26, 2023

alamb reviewed May 26, 2023

View reviewed changes

datafusion/expr/src/aggregate_function.rs

BoolOr,

}

impl AggregateFunction {

Copy link

Contributor

alamb May 26, 2023

Choose a reason for hiding this comment

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

👍

mustafasrepo deleted the feature/first_last_aggregate2 branch May 29, 2023 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions #6445

Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions #6445

Uh oh!

mustafasrepo commented May 25, 2023 •

edited

Loading

Uh oh!

ozankabak commented May 25, 2023 •

edited

Loading

Uh oh!

alamb commented May 25, 2023

Uh oh!

mustafasrepo May 26, 2023 •

edited

Loading

Uh oh!

mustafasrepo commented May 26, 2023

Uh oh!

alamb left a comment •

edited

Loading

Uh oh!

alamb May 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented May 26, 2023

Uh oh!

alamb May 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions #6445

Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions #6445

Uh oh!

Conversation

mustafasrepo commented May 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!

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

Uh oh!

Uh oh!

alamb commented May 25, 2023

Uh oh!

mustafasrepo May 26, 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!

mustafasrepo commented May 26, 2023

Uh oh!

alamb 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 May 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alamb commented May 26, 2023

Uh oh!

alamb May 26, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mustafasrepo commented May 25, 2023 •

edited

Loading

ozankabak commented May 25, 2023 •

edited

Loading

mustafasrepo May 26, 2023 •

edited

Loading

alamb left a comment •

edited

Loading