Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments

[alamb]

Describe the bug

DataFusion now supports providing ordering and nulls information to aggregates, but some aggregates ignore the flags silently

For example

❯ 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

❯ 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

❯ 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

❯ select count(*) IGNORE NULLS from (values (1), (null), (2));

To error with "IGNORE NULLS is not supported

I also expect

❯ 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 #9920 (comment) about how to make checking this easier / harder to miss

[comphead]

@huaxingao this might be interesting

[huaxingao]

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

[mustafasrepo]

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.

[alamb]

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 🤔 (as DataFusion would have to sort the input)

However, we could remove ORDER BY that don't change the output as a follow on optimization

[alamb]

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

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

[alamb]

I wrote some tests for this feature in another PR that we decided not to use. I put them here #9953 in case that is helpful @huaxingao

[mustafasrepo]

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

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

[vbarua]

I opened up an issue that's potentially related to this when it comes to supporting IGNORE NULLS
#15006