Remove target value type from ValuesSourceAggregationBuilder #49943

not-napoleon · 2019-12-06T20:07:25Z

This PR does most of the work for removing targetValueType, which was one of the more confusing concepts in the ValuesSource system. There's also some adjacent cleanup that needed to happen at the same time. Summary:

Value type going into ValuesSourceConfig#resolve() now always represents something the user told us (well, except for a few spots in rollups where I wasn't sure what to do - we should clean that up either here or in another PR)
The confusingly named field valueType on ValuesSourceAggregationBuilder got renamed to the more descriptive userValueTypeHint.
- Tests that used to set targetValueType now generally set userValueTypeHint, which I think captures the intent of the test cases to test conditions where we have a type supplied?
The overrides for the ANY case, resolveMissingAny and resolveScriptAny got merged into one method defaultValueSourceType. Not sure if that should stay for the final design, but it's still needed for now at least.
ValuesSourceTypes now know their formatters directly, rather than needing to outsource that information to ValueType.
- As a consequence of this, I added specialized CoreValuesSourceTypes for dates, IPs, and booleans. This will help down the line too with matching the correct aggregator instance.
Parser type checking got more lenient, sort of. Prior to this PR we would check userValueTypeHint against the value type the parser was expecting. Problem was that check still allowed nonsense through that would fail later in the process, for example it allowed a boolean when we expected a date, which would then fail at formatter time. Supporting that check as it existed would have required pushing all of that confusion into ValuesSourceType, and we still need to change how the parsers work later in the refactor. So instead, we no longer check that condition. Nevermind, I added that back in to keep tests passing. We'll need to address how the parser works in a later update.

Related to #42949

…target-value-type Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/StatsAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java

…target-value-type

…p back

elasticmachine · 2019-12-06T20:07:26Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

…target-value-type Conflicts: qa/multi-cluster-search/src/test/java/org/elasticsearch/search/CCSDuelIT.java server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java

not-napoleon · 2019-12-17T20:47:14Z

@elasticmachine run elasticsearch-ci/default-distro

…target-value-type Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksAggregatorFactory.java server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java

not-napoleon · 2020-01-10T20:27:37Z

server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java

