Update aggs code generation to be more explicit about required methods #121749
Conversation
idegtiarenko
added
>non-issue
Team:Analytics
# Conflicts: # x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorImplementer.java # x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/GroupingAggregatorImplementer.java # x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/Types.java
idegtiarenko
force-pushed
the
aggs_code_generation
branch
from
080b6c3 to
a7042e6
Compare
idegtiarenko
force-pushed
the
aggs_code_generation
branch
from
a7042e6 to
be957db
Compare
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| this.init = findRequiredMethod(declarationType, new String[] { "init", "initSingle" }, e -> true); | ||
| this.init = requireStaticMethod( | ||
| declarationType, | ||
| // This should be more restrictive and require org.elasticsearch.compute.aggregation.AggregatorState |
There was a problem hiding this comment.
It is enforced in 57d34e4 please let me know if that should be merged as well.
There was a problem hiding this comment.
That commit looks good to me. It exists already right? So enforcing it looks fine
| this.init = findRequiredMethod(declarationType, new String[] { "init", "initSingle" }, e -> true); | ||
| this.init = requireStaticMethod( | ||
| declarationType, | ||
| // This should be more restrictive and require org.elasticsearch.compute.aggregation.AggregatorState |
There was a problem hiding this comment.
That commit looks good to me. It exists already right? So enforcing it looks fine
| .filter(method -> nameMatcher.test(method.getSimpleName().toString())) | ||
| .filter(method -> returnTypeMatcher.test(TypeName.get(method.getReturnType()))) | ||
| .filter(method -> argumentMatcher.test(method.getParameters().stream().map(it -> TypeName.get(it.asType())).toList())) | ||
| .findFirst() |
There was a problem hiding this comment.
Should we also verify that there's only 1 method with such declaration? As to avoid overloads here. Just a little extra check that could avoid potential problems, for example, when copypasting grouping/non-grouping methods.
Not sure if there's any case where overloads are required though. Maybe we could make another require...() method/overload for those specific cases
There was a problem hiding this comment.
I do not think that such situation is possible. We would have a compilation error (before code generation) when trying to compile a class with 2 methods with exactly the same signature.
With init we have a bit more relaxed arg condition as we allow any arguments. In this case I imagine it might be valid to have something like:
public static SingleState init() { return init(DEFAULT_VALUE); }
public static SingleState init(int defaultValue) { ... }
Although it is not clear what implementation is going to be picked by the code generation then.
There was a problem hiding this comment.
I'll think how to handle this. Thanks for the pointer!
There was a problem hiding this comment.
I tried to change it and currently it fails because of:
As it has 2 signatures matching:
void SumFloatAggregator#combine(SumState current, float v)
void SumDoubleAggregator#combine(SumState current, double v)
May be we should not extend SumFloatAggregator from SumDoubleAggregator.
There is another collision:
This usage is valid. We are using first combine to add column value to accumulator and a second one to combine 2 accumulators.
I would like not to change it and delay unique signature check for now.
idegtiarenko
added
auto-backport
💔 Backport failed
You can use sqren/backport to manually backport by running |
elastic#121749) (cherry picked from commit 7deaacd)
4 participants
This introduces various changes to make it easier to understand error output from Aggs code generation.
For example missing
MaxBytesRefAggregator#combineIntermediatenow results induring code generation. Previously it resulted in multiple code compilation errors in generated code without any hints on how to resolve them.