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

[I] Aggregates ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

alamb opened a new issue, #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924

   ### Describe the bug
   
   DataFusion now supports providing ordering and nulls information to aggregates
   
   For example
   ```sql
   ❯ select first_value(column1 ORDER BY column2) FROM (values (1,2), (3,4), (-1,0)) ;
   +----------------------+
   | FIRST_VALUE(column1) |
   +----------------------+
   | -1                   |
   +----------------------+
   
   ❯ select first_value(column1 ORDER BY column2) IGNORE NULLS FROM (values (1,2), (3,4), (null,0)) ;
   +----------------------+
   | FIRST_VALUE(column1) |
   +----------------------+
   | 1                    |
   +----------------------+
   1 row in set. Query took 0.002 seconds.
   ```
   
   
   ### To Reproduce
   
   ```sql
   ❯ select count(*) from (values (1), (null), (2));
   +----------+
   | COUNT(*) |
   +----------+
   | 3        |
   +----------+
   1 row in set. Query took 0.039 seconds.
   
   ❯ select count(*) IGNORE NULLS from (values (1), (null), (2));
   +----------+
   | COUNT(*) |
   +----------+
   | 3        |
   +----------+
   1 row in set. Query took 0.001 seconds.
   ```
   
   Also, for ordering
   
   ```sql
   ❯ select avg(column1 ORDER BY column2) FROM (values (1,2), (3,4), (null,0)) ;
   +--------------+
   | AVG(column1) |
   +--------------+
   | 2.0          |
   +--------------+
   1 row in set. Query took 0.008 seconds.
   ```
   
   ### Expected behavior
   
   I expect
   
   ```sql
   ❯ select count(*) IGNORE NULLS from (values (1), (null), (2));
   ```
   
   To error with "IGNORE NULLS is not supported
   
   I also expect
   
   ```sql
   ❯ select avg(column1 ORDER BY column2) FROM (values (1,2), (3,4), (null,0)) ;
   ```
   
   to error with "ORDER BY" not supported for avg
   
   ### Additional context
   
   @jayzhan211 has some good ideas at https://github.com/apache/arrow-datafusion/pull/9920#discussion_r1549603208 about  how to make checking this easier / harder to miss


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

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

Re: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on issue #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924#issuecomment-2036329085

   I think for `AVG`, or `COUNT` type of aggregates we can safely ignore `ORDER BY` expression. Since operation result is same for any permutation of the input. However, for `first_value`, `nth_value`, `last_value` this is not the case. However, I agree that `IGNORE NULLS` should raise 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

Re: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [datafusion]

Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on issue #9924:
URL: https://github.com/apache/datafusion/issues/9924#issuecomment-2698960345

   I opened up an issue that's potentially related to this when it comes to supporting IGNORE NULLS
   https://github.com/apache/datafusion/issues/15006


-- 
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: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on issue #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924#issuecomment-2039021866

   > FWIW @jayzhan211 has a really nice API suggestion here I think https://github.com/apache/arrow-datafusion/pull/9920/files#r1549905825
   > 
   > Specifically, add these two functions to the `AggregateUDFImpl`
   > 
   > ```rust
   > /// Return true if the aggregate supports `IGNORE NULL`s
   > fn support_ignore_nulls() -> bool  { false }
   > 
   > /// Return true if the aggregate supports and `ORDER BY` to specify its ordering:
   > /// `SELECT first_value(x ORDER BY y) ... `
   > fn support_ordering() -> bool { false }
   > ```
   
   I think these APIs are very nice, clear. We can add these supports


-- 
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: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on issue #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924#issuecomment-2034983786

   @comphead Thanks for pinging me! I will work on this issue.


-- 
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: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on issue #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924#issuecomment-2034913180

   @huaxingao this might be interesting


-- 
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: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924#issuecomment-2037421727

   FWIW @jayzhan211 has a really nice API suggestion here I think https://github.com/apache/arrow-datafusion/pull/9920/files#r1549905825
   
   Specifically, add these two functions to the `AggregateUDFImpl`
   
   ```rust
   /// Return true if the aggregate supports `IGNORE NULL`s
   fn support_ignore_nulls() -> bool  { false }
   
   /// Return true if the aggregate supports and `ORDER BY` to specify its ordering:
   /// `SELECT first_value(x ORDER BY y) ... `
   fn support_ordering() -> bool { false }
   ```


-- 
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: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924#issuecomment-2038000280

   I wrote some tests for this feature in another PR that we decided not to use. I put them here https://github.com/apache/arrow-datafusion/pull/9953 in case that is helpful @huaxingao 


-- 
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: [I] Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments [arrow-datafusion]

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #9924:
URL: https://github.com/apache/arrow-datafusion/issues/9924#issuecomment-2037406494

   > I think for AVG, or COUNT type of aggregates we can safely ignore ORDER BY expression. Since operation result is same for any permutation of the input.
   
   That is true, though applying an ORDER BY to the argument of COUNT may be much slower 🤔  But maybe we could remove `ORDER BY` that don't change the output as a follow on optimization


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