Make ValuesSourceConfig behave like a config object #57762
Conversation
not-napoleon
added
:Analytics/Aggregations
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
elasticmachine
added
the
Team:Analytics
| "value source config is invalid; must have either a field context or a script or marked as unwrapped"); | ||
| } | ||
|
|
||
| final ValuesSource vs; |
There was a problem hiding this comment.
This'd probably be a little cleaner as a private final method with early returns.
server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java
Show resolved
Hide resolved
| config.missing(missing); | ||
| config.timezone(timeZone); | ||
| DocValueFormat docValueFormat = resolveFormat(format, valuesSourceType, timeZone, fieldType); | ||
| config = new ValuesSourceConfig(valuesSourceType, fieldContext, unmapped, aggregationScript, scriptValueType, missing, |
There was a problem hiding this comment.
Run it through the code-formatter?
| private final Object missing; | ||
| private final ZoneId timeZone; | ||
| private final LongSupplier nowSupplier; | ||
| private ValuesSource valuesSource; |
server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java
Show resolved
Hide resolved
| AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config, | ||
| DateHistogramAggregationBuilder.NAME); | ||
| if (aggregatorSupplier instanceof DateHistogramAggregationSupplier == false) { | ||
| throw new AggregationExecutionException("Registry miss-match - expected DateHistogramAggregationSupplier, found [" + | ||
| aggregatorSupplier.getClass().toString() + "]"); | ||
| } | ||
| Rounding.Prepared preparedRounding = valuesSource.roundingPreparer(queryShardContext.getIndexReader()).apply(shardRounding); | ||
| // TODO: Is there a reason not to get the prepared rounding in the supplier itself? |
There was a problem hiding this comment.
👍 to getting the prepared rounding in the ctor.
| -> new GeoShapeBoundsAggregator(name, aggregationContext, parent, (GeoShapeValuesSource) valuesSource, | ||
| wrapLongitude, metadata)); | ||
| (GeoBoundsAggregatorSupplier) (name, aggregationContext, parent, valuesSourceConfig, wrapLongitude, metadata) | ||
| -> new GeoShapeBoundsAggregator(name, aggregationContext, parent, valuesSourceConfig, wrapLongitude, metadata)); |
There was a problem hiding this comment.
Can this be a ctor reference like the others? It looks like it from here.
| } | ||
|
|
||
| private void registerGeoShapeCentroidAggregator(ValuesSourceRegistry.Builder builder) { | ||
| builder.register(GeoCentroidAggregationBuilder.NAME, GeoShapeValuesSourceType.instance(), | ||
| (GeoCentroidAggregatorSupplier) (name, aggregationContext, parent, valuesSource, metadata) | ||
| (MetricAggregatorSupplier) (name, valuesSourceConfig, aggregationContext, parent, metadata) |
There was a problem hiding this comment.
I think it'd be a little cleaner to make a "wrapper" MetricAggregatorSupplier that checks the license state and calls a ctor reference if it is allowed.
There was a problem hiding this comment.
I'm +0 on this right now. It doesn't come up that often, and I'm not sure the wrapper actually makes it any clearer. If you feel strongly, I can do it (or would be happy to review your version if you like), but I don't think it's a big gain over what we have now personally.
There was a problem hiding this comment.
If we only do it once I guess it doesn't buy much. I think it'd be a little clearer that we're just checking the license and then doing the ctor reference thing, but its no big deal.
| GeoCentroidAggregatorSupplier centroidSupplier = (GeoCentroidAggregatorSupplier) registry.getAggregator( | ||
| new ValuesSourceConfig(GeoShapeValuesSourceType.instance(), null, false, null, null, null), | ||
| MetricAggregatorSupplier centroidSupplier = (MetricAggregatorSupplier) registry.getAggregator( | ||
| new ValuesSourceConfig(GeoShapeValuesSourceType.instance(), null, true, null, null, null, null, null, null), |
There was a problem hiding this comment.
This is a super long number of arguments! I'm glad we don't call the ctor very much though.
There was a problem hiding this comment.
Yeah, outside of testing, the only thing that should be calling that constructor is the various resolve factory methods, which are what the rest of the framework should call. Not that they don't have a lot of arguments too. I suppose we could have a default constructor for tests, but I don't like adding production code that's only supposed to be called from tests.
There was a problem hiding this comment.
How often do we call it in tests? It doesn't look like we call it directly very often. I think mostly we use the production resolution framework.
There was a problem hiding this comment.
Correct, there's only a handful of cases we call the constructor from tests. I meant I didn't want to add a new test-only constructor just to clean up this case.
…on/elasticsearch into vs-refactor-vs-config-cleanup Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/SignificantTermsAggregatorFactory.java
This PR has two major changes for how ValuesSourceConfig works. First, it makes VSConfig immutable. It had been effectively immutable already, in that nothing outside of its factory methods called its mutators, but they still existed. Now, it's obvious at a glance that the class is immutable, which should make it easier to reason about, and to pass around without worry.
A consequence of that change, a lot of places where we previously picked out parts of VSConfig to pass into aggregators, we now just pass the whole config in. This simplifies constructor arguments, lets us merge some supplier interfaces together, and makes it easier for future developers to add aggregators that might need some part of the config we hadn't used before - it no longer requires changing a bunch of signatures up the call chain to add a new parameter.
The second major change is that VSConfig no longer creates null values sources. Previously, we'd used a null ValuesSource to communicate that there were no values (aka the unmapped-and-no-missing case). The aggregation builder would detect this and call
createUnmappedinstead ofdoCreateInternalon the aggregator factory. But since the field was nullable, a lot of aggregators have to carry around support for getting passed a null values source, even in cases where they never would be. Other places, we effectively have a parallel aggregator in the same class for nulls. In a follow-up PR, we can remove all of that needless null handling and just create proper, optimized, empty aggregators for thecreateUnmappedpath to return.Besides these major changes, the rest of this PR plumbs through the low hanging fruit of passing the VSConfig directly into aggregators. Not all aggregators were converted, because some of the more complicated ones I'd rather do in a smaller PR.