ESQL: Remove create methods in aggs#144098
Conversation
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| // or more contributor license agreements. Licensed under the Elastic License | ||
| // 2.0; you may not use this file except in compliance with the Elastic License | ||
| // 2.0. | ||
| package org.elasticsearch.compute.aggregation; |
There was a problem hiding this comment.
Thanks for cleaning up these files.
| this.state = state; | ||
| } | ||
|
|
||
| public static SpatialCentroidCartesianPointDocValuesAggregatorFunction create( |
There was a problem hiding this comment.
I'm not sure why we had these create methods but they seem to be just one layer of abstraction that eventually calls the public constructor. If we short-circuit that and call the constructor directly, we just arrived at the same result from a shorter path, less code, especially if they're a nuisance to something else you're trying to achieve. This is a safe change imo, and a passing ungrouped/grouped agg function tests will seal the case for that.
| builder.addStatement("this.warnings = warnings"); | ||
| } | ||
| builder.addStatement("this.channels = channels"); | ||
| builder.addStatement("this.state = $L", initState()); |
There was a problem hiding this comment.
nit: Would be nice to make the ungrouped/grouped agg implementors, and by consequence the code they generate, as similar as possible, for comparison/learning purposes. If you agree, we can declare the class members in the same order as in AggregatorImplementer; context, then channels, then state.
There was a problem hiding this comment.
👍
Let me do that in a follow up. Because it'll be another huge PR that you can scan over. Rather than merge into this one.
…elocations * upstream/main: (49 commits) CCS logging fixes (elastic#144070) Improve CPS cluster exclusion handling (elastic#143488) Remove snapshot condition now that node_reduce phase is in non-snapshot builds (elastic#144090) Drop deprecation warnings when updating a mapping in the cluster state applier (elastic#143884) (elastic#144040) Add ensureGreenAndNoInitializingShards helper (elastic#144044) Removed unnecessary applies_to blocks from deprecated query (elastic#144096) [CPS] Use single CrossProjectModeDecider instance (elastic#144030) Fix ESQL TS requests with LIMIT 0 (elastic#144031) ESQL: Remove `create` methods in aggs (elastic#144098) ES|QL: Refactor ChangeLimitOperator (elastic#144017) Add Paginated Hit Source Tests (elastic#142592) Fix test failure not preferred (elastic#144019) Remove serialization logic from EIS authorization response (elastic#144021) ESQL: CSV schema inference and parsing enhancements (elastic#144050) ESQL: Fix incorrectly optimized fork with nullify unmapped_fields (elastic#143030) Fix MMR release test using subqueries (elastic#144087) Refactoring `UserAgentPlugin` (elastic#140712) Drop non-finite samples in Prometheus remote write (elastic#144055) [TEST] Wait for internal inference indices to be created in authorization IT (elastic#143885) Disable ndjson datasource QA tests in release-tests (elastic#143992) ...
I'm not sure what purpose they serve other than making people ask "why do we have these?" Now that AI is pretty good at writing this code, lets use it for great justice and remove this question. I've also made the ctors package private. That forces folks building them to do so through the "providers". Having "one way in" will make things easier in the work I plan to do soon. And it fits the theme of this change somewhat. The vast, vast, vast majority of the code changes here are generated code. If you want a flavor of the change look at, say, `AnyDoubleAggregationFunctionSupplier` and `AnyDoubleGroupingAggregatorFunction`.
3 participants
I'm not sure what purpose they serve other than making people ask
"why do we have these?" Now that AI is pretty good at writing this
code, lets use it for great justice and remove this question.
I've also made the ctors package private. That forces folks building
them to do so through the "providers". Having "one way in" will
make things easier in the work I plan to do soon. And it fits the
theme of this change somewhat.
The vast, vast, vast majority of the code changes here are generated
code. If you want a flavor of the change look at, say,
AnyDoubleAggregationFunctionSupplierandAnyDoubleGroupingAggregatorFunction.