@@ -266,7 +266,7 @@ public static GeoTileGridAggregationBuilder geotileGrid(String name) {
     * Create a new {@link SignificantTerms} aggregation with the given name.
     */
    public static SignificantTermsAggregationBuilder significantTerms(String name) {
-        return new SignificantTermsAggregationBuilder(name, null);
+        return new SignificantTermsAggregationBuilder(name);


I'm slightly surprised to not have found a method in this collection for creating a RareTermsAggregationBuilder, but there isn't one.

Undoubtedly just because I forgot :) I always forget this helper class, since we only use it for tests ourselves and I prefer to construct objects directly instead of with the helper... which leads me to forget it :/

not-napoleon · 2020-01-10T20:43:40Z

...r/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java

@@ -44,7 +46,8 @@
    protected boolean keyed = false;

    protected AbstractRangeBuilder(String name, InternalRange.Factory<?, ?> rangeFactory) {
-        super(name, rangeFactory.getValueType());
+        // TODO: We used to set targetValueType here to rangeFactory.getValueType(), not sure if we need that information now.


This is one of the cases where we seemed to be doing something important with targetValueType, and I didn't see a direct way to capture that information in the new framework. I ended up using it for the default values source type, below, but I'm not at all sure this is correct.

Hmm. So I think the only places this affects things are stand-alone scripts, unmapped fields, and formatting for both... so the default down below should be equivalent? I think this is just an artifact of how the Range agg is structured, sharing base classes but trying to specialize parsing and output with subclassing.

not-napoleon · 2020-01-10T20:44:46Z

...n/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceAggregationBuilder.java

@@ -229,15 +229,17 @@ public GeoDistanceAggregationBuilder(String name, GeoPoint origin) {

    private GeoDistanceAggregationBuilder(String name, GeoPoint origin,
                                          InternalRange.Factory<InternalGeoDistance.Bucket, InternalGeoDistance> rangeFactory) {
-        super(name, rangeFactory.getValueType());
+        // TODO: Not sure if we need to capture rangeFactory.getValueType() here...


Same as with the AbstractRangeBuilder, it wasn't clear to me if we need to be doing something with the range type information from the range factory.

not-napoleon · 2020-01-10T21:01:14Z

.../main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java

@@ -149,18 +122,18 @@ private void read(StreamInput in) throws IOException {
    @Override
    protected final void doWriteTo(StreamOutput out) throws IOException {
        if (serializeTargetValueType(out.getVersion())) {
-            out.writeOptionalWriteable(targetValueType);
+            out.writeOptionalWriteable(null);


Based on my code analysis, we always wrote a null anyway, so this should be backwards compatible. "Should" being the key word.

Should we drop a TODO here to stop serializing it entirely in future versions?

I wouldn't do that yet, there are enough balls in the air, but it'd be good to not forget about.

I have a check box in the meta issue to come up with a deprecation plan for this, which is probably good enough for now. But yes, I'd like to return to serializeTargetValueType before we merge to master.

not-napoleon · 2020-01-10T21:04:44Z

.../main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java

     *
     * @param script - The user supplied script
     * @return The CoreValuesSourceType we expect this script to yield.
     */
-    protected ValuesSourceType resolveScriptAny(Script script) {
+    protected ValuesSourceType defaultValueSourceType(Script script) {


Bytes seems like a sensible default here, but I'm not convinced this shouldn't just be abstract and force each aggregation builder to specify it.

Yeah I was ++ default at first, but after looking through a bunch I'm leaning towards abstract and forcing each agg to implement. Half of them do anyway (most of the metrics), and it'd be easy to forget to override and might lead to weird unanticipated edge cases?

Yeah, I'm coming to that conclusion too. Kind of want to do it in a follow up PR though, since this is huge already and that'll be a lot of boiler plate.

not-napoleon · 2020-01-10T21:40:08Z

server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java

+        this.equivalenceType = equivalenceType;
+    }
+    @Override
+    public boolean isCastableTo(ValuesSourceType valuesSourceType) {


This function, along with the whole EquivalenceType enum exists to mimic the old ValueType.isA logic (and I re-implemented isA in terms of this function). The whole concept right now is pretty dodgy, since it lets you treat booleans as dates for example, but removing it entirely in this PR is too much, so I settled for making it explicit here. Definitely needs a follow up discussion, and possibly removal.

not-napoleon · 2020-01-10T21:42:22Z

server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java

-    };
+    },
+    // TODO: Ordinal Numbering sync with types from master
+    IP(EquivalenceType.STRING) {


The three new CoreValuesSourceTypes exist mosty to hold formatters, functionality migrated from ValueType. The goal is for ValueSourceType to hold all the information the aggregation needs to parse values, rather than having it spread between two or more classes.

not-napoleon · 2020-01-10T21:47:32Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java

@@ -80,8 +79,8 @@ public ValuesSourceType getValuesSourceType() {
    }

    public boolean isA(ValueType valueType) {


As you can see from the tests I added later, this is a very generous definition of equivalence. The modified version keeps the same rules, but makes it a bit more obvious what's going on, IMHO. Worth noting that the isAssignableFrom check will almost always pass if the previous clause (new or old) passes, just based on how fields are mapped to ValuesSourceTypes

not-napoleon · 2020-01-10T21:55:52Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java

            // TODO: Probably should validate that the resulting type is valid for this agg.  That needs to be plugable.
-            ValuesSourceType valuesSourceType = valueType != null ? valueType.getValuesSourceType() : CoreValuesSourceType.ANY;
+            valuesSourceType = userValueTypeHint != null ? userValueTypeHint.getValuesSourceType() : CoreValuesSourceType.ANY;


There's no reason to bounce through an ANY here; this should either be what the user specified or the default.

not-napoleon · 2020-01-10T21:59:29Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java

+                // TODO: PLAN - get rid of unmapped; it's only used by valid(), and we're intending to get rid of that.
+                // TODO:        Once we no longer care about unmapped, we can merge this case with  the mapped case.
+                valuesSourceType = userValueTypeHint != null ? userValueTypeHint.getValuesSourceType() : CoreValuesSourceType.ANY;
+                if (valuesSourceType == CoreValuesSourceType.ANY) {


Again, there's no reason I can see to bounce through an ANY here.

polyfractal

Still working through the PR, but my brain is getting mushy on a friday evening so going to leave these comments for now.

polyfractal · 2020-01-10T21:07:44Z

...rg/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java

                throw new ParsingException(p.getTokenLocation(),
                    "Aggregation [" + objectParser.getName() + "] was configured with an incompatible value type ["
-                        + valueType + "]. It can only work on value of type ["
-                        + targetValueType + "]");
+                        + valueType + "].  It can only work on value off type ["


I think this added a typo :) "of" -> "off"

polyfractal · 2020-01-10T21:36:19Z

...src/main/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsAggregationBuilder.java

@@ -81,6 +82,11 @@ protected void innerWriteTo(StreamOutput out) throws IOException {
        out.writeBoolean(wrapLongitude);
    }

+    @Override
+    protected ValuesSourceType defaultValueSourceType(Script script) {
+    return CoreValuesSourceType.GEOPOINT;


Nit: indention

polyfractal · 2020-01-10T21:41:11Z

server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java

+    }
+    ;
+
+    enum EquivalenceType {


Is this basically the less-bad version of the ValueType#isA() stuff we were discussing earlier? Do we feel this is temporary or permanent?

Reloaded and see your comment below, 👍

In summary, it's just as bad, slightly more explicit, and hopefully temporary.

polyfractal · 2020-01-10T22:06:03Z

...r/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java

@@ -44,7 +46,8 @@
    protected boolean keyed = false;

    protected AbstractRangeBuilder(String name, InternalRange.Factory<?, ?> rangeFactory) {
-        super(name, rangeFactory.getValueType());
+        // TODO: We used to set targetValueType here to rangeFactory.getValueType(), not sure if we need that information now.


Hmm. So I think the only places this affects things are stand-alone scripts, unmapped fields, and formatting for both... so the default down below should be equivalent? I think this is just an artifact of how the Range agg is structured, sharing base classes but trying to specialize parsing and output with subclassing.

polyfractal · 2020-01-10T22:08:54Z

...n/java/org/elasticsearch/search/aggregations/bucket/range/GeoDistanceAggregationBuilder.java

        this.origin = origin;
    }

    /**
     * Read from a stream.
     */
    public GeoDistanceAggregationBuilder(StreamInput in) throws IOException {
-        super(in, InternalGeoDistance.FACTORY.getValueSourceType(), InternalGeoDistance.FACTORY.getValueType());
+        super(in);


I think for this one the answer is no, since it will always be a GEOPOINT

polyfractal · 2020-01-10T22:11:11Z

.../main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java

-     * Subclasses needing to maintain backward compatibility to a version that did not serialize TargetValueType should use this
-     * constructor, providing the old, constant value for TargetValueType and override {@link #serializeTargetValueType(Version)} to return
-     * true only for versions that support the serialization.
+     * Read from a stream.


polyfractal · 2020-01-10T22:12:28Z

.../main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java

@@ -149,18 +122,18 @@ private void read(StreamInput in) throws IOException {
    @Override
    protected final void doWriteTo(StreamOutput out) throws IOException {
        if (serializeTargetValueType(out.getVersion())) {
-            out.writeOptionalWriteable(targetValueType);
+            out.writeOptionalWriteable(null);


Should we drop a TODO here to stop serializing it entirely in future versions?

I wouldn't do that yet, there are enough balls in the air, but it'd be good to not forget about.

polyfractal · 2020-01-10T22:12:58Z

.../main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java

+     * DO NOT OVERRIDE THIS!
+     *
+     * This method only exists for legacy support.  No new aggregations need this, nor should they override it.
+     *


Maybe add the @deprecated annotation too?

Actually, do any aggs override this today? Can we just nuke it?

Yes, several aggs do override this, but even before this PR, they were just serializing nulls.

polyfractal · 2020-01-10T22:13:58Z

.../main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java

        if (valueType == null) {
+            // TODO: This is nonsense.  We allow the value to be null (via constructor), but don't allow it to be set to null.  This means
+            //       thing looking to copy settings (like RollupRequestTranslator) need to check if userValueTypeHint is not null, and then


This made me laugh. And then cry.

polyfractal · 2020-01-10T22:15:03Z

.../main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java

     *
     * @param script - The user supplied script
     * @return The CoreValuesSourceType we expect this script to yield.
     */
-    protected ValuesSourceType resolveScriptAny(Script script) {
+    protected ValuesSourceType defaultValueSourceType(Script script) {


Yeah I was ++ default at first, but after looking through a bunch I'm leaning towards abstract and forcing each agg to implement. Half of them do anyway (most of the metrics), and it'd be easy to forget to override and might lead to weird unanticipated edge cases?

polyfractal

Left a question on a test, but otherwise I think this LGTM.

polyfractal · 2020-01-13T18:14:17Z

...src/test/java/org/elasticsearch/search/aggregations/bucket/nested/NestedAggregatorTests.java

@@ -763,7 +765,7 @@ public void testNestedWithPipeline() throws IOException {
            }
            try (IndexReader indexReader = wrap(DirectoryReader.open(directory))) {
                NestedAggregationBuilder nestedBuilder = new NestedAggregationBuilder(NESTED_AGG, NESTED_OBJECT)
-                    .subAggregation(new TermsAggregationBuilder("terms", ValueType.NUMERIC).field(VALUE_FIELD_NAME)
+                    .subAggregation(new TermsAggregationBuilder("terms").field(VALUE_FIELD_NAME)


Should we have a userValueTypeHint here?

polyfractal · 2020-01-13T18:22:07Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupIndexer.java

                            countBuilder.field(field);
                            builders.add(countBuilder);
                        } else if (metric.equals(MetricConfig.SUM.getPreferredName())) {
                            newBuilder = new SumAggregationBuilder(formatFieldName(field, SumAggregationBuilder.NAME, RollupField.VALUE));
                        } else if (metric.equals(MetricConfig.VALUE_COUNT.getPreferredName())) {
                            // TODO allow non-numeric value_counts.
-                            // Hardcoding this is fine for now since the job validation guarantees that all metric fields are numerics
+                            // I removed the hard coding of NUMERIC as part of cleaning up targetValueType, but I don't think  that resolves


* Remove ValuesSourceType argument to ValuesSourceAggregationBuilder (#48638) * ValuesSourceRegistry Prototype (#48758) * Remove generics from ValuesSource related classes (#49606) * fix percentile aggregation tests (#50712) * Basic thread safety for ValuesSourceRegistry (#50340) * Remove target value type from ValuesSourceAggregationBuilder (#49943) * Cleanup default values source type (#50992) * CoreValuesSourceType no longer implements Writable (#51276) * Remove genereics & hard coded ValuesSource references from Matrix Stats (#51131) * Put values source types on fields (#51503) * Remove VST Any (#51539) * Rewire terms agg to use new VS registry (#51182) Also adds some basic AggTestCases for untested code paths (and boilerplate for future tests once the IT are converted over) * Wire Cardinality aggregation to work with the ValuesSourceRegistry (#51337) * Wire Percentiles aggregator into new VS framework (#51639) This required a bit of a refactor to percentiles itself. Before, the Builder would switch on the chosen algo to generate an algo-specific factory. This doesn't work (or at least, would be difficult) in the new VS framework. This refactor consolidates both factories together and introduces a PercentilesConfig object to act as a standardized way to pass algo-specific parameters through the factory. This object is then used when deciding which kind of aggregator to create Note: CoreValuesSourceType.HISTOGRAM still lives in core, and will be moved in a subsequent PR. * Remove generics and target value type from MultiVSAB (#51647) * fix checkstyle after merge (#52008) * Plumb ValuesSourceRegistry through to QuerySearchContext (#51710) * Convert RareTerms to new VS registry (#52166) * Wire up Value Count (#52225) * Wire up Max & Min aggregations (#52219) * ValuesSource refactoring: Wire up Sum aggregation (#52571) * ValuesSource refactoring: Wire up SigTerms aggregation (#52590) * Soft immutability for VSConfig (#52729) * Unmute testSupportedFieldTypes, fix Percentiles/Ranks/Terms tests (#52734) Also fixes Percentiles which was incorrectly specified to only accept numeric, but in fact also accepts Boolean and Date (because those are numeric on master - thanks `testSupportedFieldTypes` for catching it!) * VS refactoring: Wire up stats aggregation (#52891) * ValuesSource refactoring: Wire up string_stats aggregation (#52875) * VS refactoring: Wire up median (MAD) aggregation (#52945) * fix valuesourcetype issue with constant_keyword field (#53041) this commit implements `getValuesSourceType` for the ConstantKeyword field type. master was merged into feature/extensible-values-source introducing a new field type that was not implementing `getValuesSourceType`. * ValuesSource refactoring: Wire up Avg aggregation (#52752) * Wire PercentileRanks aggregator into new VS framework (#51693) * Add a VSConfig resolver for aggregations not using the registry (#53038) * Vs refactor wire up ranges and date ranges (#52918) * Wire up geo_bounds aggregation to ValuesSourceRegistry (#53034) This commit updates the geo_bounds aggregation to depend on registering itself in the ValuesSourceRegistry relates #42949. * VS refactoring: convert Boxplot to new registry (#53132) * Wire-up geotile_grid and geohash_grid to ValuesSourceRegistry (#53037) This commit updates the geo*_grid aggregations to depend on registering itself in the ValuesSourceRegistry relates to the values-source refactoring meta issue #42949. * Wire-up geo_centroid agg to ValuesSourceRegistry (#53040) This commit updates the geo_centroid aggregation to depend on registering itself in the ValuesSourceRegistry. relates to the values-source refactoring meta issue #42949. * Fix type tests for Missing aggregation (#53501) * ValuesSource Refactor: move histo VSType into XPack module (#53298) - Introduces a new API (`getBareAggregatorRegistrar()`) which allows plugins to register aggregations against existing agg definitions defined in Core. - This moves the histogram VSType over to XPack where it belongs. `getHistogramValues()` still remains as a Core concept - Moves the histo-specific bits over to xpack (e.g. the actual aggregator logic). This requires extra boilerplate since we need to create a new "Analytics" Percentile/Rank aggregators to deal with the histo field. Doubly-so since percentiles/ranks are extra boiler-plate'y... should be much lighter for other aggs * Wire up DateHistogram to the ValuesSourceRegistry (#53484) * Vs refactor parser cleanup (#53198) Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Christos Soulios <[email protected]> Co-authored-by: Tal Levy <[email protected]>

* Remove ValuesSourceType argument to ValuesSourceAggregationBuilder (elastic#48638) * ValuesSourceRegistry Prototype (elastic#48758) * Remove generics from ValuesSource related classes (elastic#49606) * fix percentile aggregation tests (elastic#50712) * Basic thread safety for ValuesSourceRegistry (elastic#50340) * Remove target value type from ValuesSourceAggregationBuilder (elastic#49943) * Cleanup default values source type (elastic#50992) * CoreValuesSourceType no longer implements Writable (elastic#51276) * Remove genereics & hard coded ValuesSource references from Matrix Stats (elastic#51131) * Put values source types on fields (elastic#51503) * Remove VST Any (elastic#51539) * Rewire terms agg to use new VS registry (elastic#51182) Also adds some basic AggTestCases for untested code paths (and boilerplate for future tests once the IT are converted over) * Wire Cardinality aggregation to work with the ValuesSourceRegistry (elastic#51337) * Wire Percentiles aggregator into new VS framework (elastic#51639) This required a bit of a refactor to percentiles itself. Before, the Builder would switch on the chosen algo to generate an algo-specific factory. This doesn't work (or at least, would be difficult) in the new VS framework. This refactor consolidates both factories together and introduces a PercentilesConfig object to act as a standardized way to pass algo-specific parameters through the factory. This object is then used when deciding which kind of aggregator to create Note: CoreValuesSourceType.HISTOGRAM still lives in core, and will be moved in a subsequent PR. * Remove generics and target value type from MultiVSAB (elastic#51647) * fix checkstyle after merge (elastic#52008) * Plumb ValuesSourceRegistry through to QuerySearchContext (elastic#51710) * Convert RareTerms to new VS registry (elastic#52166) * Wire up Value Count (elastic#52225) * Wire up Max & Min aggregations (elastic#52219) * ValuesSource refactoring: Wire up Sum aggregation (elastic#52571) * ValuesSource refactoring: Wire up SigTerms aggregation (elastic#52590) * Soft immutability for VSConfig (elastic#52729) * Unmute testSupportedFieldTypes, fix Percentiles/Ranks/Terms tests (elastic#52734) Also fixes Percentiles which was incorrectly specified to only accept numeric, but in fact also accepts Boolean and Date (because those are numeric on master - thanks `testSupportedFieldTypes` for catching it!) * VS refactoring: Wire up stats aggregation (elastic#52891) * ValuesSource refactoring: Wire up string_stats aggregation (elastic#52875) * VS refactoring: Wire up median (MAD) aggregation (elastic#52945) * fix valuesourcetype issue with constant_keyword field (elastic#53041)x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupIndexer.java this commit implements `getValuesSourceType` for the ConstantKeyword field type. master was merged into feature/extensible-values-source introducing a new field type that was not implementing `getValuesSourceType`. * ValuesSource refactoring: Wire up Avg aggregation (elastic#52752) * Wire PercentileRanks aggregator into new VS framework (elastic#51693) * Add a VSConfig resolver for aggregations not using the registry (elastic#53038) * Vs refactor wire up ranges and date ranges (elastic#52918) * Wire up geo_bounds aggregation to ValuesSourceRegistry (elastic#53034) This commit updates the geo_bounds aggregation to depend on registering itself in the ValuesSourceRegistry relates elastic#42949. * VS refactoring: convert Boxplot to new registry (elastic#53132) * Wire-up geotile_grid and geohash_grid to ValuesSourceRegistry (elastic#53037) This commit updates the geo*_grid aggregations to depend on registering itself in the ValuesSourceRegistry relates to the values-source refactoring meta issue elastic#42949. * Wire-up geo_centroid agg to ValuesSourceRegistry (elastic#53040) This commit updates the geo_centroid aggregation to depend on registering itself in the ValuesSourceRegistry. relates to the values-source refactoring meta issue elastic#42949. * Fix type tests for Missing aggregation (elastic#53501) * ValuesSource Refactor: move histo VSType into XPack module (elastic#53298) - Introduces a new API (`getBareAggregatorRegistrar()`) which allows plugins to register aggregations against existing agg definitions defined in Core. - This moves the histogram VSType over to XPack where it belongs. `getHistogramValues()` still remains as a Core concept - Moves the histo-specific bits over to xpack (e.g. the actual aggregator logic). This requires extra boilerplate since we need to create a new "Analytics" Percentile/Rank aggregators to deal with the histo field. Doubly-so since percentiles/ranks are extra boiler-plate'y... should be much lighter for other aggs * Wire up DateHistogram to the ValuesSourceRegistry (elastic#53484) * Vs refactor parser cleanup (elastic#53198) Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Christos Soulios <[email protected]> Co-authored-by: Tal Levy <[email protected]>

* Add ValuesSource Registry and associated logic (#54281) * Remove ValuesSourceType argument to ValuesSourceAggregationBuilder (#48638) * ValuesSourceRegistry Prototype (#48758) * Remove generics from ValuesSource related classes (#49606) * fix percentile aggregation tests (#50712) * Basic thread safety for ValuesSourceRegistry (#50340) * Remove target value type from ValuesSourceAggregationBuilder (#49943) * Cleanup default values source type (#50992) * CoreValuesSourceType no longer implements Writable (#51276) * Remove genereics & hard coded ValuesSource references from Matrix Stats (#51131) * Put values source types on fields (#51503) * Remove VST Any (#51539) * Rewire terms agg to use new VS registry (#51182) Also adds some basic AggTestCases for untested code paths (and boilerplate for future tests once the IT are converted over) * Wire Cardinality aggregation to work with the ValuesSourceRegistry (#51337) * Wire Percentiles aggregator into new VS framework (#51639) This required a bit of a refactor to percentiles itself. Before, the Builder would switch on the chosen algo to generate an algo-specific factory. This doesn't work (or at least, would be difficult) in the new VS framework. This refactor consolidates both factories together and introduces a PercentilesConfig object to act as a standardized way to pass algo-specific parameters through the factory. This object is then used when deciding which kind of aggregator to create Note: CoreValuesSourceType.HISTOGRAM still lives in core, and will be moved in a subsequent PR. * Remove generics and target value type from MultiVSAB (#51647) * fix checkstyle after merge (#52008) * Plumb ValuesSourceRegistry through to QuerySearchContext (#51710) * Convert RareTerms to new VS registry (#52166) * Wire up Value Count (#52225) * Wire up Max & Min aggregations (#52219) * ValuesSource refactoring: Wire up Sum aggregation (#52571) * ValuesSource refactoring: Wire up SigTerms aggregation (#52590) * Soft immutability for VSConfig (#52729) * Unmute testSupportedFieldTypes, fix Percentiles/Ranks/Terms tests (#52734) Also fixes Percentiles which was incorrectly specified to only accept numeric, but in fact also accepts Boolean and Date (because those are numeric on master - thanks `testSupportedFieldTypes` for catching it!) * VS refactoring: Wire up stats aggregation (#52891) * ValuesSource refactoring: Wire up string_stats aggregation (#52875) * VS refactoring: Wire up median (MAD) aggregation (#52945) * fix valuesourcetype issue with constant_keyword field (#53041)x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupIndexer.java this commit implements `getValuesSourceType` for the ConstantKeyword field type. master was merged into feature/extensible-values-source introducing a new field type that was not implementing `getValuesSourceType`. * ValuesSource refactoring: Wire up Avg aggregation (#52752) * Wire PercentileRanks aggregator into new VS framework (#51693) * Add a VSConfig resolver for aggregations not using the registry (#53038) * Vs refactor wire up ranges and date ranges (#52918) * Wire up geo_bounds aggregation to ValuesSourceRegistry (#53034) This commit updates the geo_bounds aggregation to depend on registering itself in the ValuesSourceRegistry relates #42949. * VS refactoring: convert Boxplot to new registry (#53132) * Wire-up geotile_grid and geohash_grid to ValuesSourceRegistry (#53037) This commit updates the geo*_grid aggregations to depend on registering itself in the ValuesSourceRegistry relates to the values-source refactoring meta issue #42949. * Wire-up geo_centroid agg to ValuesSourceRegistry (#53040) This commit updates the geo_centroid aggregation to depend on registering itself in the ValuesSourceRegistry. relates to the values-source refactoring meta issue #42949. * Fix type tests for Missing aggregation (#53501) * ValuesSource Refactor: move histo VSType into XPack module (#53298) - Introduces a new API (`getBareAggregatorRegistrar()`) which allows plugins to register aggregations against existing agg definitions defined in Core. - This moves the histogram VSType over to XPack where it belongs. `getHistogramValues()` still remains as a Core concept - Moves the histo-specific bits over to xpack (e.g. the actual aggregator logic). This requires extra boilerplate since we need to create a new "Analytics" Percentile/Rank aggregators to deal with the histo field. Doubly-so since percentiles/ranks are extra boiler-plate'y... should be much lighter for other aggs * Wire up DateHistogram to the ValuesSourceRegistry (#53484) * Vs refactor parser cleanup (#53198) Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Christos Soulios <[email protected]> Co-authored-by: Tal Levy <[email protected]> * First batch of easy fixes * Remove List.of from ValuesSourceRegistry Note that we intend to have a follow up PR dealing with the mutability of the registry, so I didn't even try to address that here. * More compiler fixes * More compiler fixes * More compiler fixes * Precommit is happy and so am I * Add new Core VSTs to tests * Disabled supported type test on SigTerms until we can backport it's fix * fix checkstyle * Fix test failure from semantic merge issue * Fix some metaData->metadata replacements that got lost * Fix list of supported types for MinAggregator * Fix list of supported types for Avg * remove unused import Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Zachary Tong <[email protected]> Co-authored-by: Christos Soulios <[email protected]> Co-authored-by: Tal Levy <[email protected]>

not-napoleon added 17 commits November 19, 2019 10:05

Remove unused ValuesSource argument from LeafOnly aggregators

a7a6fd6

Merge branch 'feature/extensible-values-source' into refactor/remove-…

a8abf8e

…target-value-type

Always use user supplied value type

166dfb7

rename and java doc for clarity

34d2aa8

Start using resolveScriptAny to fix failing tests

1ed4f9c

Fix some more failing tests

d469d31

"Fix" terms aggregation test

057d152

Fix formatters for IPRange, broke boolean. Two steps forward, one ste…

b76e26a

…p back

ValueType.isA is nonsense

9e446c4

remove ValueType isA

02dae81

remove a bunch of dead code

b76e9f8

Rename resolveScriptAny to reflect its new role

4587deb

Remove uses of TVT from ValueCount

a580287

Remove uses of TVT from Terms family aggs

75f3845

Remove uses of TVT from Cardinality

ec01f46

Remove TVT from ValuesSourceAggregationBuilder

0fd4f5e

not-napoleon added :Analytics/Aggregations Aggregations >refactoring labels Dec 6, 2019

Remove unused constructor args

f93ce6b

not-napoleon requested a review from polyfractal December 6, 2019 20:15

not-napoleon added 8 commits December 6, 2019 16:08

Renamed a few things in ValuesSourceConfig, for clarity

3a4a8dc

minor cleanups

f848b5c

defaultValuesSourceType for ExtendedStats

a9b04c5

defaultValuesSourceType for Min

73a37e3

defaultValuesSourceType for Stats

05fee9f

Fix NPE in RollupRequestTranslator

e0e532c

Fix more defaultValuesSourceTypes

40abc94

Fix range aggs tests

0c49609

not-napoleon added 6 commits December 11, 2019 10:16

TODO notes for future me

c4c8a08

fix default type errors

206b0ad

Merge branch 'feature/extensible-values-source' into refactor/remove-…

9e7a47b

…target-value-type Conflicts: qa/multi-cluster-search/src/test/java/org/elasticsearch/search/CCSDuelIT.java server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java

fix default type for date histogram

bca57bb

Fix type checking on percentile & percentile rank aggs

87f8fad

more default ValuesSourceType fixes

7a5c96a

not-napoleon commented Jan 10, 2020

View reviewed changes

$polyfractal$

polyfractal reviewed Jan 10, 2020

View reviewed changes

not-napoleon added 2 commits January 13, 2020 11:01

Clean up useless use of ANY

3626c31

minor cleanups

f2d0942

$polyfractal$

polyfractal approved these changes Jan 13, 2020

View reviewed changes

missed a spot for setting userValueTypeHint

a459321

not-napoleon merged commit c44db79 into elastic:feature/extensible-values-source Jan 13, 2020

not-napoleon deleted the refactor/remove-target-value-type branch January 13, 2020 21:25

not-napoleon mentioned this pull request Jan 14, 2020

Cleanup default values source type #50992

Merged

		@@ -80,8 +79,8 @@ public ValuesSourceType getValuesSourceType() {
		}

		public boolean isA(ValueType valueType) {

Remove target value type from ValuesSourceAggregationBuilder #49943

Remove target value type from ValuesSourceAggregationBuilder #49943

Conversation

not-napoleon commented Dec 6, 2019 • edited Loading

elasticmachine commented Dec 6, 2019

not-napoleon commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-napoleon Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-napoleon commented Dec 6, 2019 •

edited

Loading

not-napoleon Jan 10, 2020 •

edited

Loading

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal left a comment