Add window to aggregation function

[dnhatn]

This change adds a window parameter to AggregateFunction, similar to the existing filter parameter. The window parameter is optional but must never be null. This PR also generalizes AggregateFunction to be composed of [source, field, filter, window, extra parameters] , with all extra parameters placed after filter and window.

The implementation of the window function will be added in a follow-up and will initially be available only for time-series aggregations such as rate, avg_over_time, etc.

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[elasticsearchmachine]

Pinging @elastic/es-storage-engine (Team:StorageEngine)

[dnhatn]

I wonder if this suffices for now. Can we postpone adding the window to AggregateFunction, until we generalize window functions outside TS? Or, we need to pass the window param to the outer aggs that wrap time-series aggs?

Thanks Kostas! The issue is that time-series aggregations are translated to regular aggregations (e.g., sum(avg_over_time(f, 5m)) becomes sum(avg(f))), which removes the window information. I considered introducing WindowAggregateFunction to wrap aggregation functions with a window (e.g., sum(avg_over_time(f, 5m)) becomes sum(window(avg(f), 5m))). This approach works but is fragile, as some places check aggregation function types (see

elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AbstractPhysicalOperationProviders.java

Line 293 in 793b0ae

if (aggregateFunction instanceof Count) {

) and it is much more complex than the current approach (we need to update the translation and verification rules). The current approach is similar to what we have with the inline filter. I can revise WindowAggregateFunction if we decide not to modify AggregateFunction. However, currently, the window parameter is only exposed to users for time-series aggregates, not for other aggregate functions.

[kkrik-es]

Makes sense, thanks for the detailed explanation. Window functions will be generalized down the road, so this is unavoidable either way.

[kkrik-es]

Should this be:

if (window instanceof Literal lit && lit.value() instanceof Duration duration) {
  return duration.isZero() == false;
}
return false;

[kkrik-es]

Nit: we missed filter, how important is this? Shall we backport just this to 9.2?

[dnhatn]

I will open a PR for 9.2.1.

[kkrik-es]

Nice, good progress!

[bpintea]

I suppose the need to add the window param to AggregateFunction (vs just TimeSeriesAggregateFunction) will be revealed later (possibly streaming command?).

[bpintea]

This is a dup of the one in AggregateFunction.

[dnhatn]

yes, removed it in 3ec11b9.

[bpintea]

I guess we expect functions to check against negative durations?

[dnhatn]

Yes, we should, but our random tests may pass any expression for this parameter.

[dnhatn]

@kkrik-es @bpintea Thank you so much for the quick review!