Some aggregates silently ignore `IGNORE NULLS` and `ORDER BY` on arguments about arrow-datafusion HOT 7 OPEN

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

from arrow-datafusion.

@huaxingao this might be interesting

from arrow-datafusion.

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

from arrow-datafusion.

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.

from arrow-datafusion.

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

from arrow-datafusion.

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 }

from arrow-datafusion.

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

from arrow-datafusion.