Fix empty aggregation function count() in Substrait

[gabotechs]

Which issue does this PR close?

• Closes Empty aggregation functions coming from substrait #15344

Rationale for this change

Fixing empty count() aggregation functions provided through Substrait

What changes are included in this PR?

Adding back this check https://github.com/apache/datafusion/pull/14824/files#diff-474e53672159d74dae38992a914a74eba81b8350ebe161f11d755f06414ed7b4 and testing it

Are these changes tested?

yes, by a new test and by extending previously existing tests

Are there any user-facing changes?

yes, users will be able to feed empty count() functions through the Substrait interface

[geoffreyclaude]

Could you just explain in the comment why we need count() to have arguments?

[gabotechs]

pretty much just rolled back whatever was removed in #14824, I would like some input first from @jayzhan211 about why was this removed and if it might be breaking something else that the tests are not catching

[jayzhan211]

I remove it because I don't why we need to convert count() to count(1), is there any reason we need count(1) in subtrait?

[jayzhan211]

If there is reason, it would be nice to document the reason besides the conversion

[gabotechs]

The main reason is that DataFusion does not support aggregate functions with no arguments:

datafusion/datafusion/physical-expr/src/aggregate.rs

Lines 100 to 114 in 11838be

	pub fn build(self) -> Result<AggregateFunctionExpr> {
	let Self {
	fun,
	args,
	alias,
	human_display,
	schema,
	ordering_req,
	ignore_nulls,
	is_distinct,
	is_reversed,
	} = self;
	if args.is_empty() {
	return internal_err!("args should not be empty");
	}

I imagine that this is the reason why it was there before.

If you do a similar operation and analyze DataFusion's logical plan (link), a similar Int64(1) argument is injected.

[gabotechs]

I'll add a comment explaining that

[alamb]

THank you @gabotechs and @geoffreyclaude and @jayzhan211

[alamb]

👍

[jayzhan211]

Thanks all!