You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mustafasrepo (via GitHub)" <gi...@apache.org> on 2023/05/25 15:25:04 UTC
[GitHub] [arrow-datafusion] mustafasrepo opened a new pull request, #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
mustafasrepo opened a new pull request, #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes [#6444](https://github.com/apache/arrow-datafusion/issues/6444).
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# 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
```sql
SELECT a, b, FIRST_VALUE(c ORDER BY a DESC) as first_c
FROM annotated_data_infinite2
```
This query will give the first value for the `Column c` when table is ordered by `a DESC`.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
Yes, new tests are added to the `groupby.slt` file for new aggregate functions.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1206568695
##########
datafusion/core/tests/dataframe_functions.rs:
##########
@@ -155,11 +155,11 @@ async fn test_fn_approx_median() -> Result<()> {
let expr = approx_median(col("b"));
let expected = vec![
- "+----------------------+",
- "| APPROXMEDIAN(test.b) |",
- "+----------------------+",
- "| 10 |",
- "+----------------------+",
+ "+-----------------------+",
Review Comment:
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1207207062
##########
datafusion/expr/src/aggregate_function.rs:
##########
@@ -79,7 +84,9 @@ pub enum AggregateFunction {
impl fmt::Display for AggregateFunction {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
// uppercase of the debug.
- write!(f, "{}", format!("{self:?}").to_uppercase())
+ // Convert Camel form to uppercase snake
+ // such as FirstValue => FIRST_VALUE
+ write!(f, "{}", convert_camel_to_upper_snake(format!("{self:?}")))
}
Review Comment:
I think, your suggestion is better. By this way, we can avoid overfitting enum name convention. Also, having explicit match, helps with debugging also. I will add your suggestion to this Pr also.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1207375672
##########
datafusion/expr/src/aggregate_function.rs:
##########
@@ -81,12 +80,43 @@ pub enum AggregateFunction {
BoolOr,
}
+impl AggregateFunction {
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#issuecomment-1564971496
Thanks again @mustafasrepo !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#issuecomment-1564530136
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1207220264
##########
datafusion/core/tests/sqllogictests/test_files/groupby.slt:
##########
@@ -2007,6 +2008,58 @@ SELECT a, d,
1 4 913
1 2 848
+# test_source_sorted_groupby3
+
+query TT
+EXPLAIN SELECT a, b, FIRST_VALUE(c ORDER BY a DESC) as first_c
+ FROM annotated_data_infinite2
+ GROUP BY a, b
+----
+logical_plan
+Projection: annotated_data_infinite2.a, annotated_data_infinite2.b, FIRST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS first_c
+--Aggregate: groupBy=[[annotated_data_infinite2.a, annotated_data_infinite2.b]], aggr=[[FIRST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
+----TableScan: annotated_data_infinite2 projection=[a, b, c]
+physical_plan
+ProjectionExec: expr=[a@0 as a, b@1 as b, FIRST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as first_c]
+--AggregateExec: mode=Single, gby=[a@0 as a, b@1 as b], aggr=[FIRST_VALUE(annotated_data_infinite2.c)], ordering_mode=FullyOrdered
+----CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a, b, c], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST, b@1 ASC NULLS LAST, c@2 ASC NULLS LAST], has_header=true
+
+query III
+SELECT a, b, FIRST_VALUE(c ORDER BY a DESC) as first_c
+ FROM annotated_data_infinite2
+ GROUP BY a, b
+----
+0 0 0
+0 1 25
+1 2 50
+1 3 75
+
+# test_source_sorted_groupby4
+
+query TT
+EXPLAIN SELECT a, b, LAST_VALUE(c ORDER BY a DESC) as last_c
+ FROM annotated_data_infinite2
+ GROUP BY a, b
+----
+logical_plan
+Projection: annotated_data_infinite2.a, annotated_data_infinite2.b, LAST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS last_c
+--Aggregate: groupBy=[[annotated_data_infinite2.a, annotated_data_infinite2.b]], aggr=[[LAST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
+----TableScan: annotated_data_infinite2 projection=[a, b, c]
+physical_plan
+ProjectionExec: expr=[a@0 as a, b@1 as b, LAST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as last_c]
+--AggregateExec: mode=Single, gby=[a@0 as a, b@1 as b], aggr=[LAST_VALUE(annotated_data_infinite2.c)], ordering_mode=FullyOrdered
+----CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a, b, c], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST, b@1 ASC NULLS LAST, c@2 ASC NULLS LAST], has_header=true
+
+query III
Review Comment:
I added a test for this use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1207223843
##########
datafusion/core/tests/sqllogictests/test_files/window.slt:
##########
@@ -3017,6 +3017,13 @@ SELECT a, b, c,
0 0 3 11 96 11 2 10 36 10 36 11 5 11 9
0 0 4 9 72 9 NULL 14 45 14 45 9 4 9 9
+#fn aggregate order by with window frame
+# In window expressions, aggregate functions should not have an ordering requirement, such requirements
+# should be defined in the window frame. Therefore, the query below should generate an error. Note that
+# PostgreSQL also behaves this way.
+statement error DataFusion error: Error during planning: Aggregate ORDER BY is not implemented for window functions
+SELECT SUM(inc_col ORDER BY a DESC) OVER() as last
+ FROM annotated_data_infinite2
Review Comment:
I added a test for this use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1206568695
##########
datafusion/core/tests/dataframe_functions.rs:
##########
@@ -155,11 +155,11 @@ async fn test_fn_approx_median() -> Result<()> {
let expr = approx_median(col("b"));
let expected = vec![
- "+----------------------+",
- "| APPROXMEDIAN(test.b) |",
- "+----------------------+",
- "| 10 |",
- "+----------------------+",
+ "+-----------------------+",
Review Comment:
As part of this PR, I have added `convert_camel_to_upper_snake` util for display of Aggregate function. Their display name is automatically, calculate from struct name. Hence display name of some of the existing aggregators is changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1207207062
##########
datafusion/expr/src/aggregate_function.rs:
##########
@@ -79,7 +84,9 @@ pub enum AggregateFunction {
impl fmt::Display for AggregateFunction {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
// uppercase of the debug.
- write!(f, "{}", format!("{self:?}").to_uppercase())
+ // Convert Camel form to uppercase snake
+ // such as FirstValue => FIRST_VALUE
+ write!(f, "{}", convert_camel_to_upper_snake(format!("{self:?}")))
}
Review Comment:
I think, your suggestion is better. By this way, we can avoid to overfit, of enum name convention. Also, having explicit match, helps with debugging also. I will add your suggestion to this Pr also.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#issuecomment-1563441626
Marking as draft while the PR is worked on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1207207062
##########
datafusion/expr/src/aggregate_function.rs:
##########
@@ -79,7 +84,9 @@ pub enum AggregateFunction {
impl fmt::Display for AggregateFunction {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
// uppercase of the debug.
- write!(f, "{}", format!("{self:?}").to_uppercase())
+ // Convert Camel form to uppercase snake
+ // such as FirstValue => FIRST_VALUE
+ write!(f, "{}", convert_camel_to_upper_snake(format!("{self:?}")))
}
Review Comment:
I think, your suggestion is better. By this way, we can avoid overfitting enum name convention. Also, having explicit match, helps with debugging also.
I changed the code as your pattern, and removed `convert_camel_to_upper_snake`. Thanks for the suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#issuecomment-1563304302
Note that this change makes `FIRST_VALUE` and `LAST_VALUE` functions, which are already available as window functions, also available in an aggregation context so that one can choose first/last values within a group according to a given ordering. 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 (as in 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:
```sql
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1206568695
##########
datafusion/core/tests/dataframe_functions.rs:
##########
@@ -155,11 +155,11 @@ async fn test_fn_approx_median() -> Result<()> {
let expr = approx_median(col("b"));
let expected = vec![
- "+----------------------+",
- "| APPROXMEDIAN(test.b) |",
- "+----------------------+",
- "| 10 |",
- "+----------------------+",
+ "+-----------------------+",
Review Comment:
As part of this PR, I have added `convert_camel_to_upper_snake` util for display of Aggregate function. 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mustafasrepo commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1206568695
##########
datafusion/core/tests/dataframe_functions.rs:
##########
@@ -155,11 +155,11 @@ async fn test_fn_approx_median() -> Result<()> {
let expr = approx_median(col("b"));
let expected = vec![
- "+----------------------+",
- "| APPROXMEDIAN(test.b) |",
- "+----------------------+",
- "| 10 |",
- "+----------------------+",
+ "+-----------------------+",
Review Comment:
As part of this PR, I have added `convert_camel_to_upper_snake` util for display of Aggregate function. Their display name is automatically, calculate 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6445: Add support for FIRST_VALUE, LAST_VALUE Aggregate Functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6445:
URL: https://github.com/apache/arrow-datafusion/pull/6445#discussion_r1206907936
##########
datafusion/core/tests/dataframe_functions.rs:
##########
@@ -175,11 +175,11 @@ async fn test_fn_approx_percentile_cont() -> Result<()> {
let expr = approx_percentile_cont(col("b"), lit(0.5));
let expected = vec![
- "+-------------------------------------------+",
- "| APPROXPERCENTILECONT(test.b,Float64(0.5)) |",
- "+-------------------------------------------+",
- "| 10 |",
- "+-------------------------------------------+",
+ "+---------------------------------------------+",
Review Comment:
I think this is nice -- it makes the aggregates consistent with the (very) recent change from @2010YOUY01 for scalar functions: https://github.com/apache/arrow-datafusion/pull/6448
##########
datafusion/expr/src/aggregate_function.rs:
##########
@@ -79,7 +84,9 @@ pub enum AggregateFunction {
impl fmt::Display for AggregateFunction {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
// uppercase of the debug.
- write!(f, "{}", format!("{self:?}").to_uppercase())
+ // Convert Camel form to uppercase snake
+ // such as FirstValue => FIRST_VALUE
+ write!(f, "{}", convert_camel_to_upper_snake(format!("{self:?}")))
}
Review Comment:
I think this would be better if it were explicit rather than trying to reverse engineer the debug name. That way we wouldn't have to tie the rust variant name to the enum name.
For example
```rust
impl AggregateFunction
fn name(&self) -> &str {
match self
...
FirstValue => "first_value",
...
}
}
}
impl fmt::Display for AggregateFunction {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self.name())
}
}
```
What do you think?
We can do this in a follow on PR but it would be nice to avoid the `convert_camel_to_upper_snake` code
##########
datafusion/core/tests/sqllogictests/test_files/window.slt:
##########
@@ -3017,6 +3017,13 @@ SELECT a, b, c,
0 0 3 11 96 11 2 10 36 10 36 11 5 11 9
0 0 4 9 72 9 NULL 14 45 14 45 9 4 9 9
+#fn aggregate order by with window frame
Review Comment:
👍
##########
datafusion/core/tests/sqllogictests/test_files/window.slt:
##########
@@ -3017,6 +3017,13 @@ SELECT a, b, c,
0 0 3 11 96 11 2 10 36 10 36 11 5 11 9
0 0 4 9 72 9 NULL 14 45 14 45 9 4 9 9
+#fn aggregate order by with window frame
+# In window expressions, aggregate functions should not have an ordering requirement, such requirements
+# should be defined in the window frame. Therefore, the query below should generate an error. Note that
+# PostgreSQL also behaves this way.
+statement error DataFusion error: Error during planning: Aggregate ORDER BY is not implemented for window functions
+SELECT SUM(inc_col ORDER BY a DESC) OVER() as last
+ FROM annotated_data_infinite2
Review Comment:
I recommend another negative test to make sure the window function and over clause can not be used in the same query. Something like
```sql
EXPLAIN SELECT a, b, LAST_VALUE(c ORDER BY a DESC) OVER (order by a) as last_c
```
##########
docs/source/user-guide/sql/aggregate_functions.md:
##########
@@ -202,6 +204,32 @@ array_agg(expression [ORDER BY expression])
#### Arguments
+- **expression**: Expression to operate on.
Review Comment:
👍
##########
datafusion/core/tests/sqllogictests/test_files/groupby.slt:
##########
@@ -2007,6 +2008,58 @@ SELECT a, d,
1 4 913
1 2 848
+# test_source_sorted_groupby3
+
+query TT
+EXPLAIN SELECT a, b, FIRST_VALUE(c ORDER BY a DESC) as first_c
+ FROM annotated_data_infinite2
+ GROUP BY a, b
+----
+logical_plan
+Projection: annotated_data_infinite2.a, annotated_data_infinite2.b, FIRST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS first_c
+--Aggregate: groupBy=[[annotated_data_infinite2.a, annotated_data_infinite2.b]], aggr=[[FIRST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
+----TableScan: annotated_data_infinite2 projection=[a, b, c]
+physical_plan
+ProjectionExec: expr=[a@0 as a, b@1 as b, FIRST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as first_c]
+--AggregateExec: mode=Single, gby=[a@0 as a, b@1 as b], aggr=[FIRST_VALUE(annotated_data_infinite2.c)], ordering_mode=FullyOrdered
+----CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a, b, c], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST, b@1 ASC NULLS LAST, c@2 ASC NULLS LAST], has_header=true
+
+query III
+SELECT a, b, FIRST_VALUE(c ORDER BY a DESC) as first_c
+ FROM annotated_data_infinite2
+ GROUP BY a, b
+----
+0 0 0
+0 1 25
+1 2 50
+1 3 75
+
+# test_source_sorted_groupby4
+
+query TT
+EXPLAIN SELECT a, b, LAST_VALUE(c ORDER BY a DESC) as last_c
+ FROM annotated_data_infinite2
+ GROUP BY a, b
+----
+logical_plan
+Projection: annotated_data_infinite2.a, annotated_data_infinite2.b, LAST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS last_c
+--Aggregate: groupBy=[[annotated_data_infinite2.a, annotated_data_infinite2.b]], aggr=[[LAST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
+----TableScan: annotated_data_infinite2 projection=[a, b, c]
+physical_plan
+ProjectionExec: expr=[a@0 as a, b@1 as b, LAST_VALUE(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as last_c]
+--AggregateExec: mode=Single, gby=[a@0 as a, b@1 as b], aggr=[LAST_VALUE(annotated_data_infinite2.c)], ordering_mode=FullyOrdered
+----CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a, b, c], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST, b@1 ASC NULLS LAST, c@2 ASC NULLS LAST], has_header=true
+
+query III
Review Comment:
I wonder if it is worth adding some sort of test of the behavior when there is no order? Like `SELECT a, b, LAST_VALUE(c) as last_c` -- mostly to ensure it doesn't cause an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org