Add MultiValuesSource support to Aggregation Framework #18285
Conversation
|
@colings86, would you mind taking a look at this if you get a chance? It takes the initial work you did with MultiValuesSource but removes the code duplication. |
nknize
force-pushed
the
feature/MultiValuesSource_Agg_Support
branch
from
9fe180a to
0079dcc
Compare
| return false; | ||
| } | ||
| } | ||
| } else if ("field".equals(currentFieldName)) { |
There was a problem hiding this comment.
Can we use ParseField here? In fact, could we take this opportunity to update the SingleValuesSourceParser and ValuesSourceParser to use ParseField too, as well as their builder toXContent methods?
|
@nknize I left some comments but I like what you have so far |
Refactor ValuesSourceAggregatorBuilder, Factory, and Parser as base classes to new {Single | Multi} ValuesSourceAggregatorBuilder, Factory, and Parser classes. Refactor existing single value aggregations to extend SingleValuesSourceAggregator support classes.
…ommon code from read and createBuilder methods to base class.
nknize
force-pushed
the
feature/MultiValuesSource_Agg_Support
branch
from
0079dcc to
49760ab
Compare
|
Thanks @colings86. First round review changes have been made. It looks like there are a lot of duplicate ParseField static instances like: |
| public static final ParseField FORMAT_FIELD = new ParseField("format"); | ||
| public static final ParseField MISSING_FIELD = new ParseField("missing"); | ||
| public static final ParseField TIME_ZONE_FIELD = new ParseField("time_zone"); | ||
| public static final ParseField VALUE_TYPE_FIELD = new ParseField("value_type", "valueType"); |
There was a problem hiding this comment.
I'm pretty sure camelCase shouldn't be supported any more.
There was a problem hiding this comment.
+1 but I would check with @clintongormley whether we are happy making this breaking change in 5.0. However, we haven't actually formally removed camelCase in ParseField IIRC so adding this extra name here might be unnecessary anyway
There was a problem hiding this comment.
It was removed from ParseField in #17933. It also shouldn't be added here.
There was a problem hiding this comment.
Ok great, thanks for the correction @rjernst
There was a problem hiding this comment.
I set camelCase valueType here because it wasn't removed from the original parser: https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/search/aggregations/support/AbstractValuesSourceParser.java#L121
I thought it best to set as deprecated here but based on the feedback I presume its OK to simply remove it?
|
@nknize thanks for making the changes. This looks pretty good now. I left a comment about where the ParseField constants are put because I don't think that they are in the right place. Also, I know we are going to have an implementation of a MultiValuesSourceAgg in a module but since the infrastructure is in core I think we should at least do basic tests on the infrastructure to make sure it works in core rather than relying on the module. Maybe we should have a Unit test (that extends BaseAggregationTestCase) and an integration test that register a mock or very simple MultiValueSource aggregation and then tests to make sure all the bits of the multi source agg infrastructure works? So in the unit test it checks that the MultiValuesSourceAggregatorBuilder parses and renders to the same thing, hashCode and equals works and the wire format works correctly, and for the integration test it makes sure the options set in the builder are propogated properly through to the aggregator and it returns something sensible? Alternatively to the integration test maybe we just need unit tests for the Parser and Factory? |
|
After talking with @colings86 and @jpountz we've decided to close this PR in favor of the following:
|
This PR refactors core ValuesSourceAggregatorBuilder, Factory, and Parser as base classes to new {Single | Multi} ValuesSourceAggregatorBuilder, Factory, and Parser classes. This provides support for developing new aggregations that operate on multiple fields as is needed by the MultiFieldStats aggregation discussed in #16817.
Existing single field aggregations are refactored to extend SingleValuesSourceAggregator and a follow on PR will add a new matrix aggregation module which includes the MultiFieldStats aggregation reviewed in PR #16826.