Skip to content

Commit

Permalink
Switch AggregationSpec to ContextParser (#50871)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nik9000 authored Jan 14, 2020
1 parent a5b462c commit 5809912
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 6 deletions.
39 changes: 36 additions & 3 deletions server/src/main/java/org/elasticsearch/plugins/SearchPlugin.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.lucene.search.function.ScoreFunction;
import org.elasticsearch.common.xcontent.ContextParser;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.QueryBuilder;
Expand Down Expand Up @@ -248,7 +249,7 @@ public QuerySpec(String name, Writeable.Reader<T> reader, QueryParser<T> parser)
/**
* Specification for an {@link Aggregation}.
*/
class AggregationSpec extends SearchExtensionSpec<AggregationBuilder, Aggregator.Parser> {
class AggregationSpec extends SearchExtensionSpec<AggregationBuilder, ContextParser<String, ? extends AggregationBuilder>> {
private final Map<String, Writeable.Reader<? extends InternalAggregation>> resultReaders = new TreeMap<>();

/**
Expand All @@ -261,7 +262,8 @@ class AggregationSpec extends SearchExtensionSpec<AggregationBuilder, Aggregator
* {@link StreamInput}
* @param parser the parser the reads the aggregation builder from xcontent
*/
public AggregationSpec(ParseField name, Writeable.Reader<? extends AggregationBuilder> reader, Aggregator.Parser parser) {
public <T extends AggregationBuilder> AggregationSpec(ParseField name, Writeable.Reader<T> reader,
ContextParser<String, T> parser) {
super(name, reader, parser);
}

Expand All @@ -274,10 +276,41 @@ public AggregationSpec(ParseField name, Writeable.Reader<? extends AggregationBu
* {@link StreamInput}
* @param parser the parser the reads the aggregation builder from xcontent
*/
public AggregationSpec(String name, Writeable.Reader<? extends AggregationBuilder> reader, Aggregator.Parser parser) {
public <T extends AggregationBuilder> AggregationSpec(String name, Writeable.Reader<T> reader, ContextParser<String, T> parser) {
super(name, reader, parser);
}

/**
* Specification for an {@link Aggregation}.
*
* @param name holds the names by which this aggregation might be parsed. The {@link ParseField#getPreferredName()} is special as it
* is the name by under which the reader is registered. So it is the name that the {@link AggregationBuilder} should return
* from {@link NamedWriteable#getWriteableName()}.
* @param reader the reader registered for this aggregation's builder. Typically a reference to a constructor that takes a
* {@link StreamInput}
* @param parser the parser the reads the aggregation builder from xcontent
* @deprecated Use the ctor that takes a {@link ContextParser} instead
*/
@Deprecated
public AggregationSpec(ParseField name, Writeable.Reader<? extends AggregationBuilder> reader, Aggregator.Parser parser) {
super(name, reader, (p, aggName) -> parser.parse(aggName, p));
}

/**
* Specification for an {@link Aggregation}.
*
* @param name the name by which this aggregation might be parsed or deserialized. Make sure that the {@link AggregationBuilder}
* returns this from {@link NamedWriteable#getWriteableName()}.
* @param reader the reader registered for this aggregation's builder. Typically a reference to a constructor that takes a
* {@link StreamInput}
* @param parser the parser the reads the aggregation builder from xcontent
* @deprecated Use the ctor that takes a {@link ContextParser} instead
*/
@Deprecated
public AggregationSpec(String name, Writeable.Reader<? extends AggregationBuilder> reader, Aggregator.Parser parser) {
super(name, reader, (p, aggName) -> parser.parse(aggName, p));
}

/**
* Add a reader for the shard level results of the aggregation with {@linkplain #getName}'s {@link ParseField#getPreferredName()} as
* the {@link NamedWriteable#getWriteableName()}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -422,16 +422,16 @@ private void registerAggregations(List<SearchPlugin> plugins) {
registerAggregation(new AggregationSpec(GeoCentroidAggregationBuilder.NAME, GeoCentroidAggregationBuilder::new,
GeoCentroidAggregationBuilder::parse).addResultReader(InternalGeoCentroid::new));
registerAggregation(new AggregationSpec(ScriptedMetricAggregationBuilder.NAME, ScriptedMetricAggregationBuilder::new,
(name, p) -> ScriptedMetricAggregationBuilder.PARSER.parse(p, name)).addResultReader(InternalScriptedMetric::new));
ScriptedMetricAggregationBuilder.PARSER).addResultReader(InternalScriptedMetric::new));
registerAggregation((new AggregationSpec(CompositeAggregationBuilder.NAME, CompositeAggregationBuilder::new,
(name, p) -> CompositeAggregationBuilder.PARSER.parse(p, name)).addResultReader(InternalComposite::new)));
CompositeAggregationBuilder.PARSER).addResultReader(InternalComposite::new)));
registerFromPlugin(plugins, SearchPlugin::getAggregations, this::registerAggregation);
}

private void registerAggregation(AggregationSpec spec) {
namedXContents.add(new NamedXContentRegistry.Entry(BaseAggregationBuilder.class, spec.getName(), (p, c) -> {
String name = (String) c;
return spec.getParser().parse(name, p);
return spec.getParser().parse(p, name);
}));
namedWriteables.add(
new NamedWriteableRegistry.Entry(AggregationBuilder.class, spec.getName().getPreferredName(), spec.getReader()));
Expand Down

0 comments on commit 5809912

Please sign in to comment.