Document aggregation code generation #121644
Conversation
idegtiarenko
added
>non-issue
Team:Analytics
...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/package-info.java
Show resolved
Hide resolved
| * when both input type I and mutable accumulator state SS and GS are primitive (DOUBLE, INT). | ||
| * </li> | ||
| * <li>TBD explain {@code IntermediateState}</li> | ||
| * <li>TBD explain special internal state `seen`</li> |
There was a problem hiding this comment.
With the warnings feature, there's also the "failed" state. Identical to seen. Never used in main though, only here: https://github.com/elastic/elasticsearch/pull/116170/files#diff-8a408014887a6dc87eed1f71346536fc77636245d5411714b6ba2cf265812538R18
Maybe worth mentioning, with the warnExceptions attribute
| * Grouping aggregation expects: | ||
| * <ul> | ||
| * <li>type GS (a mutable state used to accumulate result of the grouping aggregation) to be public, not inner and implements {@link org.elasticsearch.compute.aggregation.GroupingAggregatorState}</li> | ||
| * <li>type I (input to your aggregation function), usually primitive types and {@link org.apache.lucene.util.BytesRef}</li> |
There was a problem hiding this comment.
I think you mean:
| * <li>type I (input to your aggregation function), usually primitive types and {@link org.apache.lucene.util.BytesRef}</li> | |
| * <li>type T (input to your aggregation function), usually primitive types and {@link org.apache.lucene.util.BytesRef}</li> |
From the following comments.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
There was a problem hiding this comment.
Looks good, thanks Ievgen!
My only observation is that we're adding specifics about how to implement the Aggregator and GroupingAggregator annotations into the package info - I think they should rather stay with the corresponding annotations. But that was already true before this PR to some extent, just that now we add even more details.
| * <h4>Before you start implementing it, please note that:</h4> | ||
| * <ul> | ||
| * <li>All methods must be public static</li> | ||
| * <li> | ||
| * init/initSingle/initGrouping could have optional BigArrays, DriverContext arguments that are going to be injected automatically. | ||
| * It is also possible to declare any number of arbitrary arguments that must be provided via generated Supplier. | ||
| * </li> | ||
| * <li> | ||
| * combine, combineStates, combineIntermediate, evaluateFinal methods (see below) could be omitted and generated automatically | ||
| * when both input type I and mutable accumulator state SS and GS are primitive (DOUBLE, INT). | ||
| * </li> | ||
| * <li> | ||
| * Code generation expects at least one IntermediateState field that is going to be used to keep | ||
| * the serialized state of the aggregation (eg AggregatorState and GroupingAggregatorState). | ||
| * It must be defined even if you rely on autogenerated implementation for the primitive types. | ||
| * </li> | ||
| * </ul> | ||
| * <h4>Aggregation expects:</h4> |
There was a problem hiding this comment.
The text here and below is very detailed and makes mentions of specific methods, parameters etc. This seems to have overlap with the javadoc of the org.elasticsearch.compute.ann.Aggregator which IMHO increases the chance of this javadoc getting quite stale, esp. if we have to make changes to the aggregator annotations.
I think it's better to say take a look at the annotation's description to see what methods they require you to implement + update the org.elasticsearch.compute.ann.Aggregator javadoc with the details provided here if the annotation's javadoc was insufficient.
There was a problem hiding this comment.
I was treating it as an entry point into aggregation function manual.
Currently it contains some sections (re optional methods, parameters and public static requirement) that are common for both @Aggregator and @GroupingAggregator. For this reason I would like to keep it here for now.
I am separately looking into if it is worth merging above annotations. If that is possible this for sure should become a java doc.
| * <h4>Aggregation expects:</h4> | ||
| * <ul> | ||
| * <li> | ||
| * type SS (a mutable state used to accumulate result of the aggregation) to be public, not inner and implements |
There was a problem hiding this comment.
nit: IMHO it's worth writing out the usual type names. I think this is normally SingleState. (Also, the abbreviation SS can have unfortunate connotations.)
| * <ul> | ||
| * <li>All methods must be public static</li> | ||
| * <li> | ||
| * init/initSingle/initGrouping could have optional BigArrays, DriverContext arguments that are going to be injected automatically. |
There was a problem hiding this comment.
nit: things like BigArrays and DriverContext could be linked to via {@link ...}. (Applies to this javadoc in general.)
| * It is also possible to declare any number of arbitrary arguments that must be provided via generated Supplier. | ||
| * </li> | ||
| * <li> | ||
| * combine, combineStates, combineIntermediate, evaluateFinal methods (see below) could be omitted and generated automatically |
There was a problem hiding this comment.
super nit: I think we usually format mentions of methods with {@code ...}. (Also applies to this javadoc in general.)
| * <h4>Grouping aggregation expects:</h4> | ||
| * <ul> | ||
| * <li> | ||
| * type GS (a mutable state used to accumulate result of the grouping aggregation) to be public, not inner and implements |
...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/package-info.java
Show resolved
Hide resolved
5 participants
Update code generation documentation with Aggs method signatures expected to be seen/used by the generated code