You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "huaxingao (via GitHub)" <gi...@apache.org> on 2024/04/08 06:24:43 UTC

[PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

huaxingao opened a new pull request, #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991

   ## 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 https://github.com/apache/arrow-datafusion/issues/9924.
   
   ## 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?
   
   <!--
   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?
   
   <!--
   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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1555287323


##########
datafusion/sqllogictest/test_files/group_by.slt:
##########
@@ -2129,41 +2129,11 @@ query III
 1 2 1550
 1 3 2175
 
-
-# test_source_sorted_groupby2
-# If ordering is not important for the aggregation function, we should ignore the ordering requirement. Hence
-# "ORDER BY a DESC" should have no effect.
-query TT
-EXPLAIN SELECT a, d,
- SUM(c ORDER BY a DESC) as summation1
- FROM annotated_data_infinite2
- GROUP BY d, a
-----
-logical_plan
-Projection: annotated_data_infinite2.a, annotated_data_infinite2.d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS summation1
---Aggregate: groupBy=[[annotated_data_infinite2.d, annotated_data_infinite2.a]], aggr=[[SUM(CAST(annotated_data_infinite2.c AS Int64)) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
-----TableScan: annotated_data_infinite2 projection=[a, c, d]
-physical_plan
-ProjectionExec: expr=[a@1 as a, d@0 as d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as summation1]
---AggregateExec: mode=Single, gby=[d@2 as d, a@0 as a], aggr=[SUM(annotated_data_infinite2.c)], ordering_mode=PartiallySorted([1])
-----StreamingTableExec: partition_sizes=1, projection=[a, c, d], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST]
-
-query III
+statement error DataFusion error: This feature is not implemented: ORDER BY is not implemented for SUM

Review Comment:
   I am not sure if I understand the requirement of https://github.com/apache/arrow-datafusion/issues/9924 correctly:
   
   In my PR, I have `support_ordering` to true for `first`, `last`, `nth_value` and `array_agg_ordered`. For all the other aggregate functions, `support_ordering` is false and `ORDER BY` returns `not implemented` error.
   
   Is this what we want? This seems to be a breaking change for me.
   



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#issuecomment-2053697778

   @alamb 
   
   I am not so sure how to proceed with this PR. Do we still need this PR?
   For `IGNORE NULLS`, I think we have consensus to error in the parser if aggregates don't support `IGNORE NULLS`.
   
   For `ORDER BY`, there are two options:
   1. Keep the current behavior, that is, allow `SUM(ORDER BY ...)`. The rationale is:
   
   - The DataFusion optimizer would remove the `ORDER BY` if the order doesn't matter.
   - For float type, the order matters, so we need the `ORDER BY`
   - Postgres allows the `SUM(ORDER BY ...)`.
   
   2. Error in the parser if ordering doesn't matter (That's what Spark does).
   
   So, we will either change the parser or leave the current implementation as is. It seems to me we don't need this PR anymore?


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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1559893156


##########
datafusion/sqllogictest/test_files/group_by.slt:
##########
@@ -2129,41 +2129,11 @@ query III
 1 2 1550
 1 3 2175
 
-
-# test_source_sorted_groupby2
-# If ordering is not important for the aggregation function, we should ignore the ordering requirement. Hence
-# "ORDER BY a DESC" should have no effect.
-query TT
-EXPLAIN SELECT a, d,
- SUM(c ORDER BY a DESC) as summation1
- FROM annotated_data_infinite2
- GROUP BY d, a
-----
-logical_plan
-Projection: annotated_data_infinite2.a, annotated_data_infinite2.d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS summation1
---Aggregate: groupBy=[[annotated_data_infinite2.d, annotated_data_infinite2.a]], aggr=[[SUM(CAST(annotated_data_infinite2.c AS Int64)) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
-----TableScan: annotated_data_infinite2 projection=[a, c, d]
-physical_plan
-ProjectionExec: expr=[a@1 as a, d@0 as d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as summation1]
---AggregateExec: mode=Single, gby=[d@2 as d, a@0 as a], aggr=[SUM(annotated_data_infinite2.c)], ordering_mode=PartiallySorted([1])
-----StreamingTableExec: partition_sizes=1, projection=[a, c, d], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST]
-
-query III
+statement error DataFusion error: This feature is not implemented: ORDER BY is not implemented for SUM

Review Comment:
   I hadn't thought about the implications for `SUM(ORDER BY ...)` for floats 🤔  It appears that this is consistent with what postgres does:
   
   
   ```sql
   postgres=# create table foo (x float);
   CREATE TABLE
   postgres=# insert into foo values (1.0);
   INSERT 0 1
   postgres=# insert into foo values (2.0);
   INSERT 0 1
   postgres=# insert into foo values (-1.0);
   INSERT 0 1
   postgres=# select sum(x ORDER BY x) from foo;
    sum
   -----
      2
   (1 row)
   
   postgres=# select sum(x IGNORE NULLS) from foo;
   ERROR:  syntax error at or near "IGNORE"
   LINE 1: select sum(x IGNORE NULLS) from foo;
                        ^
   postgres=#
   ```
   
   I also verified that postgres actually does sort the input:
   ```sql
   postgres=# explain select sum(x ORDER BY x) from foo;
                               QUERY PLAN
   -------------------------------------------------------------------
    Aggregate  (cost=169.81..169.82 rows=1 width=8)
      ->  Sort  (cost=158.51..164.16 rows=2260 width=8)
            Sort Key: x
            ->  Seq Scan on foo  (cost=0.00..32.60 rows=2260 width=8)
   (4 rows)
   
   postgres=#
   ```



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1557905465


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3428,3 +3428,16 @@ SELECT LAST_VALUE(column1 ORDER BY column2 DESC) IGNORE NULLS FROM t;
 
 statement ok
 DROP TABLE t;
+
+# Test for IGNORE NULLS / ORDER BY not implemented
+statement error DataFusion error: This feature is not implemented: IGNORE NULLS is not implemented for COUNT
+SELECT COUNT(*) IGNORE NULLS FROM (values (1), (null), (2));

Review Comment:
   Filed https://github.com/sqlparser-rs/sqlparser-rs/issues/1206



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#issuecomment-2048188691

   > this query is invalid, our parser allows it but it should fail. IGNORE NULLS have a limited functions to be working with. all other query engine parsers does.
   
   `IGNORE NULLS` definitely makes sense to error if the aggregate doesn't handle it (as it would result in incorrect answers)
   
   `ORDER BY` seems more gray to me -- as in the answers won't be incorrect but it would potentially be doing more work than necessary


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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#issuecomment-2053710366

   > For `ORDER BY`, there are two options:
   > 
   > 1. Keep the current behavior, that is, allow `SUM(ORDER BY ...)`. The rationale is:
   > 
   > * The DataFusion optimizer would remove the `ORDER BY` if the order doesn't matter.
   > * For float type, the order matters, so we need the `ORDER BY`
   > * Postgres allows the `SUM(ORDER BY ...)`.
   
   Yes, let's follow this approach.
   
   


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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1559866209


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3428,3 +3428,16 @@ SELECT LAST_VALUE(column1 ORDER BY column2 DESC) IGNORE NULLS FROM t;
 
 statement ok
 DROP TABLE t;
+
+# Test for IGNORE NULLS / ORDER BY not implemented
+statement error DataFusion error: This feature is not implemented: IGNORE NULLS is not implemented for COUNT
+SELECT COUNT(*) IGNORE NULLS FROM (values (1), (null), (2));

Review Comment:
   Queries like that should fail by the invalid syntax and this is parsers work to stop it IMHO. And spark doesn't allow query like that, it fails on query compile stage. 
   
   ```
   scala> spark.sql("SELECT COUNT(*) IGNORE NULLS over () FROM (values (1), (null), (2));").show(false)
   org.apache.spark.sql.AnalysisException: Function count does not support IGNORE NULLS.; line 1 pos 7
     at org.apache.spark.sql.errors.QueryCompilationErrors$.functionWithUnsupportedSyntaxError(QueryCompilationErrors.scala:602)
   ```



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1559844325


##########
datafusion/sqllogictest/test_files/group_by.slt:
##########
@@ -2129,41 +2129,11 @@ query III
 1 2 1550
 1 3 2175
 
-
-# test_source_sorted_groupby2
-# If ordering is not important for the aggregation function, we should ignore the ordering requirement. Hence
-# "ORDER BY a DESC" should have no effect.
-query TT
-EXPLAIN SELECT a, d,
- SUM(c ORDER BY a DESC) as summation1
- FROM annotated_data_infinite2
- GROUP BY d, a
-----
-logical_plan
-Projection: annotated_data_infinite2.a, annotated_data_infinite2.d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS summation1
---Aggregate: groupBy=[[annotated_data_infinite2.d, annotated_data_infinite2.a]], aggr=[[SUM(CAST(annotated_data_infinite2.c AS Int64)) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
-----TableScan: annotated_data_infinite2 projection=[a, c, d]
-physical_plan
-ProjectionExec: expr=[a@1 as a, d@0 as d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as summation1]
---AggregateExec: mode=Single, gby=[d@2 as d, a@0 as a], aggr=[SUM(annotated_data_infinite2.c)], ordering_mode=PartiallySorted([1])
-----StreamingTableExec: partition_sizes=1, projection=[a, c, d], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST]
-
-query III
+statement error DataFusion error: This feature is not implemented: ORDER BY is not implemented for SUM

Review Comment:
   This depends on the data type. For integral types, `ORDER BY` doesn't matter for `SUM` (but it shouldn't be an error to still specify it -- the optimizer should just remove it). For floating-point types, it actually makes a difference in the result.



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1557885388


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3428,3 +3428,16 @@ SELECT LAST_VALUE(column1 ORDER BY column2 DESC) IGNORE NULLS FROM t;
 
 statement ok
 DROP TABLE t;
+
+# Test for IGNORE NULLS / ORDER BY not implemented
+statement error DataFusion error: This feature is not implemented: IGNORE NULLS is not implemented for COUNT
+SELECT COUNT(*) IGNORE NULLS FROM (values (1), (null), (2));

Review Comment:
   good, however this should be early exited on parser... I'll create a follow up for the parser



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1559832348


##########
datafusion/sqllogictest/test_files/group_by.slt:
##########
@@ -2129,41 +2129,11 @@ query III
 1 2 1550
 1 3 2175
 
-
-# test_source_sorted_groupby2
-# If ordering is not important for the aggregation function, we should ignore the ordering requirement. Hence
-# "ORDER BY a DESC" should have no effect.
-query TT
-EXPLAIN SELECT a, d,
- SUM(c ORDER BY a DESC) as summation1
- FROM annotated_data_infinite2
- GROUP BY d, a
-----
-logical_plan
-Projection: annotated_data_infinite2.a, annotated_data_infinite2.d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS summation1
---Aggregate: groupBy=[[annotated_data_infinite2.d, annotated_data_infinite2.a]], aggr=[[SUM(CAST(annotated_data_infinite2.c AS Int64)) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
-----TableScan: annotated_data_infinite2 projection=[a, c, d]
-physical_plan
-ProjectionExec: expr=[a@1 as a, d@0 as d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as summation1]
---AggregateExec: mode=Single, gby=[d@2 as d, a@0 as a], aggr=[SUM(annotated_data_infinite2.c)], ordering_mode=PartiallySorted([1])
-----StreamingTableExec: partition_sizes=1, projection=[a, c, d], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST]
-
-query III
+statement error DataFusion error: This feature is not implemented: ORDER BY is not implemented for SUM

Review Comment:
   @mustafasrepo  / @ozankabak  / @metesynnada  can you help us understand if the ordering for `SUM(c ORDER BY a DESC)` has meaning?
   
   DataFusion currently ignores cases
   
   We propose making it an error to include clauses that make no sense
   
   However, I could see an argument for permitting the user to write `ORDER BY` even if the aggregate didn't care about ordering (and simply remove the ORDER BY as a optimization) 
   
   I can also see the rationale for failing fast and erroring as the user probably didn't meant to specify an ordering on SUM 🤔 



##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3428,3 +3428,16 @@ SELECT LAST_VALUE(column1 ORDER BY column2 DESC) IGNORE NULLS FROM t;
 
 statement ok
 DROP TABLE t;
+
+# Test for IGNORE NULLS / ORDER BY not implemented
+statement error DataFusion error: This feature is not implemented: IGNORE NULLS is not implemented for COUNT
+SELECT COUNT(*) IGNORE NULLS FROM (values (1), (null), (2));

Review Comment:
   I think we need to check in the physical plan layer as well for the reason @andygrove  says.
   
   Another approach could be an analyzer rule (following the model of how certain subqueries are handled) like this:
    
   https://github.com/apache/arrow-datafusion/blob/582050728914650c6d4340ca803a0e9af087d8ec/datafusion/optimizer/src/analyzer/subquery.rs#L32-L39
   
   



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1559844325


##########
datafusion/sqllogictest/test_files/group_by.slt:
##########
@@ -2129,41 +2129,11 @@ query III
 1 2 1550
 1 3 2175
 
-
-# test_source_sorted_groupby2
-# If ordering is not important for the aggregation function, we should ignore the ordering requirement. Hence
-# "ORDER BY a DESC" should have no effect.
-query TT
-EXPLAIN SELECT a, d,
- SUM(c ORDER BY a DESC) as summation1
- FROM annotated_data_infinite2
- GROUP BY d, a
-----
-logical_plan
-Projection: annotated_data_infinite2.a, annotated_data_infinite2.d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS summation1
---Aggregate: groupBy=[[annotated_data_infinite2.d, annotated_data_infinite2.a]], aggr=[[SUM(CAST(annotated_data_infinite2.c AS Int64)) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
-----TableScan: annotated_data_infinite2 projection=[a, c, d]
-physical_plan
-ProjectionExec: expr=[a@1 as a, d@0 as d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as summation1]
---AggregateExec: mode=Single, gby=[d@2 as d, a@0 as a], aggr=[SUM(annotated_data_infinite2.c)], ordering_mode=PartiallySorted([1])
-----StreamingTableExec: partition_sizes=1, projection=[a, c, d], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST]
-
-query III
+statement error DataFusion error: This feature is not implemented: ORDER BY is not implemented for SUM

Review Comment:
   A first principles viewpoint suggests this should depend on the data type. For integral types, `ORDER BY` wouldn't matter for `SUM`, but it shouldn't be an error to still specify it -- the optimizer should just remove it. For floating-point types, the summation order actually makes a difference in the result. Any data type for which addition doesn't commute, `ORDER BY` will have an impact on the result.
   
   However, I'm not sure if SQL standard says anything definitive in this matter. If not, it would be prudent to follow the results of this first-principles analysis.



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [datafusion]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9991:
URL: https://github.com/apache/datafusion/pull/9991#issuecomment-2164199857

   Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.


-- 
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@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1559844325


##########
datafusion/sqllogictest/test_files/group_by.slt:
##########
@@ -2129,41 +2129,11 @@ query III
 1 2 1550
 1 3 2175
 
-
-# test_source_sorted_groupby2
-# If ordering is not important for the aggregation function, we should ignore the ordering requirement. Hence
-# "ORDER BY a DESC" should have no effect.
-query TT
-EXPLAIN SELECT a, d,
- SUM(c ORDER BY a DESC) as summation1
- FROM annotated_data_infinite2
- GROUP BY d, a
-----
-logical_plan
-Projection: annotated_data_infinite2.a, annotated_data_infinite2.d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS summation1
---Aggregate: groupBy=[[annotated_data_infinite2.d, annotated_data_infinite2.a]], aggr=[[SUM(CAST(annotated_data_infinite2.c AS Int64)) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
-----TableScan: annotated_data_infinite2 projection=[a, c, d]
-physical_plan
-ProjectionExec: expr=[a@1 as a, d@0 as d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as summation1]
---AggregateExec: mode=Single, gby=[d@2 as d, a@0 as a], aggr=[SUM(annotated_data_infinite2.c)], ordering_mode=PartiallySorted([1])
-----StreamingTableExec: partition_sizes=1, projection=[a, c, d], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST]
-
-query III
+statement error DataFusion error: This feature is not implemented: ORDER BY is not implemented for SUM

Review Comment:
   A first principles viewpoint suggests this should depend on the data type. For integral types, `ORDER BY` wouldn't matter for `SUM`, but it shouldn't be an error to still specify it -- the optimizer should just remove it. For floating-point types, the summation order actually makes a difference in the result.
   
   However, I'm not sure if SQL standard says anything definitive in this matter. If not, it would be prudent to follow the results of this first-principles analysis.



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1557613644


##########
datafusion/physical-expr/src/aggregate/build_in.rs:
##########
@@ -417,7 +417,18 @@ pub fn create_aggregate_expr(
         (AggregateFunction::StringAgg, true) => {
             return not_impl_err!("STRING_AGG(DISTINCT) aggregations are not available");
         }
-    })
+    };
+
+    let agg_name = aggregate_expr.name();
+    if ignore_nulls && !aggregate_expr.support_ignore_nulls() {

Review Comment:
   I would prefer we check these at the beginning of the function, so we can avoid unnecessary computing for invalid cases



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1557613644


##########
datafusion/physical-expr/src/aggregate/build_in.rs:
##########
@@ -417,7 +417,18 @@ pub fn create_aggregate_expr(
         (AggregateFunction::StringAgg, true) => {
             return not_impl_err!("STRING_AGG(DISTINCT) aggregations are not available");
         }
-    })
+    };
+
+    let agg_name = aggregate_expr.name();
+    if ignore_nulls && !aggregate_expr.support_ignore_nulls() {

Review Comment:
   I would prefer these checks at the beginning of the function, so we can avoid unnecessary compute for invalid cases



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1557605764


##########
datafusion/sqllogictest/test_files/group_by.slt:
##########
@@ -2129,41 +2129,11 @@ query III
 1 2 1550
 1 3 2175
 
-
-# test_source_sorted_groupby2
-# If ordering is not important for the aggregation function, we should ignore the ordering requirement. Hence
-# "ORDER BY a DESC" should have no effect.
-query TT
-EXPLAIN SELECT a, d,
- SUM(c ORDER BY a DESC) as summation1
- FROM annotated_data_infinite2
- GROUP BY d, a
-----
-logical_plan
-Projection: annotated_data_infinite2.a, annotated_data_infinite2.d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST] AS summation1
---Aggregate: groupBy=[[annotated_data_infinite2.d, annotated_data_infinite2.a]], aggr=[[SUM(CAST(annotated_data_infinite2.c AS Int64)) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]]]
-----TableScan: annotated_data_infinite2 projection=[a, c, d]
-physical_plan
-ProjectionExec: expr=[a@1 as a, d@0 as d, SUM(annotated_data_infinite2.c) ORDER BY [annotated_data_infinite2.a DESC NULLS FIRST]@2 as summation1]
---AggregateExec: mode=Single, gby=[d@2 as d, a@0 as a], aggr=[SUM(annotated_data_infinite2.c)], ordering_mode=PartiallySorted([1])
-----StreamingTableExec: partition_sizes=1, projection=[a, c, d], infinite_source=true, output_ordering=[a@0 ASC NULLS LAST]
-
-query III
+statement error DataFusion error: This feature is not implemented: ORDER BY is not implemented for SUM

Review Comment:
   I think we should rewrite the test that excluded ordering for SUM in this 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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#issuecomment-2048157755

   > Thanks @huaxingao -- I agree this does what the ticket requests; However given what you have found about `SUM(ORDER BY ...)` it may make sense to permit that case 🤔
   
   this query is invalid, our parser allows it but it should fail. IGNORE NULLS have a limited functions to be worked with.  all other query engine parsers does. 


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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1557895178


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3428,3 +3428,16 @@ SELECT LAST_VALUE(column1 ORDER BY column2 DESC) IGNORE NULLS FROM t;
 
 statement ok
 DROP TABLE t;
+
+# Test for IGNORE NULLS / ORDER BY not implemented
+statement error DataFusion error: This feature is not implemented: IGNORE NULLS is not implemented for COUNT
+SELECT COUNT(*) IGNORE NULLS FROM (values (1), (null), (2));

Review Comment:
   If its doable in parser we probably may want to revert those checks/tests from DF



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1557937703


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3428,3 +3428,16 @@ SELECT LAST_VALUE(column1 ORDER BY column2 DESC) IGNORE NULLS FROM t;
 
 statement ok
 DROP TABLE t;
+
+# Test for IGNORE NULLS / ORDER BY not implemented
+statement error DataFusion error: This feature is not implemented: IGNORE NULLS is not implemented for COUNT
+SELECT COUNT(*) IGNORE NULLS FROM (values (1), (null), (2));

Review Comment:
   I agree it is probably better to check on parser. Spark throws Exception on parser if `IGNORE NULLS` is not supported. 



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#discussion_r1559727683


##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -3428,3 +3428,16 @@ SELECT LAST_VALUE(column1 ORDER BY column2 DESC) IGNORE NULLS FROM t;
 
 statement ok
 DROP TABLE t;
+
+# Test for IGNORE NULLS / ORDER BY not implemented
+statement error DataFusion error: This feature is not implemented: IGNORE NULLS is not implemented for COUNT
+SELECT COUNT(*) IGNORE NULLS FROM (values (1), (null), (2));

Review Comment:
   I assume that we should still have the checks in the physical plan, for use cases such as Comet where we are not using the DataFusion SQL parsing?



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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#issuecomment-2053649306

   Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look 


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

Re: [PR] Add support_ignore_nulls and support_ordering in Aggregate expression [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9991:
URL: https://github.com/apache/arrow-datafusion/pull/9991#issuecomment-2053746614

   > For IGNORE NULLS, I think we have consensus to error in the parser if aggregates don't support IGNORE NULLS.
   
   Sounds good to me -- we can also move the error later if we want to enforce it for the `expr_fn` API as well
   
   Thanks @huaxingao  and @ozankabak 


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