Skip to content

Conversation

@idegtiarenko
Copy link
Contributor

@idegtiarenko idegtiarenko commented Feb 6, 2025

This updates code generation to require that State implements AggregatorState/GroupingAggregatorState as we rely on their methods in generated code.

All aggs states are not explicitly implement them.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems good!

@idegtiarenko idegtiarenko changed the title Enforce aggs interface interface Enforce aggs interface Feb 10, 2025
@idegtiarenko idegtiarenko marked this pull request as ready for review February 10, 2025 08:50
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Feb 10, 2025
@idegtiarenko idegtiarenko added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Feb 10, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Feb 10, 2025
Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

declarationType,
// This should be more restrictive and require org.elasticsearch.compute.aggregation.AggregatorState
requirePrimitiveOrImplements(elements, Types.RELEASABLE),
requirePrimitiveOrImplements(elements, ClassName.get("org.elasticsearch.compute.aggregation", "AggregatorState")),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move those to the Types constants class? To follow what we do with other types.
Same with the GroupingAggregatorState one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

declarationType,
// This should be more restrictive and require org.elasticsearch.compute.aggregation.AggregatorState
requirePrimitiveOrImplements(elements, Types.RELEASABLE),
requirePrimitiveOrImplements(elements, ClassName.get("org.elasticsearch.compute.aggregation", "AggregatorState")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@idegtiarenko idegtiarenko merged commit a57b985 into elastic:main Feb 12, 2025
17 checks passed
@idegtiarenko idegtiarenko deleted the enforce_interface branch February 12, 2025 08:18
idegtiarenko added a commit to idegtiarenko/elasticsearch that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants