Switch AggregationSpec to ContextParser #50871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We seem to have settled on the `ContextParser` interface for parsing stuff, mostly because `ObjectParser` implements it. We don't really *need* the old `Aggregator.Parser` interface any more because it duplicates `ContextParser` but with the arguments reversed. This adds support to `AggregationSpec` to declare aggregation parsers using `ContextParser`. This should integrate cleanly with `ObjectParser`. It doesn't drop support for `Aggregator.Parser` or change the plugin intrface at all so it *should* be safe to backport to 7.x. And we can remove `Aggregator.Parser` in a follow up which is only targeted to 8.0.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
woops that looks like it upset the compiler. |
polyfractal
approved these changes
Jan 13, 2020
There was a problem hiding this comment.
LGTM.
@not-napoleon I don't think this affects or will-affect any of the VS refactor work, but since it's touching agg registration I thought I'd ping you just as an FYI.
|
@not-napoleon would you prefer that I hold off on merging this? |
|
Thanks for asking, but there's no reason I can see to hold off merging this for the values source stuff. I have a couple of changes in |
|
❤️ |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Jan 14, 2020
We seem to have settled on the `ContextParser` interface for parsing stuff, mostly because `ObjectParser` implements it. We don't really *need* the old `Aggregator.Parser` interface any more because it duplicates `ContextParser` but with the arguments reversed. This adds support to `AggregationSpec` to declare aggregation parsers using `ContextParser`. This should integrate cleanly with `ObjectParser`. It doesn't drop support for `Aggregator.Parser` or change the plugin intrface at all so it *should* be safe to backport to 7.x. And we can remove `Aggregator.Parser` in a follow up which is only targeted to 8.0.
nik9000
added a commit
that referenced
this pull request
Jan 14, 2020
We seem to have settled on the `ContextParser` interface for parsing stuff, mostly because `ObjectParser` implements it. We don't really *need* the old `Aggregator.Parser` interface any more because it duplicates `ContextParser` but with the arguments reversed. This adds support to `AggregationSpec` to declare aggregation parsers using `ContextParser`. This should integrate cleanly with `ObjectParser`. It doesn't drop support for `Aggregator.Parser` or change the plugin intrface at all so it *should* be safe to backport to 7.x. And we can remove `Aggregator.Parser` in a follow up which is only targeted to 8.0.
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
We seem to have settled on the `ContextParser` interface for parsing stuff, mostly because `ObjectParser` implements it. We don't really *need* the old `Aggregator.Parser` interface any more because it duplicates `ContextParser` but with the arguments reversed. This adds support to `AggregationSpec` to declare aggregation parsers using `ContextParser`. This should integrate cleanly with `ObjectParser`. It doesn't drop support for `Aggregator.Parser` or change the plugin intrface at all so it *should* be safe to backport to 7.x. And we can remove `Aggregator.Parser` in a follow up which is only targeted to 8.0.
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Mar 2, 2020
With elastic#50871 aggrgations should now be parsed directly by an `ObjectParser` or `ConstructingObjectParser` without the need for the ceremonial `parse` method. This removes 10 of those `parse` methods and parses the aggregation directly from their `ObjectParser`.
nik9000
added a commit
that referenced
this pull request
Mar 3, 2020
With #50871 aggrgations should now be parsed directly by an `ObjectParser` or `ConstructingObjectParser` without the need for the ceremonial `parse` method. This removes 10 of those `parse` methods and parses the aggregation directly from their `ObjectParser`.
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Mar 3, 2020
) With elastic#50871 aggrgations should now be parsed directly by an `ObjectParser` or `ConstructingObjectParser` without the need for the ceremonial `parse` method. This removes 10 of those `parse` methods and parses the aggregation directly from their `ObjectParser`.
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Mar 3, 2020
With elastic#50871 aggrgations should now be parsed directly by an `ObjectParser` or `ConstructingObjectParser` without the need for the ceremonial `parse` method. This removes 9 of those `parse` methods and parses the aggregation directly from their `ObjectParser`.
nik9000
added a commit
that referenced
this pull request
Mar 4, 2020
With #50871 aggrgations should now be parsed directly by an `ObjectParser` or `ConstructingObjectParser` without the need for the ceremonial `parse` method. This removes 9 of those `parse` methods and parses the aggregation directly from their `ObjectParser`.
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Mar 4, 2020
With elastic#50871 aggrgations should now be parsed directly by an `ObjectParser` or `ConstructingObjectParser` without the need for the ceremonial `parse` method. This removes 9 of those `parse` methods and parses the aggregation directly from their `ObjectParser`.
nik9000
added a commit
that referenced
this pull request
Mar 4, 2020
With #50871 aggrgations should now be parsed directly by an `ObjectParser` or `ConstructingObjectParser` without the need for the ceremonial `parse` method. This removes 9 of those `parse` methods and parses the aggregation directly from their `ObjectParser`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We seem to have settled on the
ContextParserinterface for parsingstuff, mostly because
ObjectParserimplements it. We don't reallyneed the old
Aggregator.Parserinterface any more because itduplicates
ContextParserbut with the arguments reversed.This adds support to
AggregationSpecto declare aggregation parsersusing
ContextParser. This should integrate cleanly withObjectParser. It doesn't drop support forAggregator.Parserorchange the plugin interface at all so it should be safe to backport to
7.x. And we can remove
Aggregator.Parserin a follow up which is onlytargeted to 8.0.