Skip to content

Commit

Permalink
Drop "funny" functions building parsers (#50715) (#50814)
Browse files Browse the repository at this point in the history
Replaces the "funny"
`Function<String, ConstructingObjectParser<T, Void>>` with a much
simpler `ConstructingObjectParser<T, String>`. This makes pretty much
all of our object parsers static.
  • Loading branch information
nik9000 committed Jan 9, 2020
1 parent f70e8f6 commit ae40e22
Show file tree
Hide file tree
Showing 12 changed files with 88 additions and 123 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public static GetRollupCapsResponse fromXContent(final XContentParser parser) th
if (token.equals(XContentParser.Token.FIELD_NAME)) {
String pattern = parser.currentName();

RollableIndexCaps cap = RollableIndexCaps.PARSER.apply(pattern).apply(parser, null);
RollableIndexCaps cap = RollableIndexCaps.PARSER.parse(parser, pattern);
jobs.put(pattern, cap);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public static GetRollupIndexCapsResponse fromXContent(final XContentParser parse
if (token.equals(XContentParser.Token.FIELD_NAME)) {
String pattern = parser.currentName();

RollableIndexCaps cap = RollableIndexCaps.PARSER.apply(pattern).apply(parser, null);
RollableIndexCaps cap = RollableIndexCaps.PARSER.apply(parser, pattern);
jobs.put(pattern, cap);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
import java.util.stream.Collectors;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;

/**
* Represents the rollup capabilities of a non-rollup index. E.g. what values/aggregations
* were rolled up for this index, in what rollup jobs that data is stored and where those
Expand All @@ -41,16 +42,15 @@
public class RollableIndexCaps implements ToXContentFragment {
private static final ParseField ROLLUP_JOBS = new ParseField("rollup_jobs");

public static final Function<String, ConstructingObjectParser<RollableIndexCaps, Void>> PARSER = indexName -> {
@SuppressWarnings("unchecked")
ConstructingObjectParser<RollableIndexCaps, Void> p
= new ConstructingObjectParser<>(indexName, true,
a -> new RollableIndexCaps(indexName, (List<RollupJobCaps>) a[0]));

p.declareObjectArray(ConstructingObjectParser.constructorArg(), RollupJobCaps.PARSER::apply,
ROLLUP_JOBS);
return p;
};
public static final ConstructingObjectParser<RollableIndexCaps, String> PARSER = new ConstructingObjectParser<>(
ROLLUP_JOBS.getPreferredName(), true, (Object[] args, String indexName) -> {
@SuppressWarnings("unchecked")
List<RollupJobCaps> caps = (List<RollupJobCaps>) args[0];
return new RollableIndexCaps(indexName, caps);
});
static {
PARSER.declareObjectArray(constructorArg(), (p, name) -> RollupJobCaps.PARSER.parse(p, null), ROLLUP_JOBS);
}

private final String indexName;
private final List<RollupJobCaps> jobCaps;
Expand Down
12 changes: 6 additions & 6 deletions server/src/main/java/org/elasticsearch/search/SearchModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ private void registerAggregations(List<SearchPlugin> plugins) {
registerAggregation(new AggregationSpec(ScriptedMetricAggregationBuilder.NAME, ScriptedMetricAggregationBuilder::new,
ScriptedMetricAggregationBuilder::parse).addResultReader(InternalScriptedMetric::new));
registerAggregation((new AggregationSpec(CompositeAggregationBuilder.NAME, CompositeAggregationBuilder::new,
CompositeAggregationBuilder::parse).addResultReader(InternalComposite::new)));
(name, p) -> CompositeAggregationBuilder.PARSER.parse(p, name)).addResultReader(InternalComposite::new)));
registerFromPlugin(plugins, SearchPlugin::getAggregations, this::registerAggregation);
}

Expand Down Expand Up @@ -540,7 +540,7 @@ private void registerPipelineAggregations(List<SearchPlugin> plugins) {
BucketScriptPipelineAggregationBuilder.NAME,
BucketScriptPipelineAggregationBuilder::new,
BucketScriptPipelineAggregator::new,
BucketScriptPipelineAggregationBuilder::parse));
(name, p) -> BucketScriptPipelineAggregationBuilder.PARSER.parse(p, name)));
registerPipelineAggregation(new PipelineAggregationSpec(
BucketSelectorPipelineAggregationBuilder.NAME,
BucketSelectorPipelineAggregationBuilder::new,
Expand All @@ -557,10 +557,10 @@ private void registerPipelineAggregations(List<SearchPlugin> plugins) {
SerialDiffPipelineAggregator::new,
SerialDiffPipelineAggregationBuilder::parse));
registerPipelineAggregation(new PipelineAggregationSpec(
MovFnPipelineAggregationBuilder.NAME,
MovFnPipelineAggregationBuilder::new,
MovFnPipelineAggregator::new,
MovFnPipelineAggregationBuilder::parse));
MovFnPipelineAggregationBuilder.NAME,
MovFnPipelineAggregationBuilder::new,
MovFnPipelineAggregator::new,
(name, p) -> MovFnPipelineAggregationBuilder.PARSER.parse(p, name)));

registerFromPlugin(plugins, SearchPlugin::getPipelineAggregations, this::registerPipelineAggregation);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.search.aggregations.AbstractAggregationBuilder;
import org.elasticsearch.search.aggregations.AggregationBuilder;
Expand All @@ -39,7 +38,8 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Function;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;

public class CompositeAggregationBuilder extends AbstractAggregationBuilder<CompositeAggregationBuilder> {
public static final String NAME = "composite";
Expand All @@ -48,27 +48,17 @@ public class CompositeAggregationBuilder extends AbstractAggregationBuilder<Comp
public static final ParseField SIZE_FIELD_NAME = new ParseField("size");
public static final ParseField SOURCES_FIELD_NAME = new ParseField("sources");

private static final Function<String, ConstructingObjectParser<CompositeAggregationBuilder, Void>> PARSER = name -> {
@SuppressWarnings("unchecked")
ConstructingObjectParser<CompositeAggregationBuilder, Void> parser = new ConstructingObjectParser<>(NAME, a -> {
CompositeAggregationBuilder builder = new CompositeAggregationBuilder(name, (List<CompositeValuesSourceBuilder<?>>)a[0]);
if (a[1] != null) {
builder.size((Integer)a[1]);
}
if (a[2] != null) {
builder.aggregateAfter((Map<String, Object>)a[2]);
}
return builder;
});
parser.declareObjectArray(ConstructingObjectParser.constructorArg(),
public static final ConstructingObjectParser<CompositeAggregationBuilder, String> PARSER = new ConstructingObjectParser<>(
NAME, false, (args, name) -> {
@SuppressWarnings("unchecked")
List<CompositeValuesSourceBuilder<?>> sources = (List<CompositeValuesSourceBuilder<?>>) args[0];
return new CompositeAggregationBuilder(name, sources);
});
static {
PARSER.declareObjectArray(constructorArg(),
(p, c) -> CompositeValuesSourceParserHelper.fromXContent(p), SOURCES_FIELD_NAME);
parser.declareInt(ConstructingObjectParser.optionalConstructorArg(), SIZE_FIELD_NAME);
parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, context) -> p.map(), AFTER_FIELD_NAME);
return parser;
};

public static CompositeAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException {
return PARSER.apply(aggregationName).parse(parser, null);
PARSER.declareInt(CompositeAggregationBuilder::size, SIZE_FIELD_NAME);
PARSER.declareObject(CompositeAggregationBuilder::aggregateAfter, (p, context) -> p.map(), AFTER_FIELD_NAME);
}

private List<CompositeValuesSourceBuilder<?>> sources;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@
import java.util.Map.Entry;
import java.util.Objects;
import java.util.TreeMap;
import java.util.function.Function;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH;
import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT;
import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.GAP_POLICY;
Expand All @@ -51,31 +51,24 @@ public class BucketScriptPipelineAggregationBuilder extends AbstractPipelineAggr
private String format = null;
private GapPolicy gapPolicy = GapPolicy.SKIP;

private static final Function<String, ConstructingObjectParser<BucketScriptPipelineAggregationBuilder, Void>> PARSER
= name -> {

@SuppressWarnings("unchecked")
ConstructingObjectParser<BucketScriptPipelineAggregationBuilder, Void> parser = new ConstructingObjectParser<>(
BucketScriptPipelineAggregationBuilder.NAME,
false,
o -> new BucketScriptPipelineAggregationBuilder(name, (Map<String, String>) o[0], (Script) o[1]));

parser.declareField(ConstructingObjectParser.constructorArg()
, BucketScriptPipelineAggregationBuilder::extractBucketPath
, BUCKETS_PATH_FIELD
, ObjectParser.ValueType.OBJECT_ARRAY_OR_STRING);
parser.declareField(ConstructingObjectParser.constructorArg(),
(p, c) -> Script.parse(p), Script.SCRIPT_PARSE_FIELD, ObjectParser.ValueType.OBJECT_OR_STRING);

parser.declareString(BucketScriptPipelineAggregationBuilder::format, FORMAT);
parser.declareField(BucketScriptPipelineAggregationBuilder::gapPolicy, p -> {
public static final ConstructingObjectParser<BucketScriptPipelineAggregationBuilder, String> PARSER = new ConstructingObjectParser<>(
NAME, false, (args, name) -> {
@SuppressWarnings("unchecked")
Map<String, String> bucketsPathsMap = (Map<String, String>) args[0];
return new BucketScriptPipelineAggregationBuilder(name, bucketsPathsMap, (Script) args[1]);
});
static {
PARSER.declareField(constructorArg(), BucketScriptPipelineAggregationBuilder::extractBucketPath,
BUCKETS_PATH_FIELD, ObjectParser.ValueType.OBJECT_ARRAY_OR_STRING);
Script.declareScript(PARSER, constructorArg());

PARSER.declareString(BucketScriptPipelineAggregationBuilder::format, FORMAT);
PARSER.declareField(BucketScriptPipelineAggregationBuilder::gapPolicy, p -> {
if (p.currentToken() == XContentParser.Token.VALUE_STRING) {
return GapPolicy.parse(p.text().toLowerCase(Locale.ROOT), p.getTokenLocation());
}
throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]");
}, GAP_POLICY, ObjectParser.ValueType.STRING);

return parser;
};


Expand Down Expand Up @@ -205,10 +198,6 @@ protected XContentBuilder internalXContent(XContentBuilder builder, Params param
return builder;
}

public static BucketScriptPipelineAggregationBuilder parse(String aggName, XContentParser parser) {
return PARSER.apply(aggName).apply(parser, null);
}

@Override
protected boolean overrideBucketsPath() {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.function.Function;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH;
import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT;
import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.GAP_POLICY;
Expand All @@ -58,29 +58,23 @@ public class MovFnPipelineAggregationBuilder extends AbstractPipelineAggregation
private int window;
private int shift;

private static final Function<String, ConstructingObjectParser<MovFnPipelineAggregationBuilder, Void>> PARSER
= name -> {

ConstructingObjectParser<MovFnPipelineAggregationBuilder, Void> parser = new ConstructingObjectParser<>(
MovFnPipelineAggregationBuilder.NAME,
false,
o -> new MovFnPipelineAggregationBuilder(name, (String) o[0], (Script) o[1], (int)o[2]));

parser.declareString(ConstructingObjectParser.constructorArg(), BUCKETS_PATH_FIELD);
parser.declareField(ConstructingObjectParser.constructorArg(),
public static final ConstructingObjectParser<MovFnPipelineAggregationBuilder, String> PARSER = new ConstructingObjectParser<>(
NAME, false,
(args, name) -> new MovFnPipelineAggregationBuilder(name, (String) args[0], (Script) args[1], (int)args[2]));
static {
PARSER.declareString(constructorArg(), BUCKETS_PATH_FIELD);
PARSER.declareField(constructorArg(),
(p, c) -> Script.parse(p), Script.SCRIPT_PARSE_FIELD, ObjectParser.ValueType.OBJECT_OR_STRING);
parser.declareInt(ConstructingObjectParser.constructorArg(), WINDOW);
PARSER.declareInt(constructorArg(), WINDOW);

parser.declareInt(MovFnPipelineAggregationBuilder::setShift, SHIFT);
parser.declareString(MovFnPipelineAggregationBuilder::format, FORMAT);
parser.declareField(MovFnPipelineAggregationBuilder::gapPolicy, p -> {
PARSER.declareInt(MovFnPipelineAggregationBuilder::setShift, SHIFT);
PARSER.declareString(MovFnPipelineAggregationBuilder::format, FORMAT);
PARSER.declareField(MovFnPipelineAggregationBuilder::gapPolicy, p -> {
if (p.currentToken() == XContentParser.Token.VALUE_STRING) {
return GapPolicy.parse(p.text().toLowerCase(Locale.ROOT), p.getTokenLocation());
}
throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]");
}, GAP_POLICY, ObjectParser.ValueType.STRING);

return parser;
};


Expand Down Expand Up @@ -212,10 +206,6 @@ protected XContentBuilder internalXContent(XContentBuilder builder, Params param
return builder;
}

public static MovFnPipelineAggregationBuilder parse(String aggName, XContentParser parser) {
return PARSER.apply(aggName).apply(parser, null);
}

/**
* Used for serialization testing, since pipeline aggs serialize themselves as a named object but are parsed
* as a regular object with the name passed in.
Expand All @@ -228,7 +218,7 @@ static MovFnPipelineAggregationBuilder parse(XContentParser parser) throws IOExc
String aggName = parser.currentName();
parser.nextToken(); // "moving_fn"
parser.nextToken(); // start_object
return PARSER.apply(aggName).apply(parser, null);
return PARSER.apply(parser, aggName);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ public void testSingleBucketPathAgg() throws Exception {
.endObject()
.endObject();
BucketScriptPipelineAggregationBuilder bucketScriptAgg =
BucketScriptPipelineAggregationBuilder.parse("seriesArithmetic", createParser(content));
BucketScriptPipelineAggregationBuilder.PARSER.parse(createParser(content), "seriesArithmetic");

SearchResponse response = client()
.prepareSearch("idx", "idx_unmapped")
Expand Down Expand Up @@ -690,7 +690,7 @@ public void testArrayBucketPathAgg() throws Exception {
.endObject()
.endObject();
BucketScriptPipelineAggregationBuilder bucketScriptAgg =
BucketScriptPipelineAggregationBuilder.parse("seriesArithmetic", createParser(content));
BucketScriptPipelineAggregationBuilder.PARSER.parse(createParser(content), "seriesArithmetic");

SearchResponse response = client()
.prepareSearch("idx", "idx_unmapped")
Expand Down Expand Up @@ -747,7 +747,7 @@ public void testObjectBucketPathAgg() throws Exception {
.endObject()
.endObject();
BucketScriptPipelineAggregationBuilder bucketScriptAgg =
BucketScriptPipelineAggregationBuilder.parse("seriesArithmetic", createParser(content));
BucketScriptPipelineAggregationBuilder.PARSER.parse(createParser(content), "seriesArithmetic");

SearchResponse response = client()
.prepareSearch("idx", "idx_unmapped")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ public void testParseBucketPath() throws IOException {
.field("lang", "expression")
.endObject()
.endObject();
BucketScriptPipelineAggregationBuilder builder1 = BucketScriptPipelineAggregationBuilder.parse("count", createParser(content));
BucketScriptPipelineAggregationBuilder builder1 = BucketScriptPipelineAggregationBuilder.PARSER.parse(
createParser(content), "count");
assertEquals(builder1.getBucketsPaths().length , 1);
assertEquals(builder1.getBucketsPaths()[0], "_count");

Expand All @@ -86,7 +87,8 @@ public void testParseBucketPath() throws IOException {
.field("lang", "expression")
.endObject()
.endObject();
BucketScriptPipelineAggregationBuilder builder2 = BucketScriptPipelineAggregationBuilder.parse("count", createParser(content));
BucketScriptPipelineAggregationBuilder builder2 = BucketScriptPipelineAggregationBuilder.PARSER.parse(
createParser(content), "count");
assertEquals(builder2.getBucketsPaths().length , 2);
assertEquals(builder2.getBucketsPaths()[0], "_count1");
assertEquals(builder2.getBucketsPaths()[1], "_count2");
Expand All @@ -99,7 +101,8 @@ public void testParseBucketPath() throws IOException {
.field("lang", "expression")
.endObject()
.endObject();
BucketScriptPipelineAggregationBuilder builder3 = BucketScriptPipelineAggregationBuilder.parse("count", createParser(content));
BucketScriptPipelineAggregationBuilder builder3 = BucketScriptPipelineAggregationBuilder.PARSER.parse(
createParser(content), "count");
assertEquals(builder3.getBucketsPaths().length , 2);
assertEquals(builder3.getBucketsPaths()[0], "_count1");
assertEquals(builder3.getBucketsPaths()[1], "_count2");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public List<PipelineAggregationSpec> getPipelineAggregations() {
CumulativeCardinalityPipelineAggregationBuilder.NAME,
CumulativeCardinalityPipelineAggregationBuilder::new,
CumulativeCardinalityPipelineAggregator::new,
CumulativeCardinalityPipelineAggregationBuilder::parse)
(name, p) -> CumulativeCardinalityPipelineAggregationBuilder.PARSER.parse(p, name))
);
}

Expand Down

0 comments on commit ae40e22

Please sign in to comment.