Vs refactor parser cleanup by not-napoleon · Pull Request #53198 · elastic/elasticsearch

not-napoleon · 2020-03-05T22:21:10Z

This PR removes lingering hard coded type logic in ValuesSourceParserHelper, and tries to minimize the exposure of the ValueType.isA concept, which has caused some weird casting situations before.

Relates to #42949

…er-cleanup Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsAggregationBuilder.java x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/boxplot/BoxplotAggregationBuilder.java

elasticmachine · 2020-03-06T17:27:29Z

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

nik9000 · 2020-03-06T20:36:19Z

...ain/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java

    public static ObjectParser<GeoGridAggregationBuilder, Void> createParser(String name, PrecisionParser precisionParser) {
        ObjectParser<GeoGridAggregationBuilder, Void> parser = new ObjectParser<>(name);
-        ValuesSourceParserHelper.declareGeoFields(parser, false, false);
+        ValuesSourceAggregationBuilder.declareFields(parser, false, false, false);


Probably a thing for later more than now: three boolean parameters can be kind of hard to keep track of. I think IntelliJ has something where it puts the names of parameters but that isn't universal. Anyway, something else might be a little more descriptive. I'm not sure what though. It certainly can wait for later too.

I went back and forth on that. The other obvious alternative is to have a declareTimeZoneAwareField and declareTimeZoneUnawareField methods, which would replace the third boolean. But I kind of hate moving parameters into method names, so I went with this one. I agree it's hard to keep track of though. I suppose we could make it a small builder, but that seems like too much. IDK. Open to suggestions.

…er-cleanup Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/sampler/DiversifiedAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java 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/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/StatsAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/SumAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java

not-napoleon · 2020-03-09T14:22:36Z

@elasticmachine run elasticsearch-ci/2

polyfractal

LGTM!

polyfractal · 2020-03-10T17:47:38Z

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

    }
    ;

-    enum EquivalenceType {


…er-cleanup Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregationBuilder.java

…er-cleanup

…er-cleanup Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java

* 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 <polyfractal@elastic.co> Co-authored-by: Zachary Tong <zach@elastic.co> Co-authored-by: Christos Soulios <1561376+csoulios@users.noreply.github.com> Co-authored-by: Tal Levy <JubBoy333@gmail.com>

* 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 <polyfractal@elastic.co> Co-authored-by: Zachary Tong <zach@elastic.co> Co-authored-by: Christos Soulios <1561376+csoulios@users.noreply.github.com> Co-authored-by: Tal Levy <JubBoy333@gmail.com>

* 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 <polyfractal@elastic.co> Co-authored-by: Zachary Tong <zach@elastic.co> Co-authored-by: Christos Soulios <1561376+csoulios@users.noreply.github.com> Co-authored-by: Tal Levy <JubBoy333@gmail.com> * 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 <polyfractal@elastic.co> Co-authored-by: Zachary Tong <zach@elastic.co> Co-authored-by: Christos Soulios <1561376+csoulios@users.noreply.github.com> Co-authored-by: Tal Levy <JubBoy333@gmail.com>

not-napoleon added 9 commits March 5, 2020 15:33

Get rid of typed field creators

364fd48

only really need one declareFields

f187b34

No reason to keep the helper to just hold one function

494ff47

remove isA usage from MultiVSAB

901195b

rename for clarity (and shade)

50cf3c4

move isA hard coded logic into ValueType

a830d6d

Undo duplicate hack

b2f83fc

these can be private

62d6227

not-napoleon added :Analytics/Aggregations Aggregations >refactoring labels Mar 6, 2020

not-napoleon requested a review from polyfractal March 6, 2020 17:27

Fix checkstyle

b4709cc

nik9000 reviewed Mar 6, 2020

View reviewed changes

$polyfractal$

polyfractal approved these changes Mar 10, 2020

View reviewed changes

not-napoleon mentioned this pull request Mar 10, 2020

ValuesSource Refactor: move histo VSType into XPack module #53298

Merged

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

8989358

…er-cleanup Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregationBuilder.java

not-napoleon mentioned this pull request Mar 12, 2020

Refactor ValuesSource and related classes #42949

Closed

85 tasks

not-napoleon added 4 commits March 12, 2020 10:56

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

bffc774

…er-cleanup

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

0c119ba

…er-cleanup

fix AnalyticsVST complie error - forgot to add the file when merging

f82ebe3

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

not-napoleon deleted the vs-refactor-parser-cleanup branch March 13, 2020 14:30

not-napoleon restored the vs-refactor-parser-cleanup branch March 25, 2020 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vs refactor parser cleanup#53198

Vs refactor parser cleanup#53198
not-napoleon merged 16 commits intoelastic:feature/extensible-values-sourcefrom
not-napoleon:vs-refactor-parser-cleanup

not-napoleon commented Mar 5, 2020 •

edited

Loading

Uh oh!

elasticmachine commented Mar 6, 2020

Uh oh!

nik9000 Mar 6, 2020

Uh oh!

not-napoleon Mar 6, 2020

Uh oh!

not-napoleon commented Mar 9, 2020

Uh oh!

$@polyfractal$ polyfractal left a comment

Uh oh!

$@polyfractal$ polyfractal Mar 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

not-napoleon commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 6, 2020

Uh oh!

nik9000 Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

not-napoleon Mar 6, 2020

Choose a reason for hiding this comment

Uh oh!

not-napoleon commented Mar 9, 2020

Uh oh!

polyfractal left a comment

Choose a reason for hiding this comment

Uh oh!

polyfractal Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

not-napoleon commented Mar 5, 2020 •

edited

Loading

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Mar 10, 2020