Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vs refactor parser cleanup #53198

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.AbstractObjectParser;
import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.ToXContent.Params;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.ToXContent.Params;
import org.elasticsearch.script.Script;
import org.elasticsearch.search.aggregations.support.ValueType;

Expand All @@ -37,14 +37,15 @@
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;

public class CompositeValuesSourceParserHelper {

static <VB extends CompositeValuesSourceBuilder<VB>, T> void declareValuesSourceFields(AbstractObjectParser<VB, T> objectParser,
ValueType expectedValueType) {
objectParser.declareField(VB::field, XContentParser::text,
new ParseField("field"), ObjectParser.ValueType.STRING);
objectParser.declareBoolean(VB::missingBucket, new ParseField("missing_bucket"));

objectParser.declareField(VB::valueType, p -> {
ValueType valueType = ValueType.resolveForScript(p.text());
ValueType valueType = ValueType.lenientParse(p.text());
if (expectedValueType != null && valueType.isNotA(expectedValueType)) {
throw new ParsingException(p.getTokenLocation(),
"Aggregation [" + objectParser.getName() + "] was configured with an incompatible value type ["
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

import java.io.IOException;
Expand All @@ -65,7 +64,7 @@ protected interface PrecisionParser {

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

parser.declareField((p, builder, context) -> builder.precision(precisionParser.parse(p)), FIELD_PRECISION,
org.elasticsearch.common.xcontent.ObjectParser.ValueType.INT);
parser.declareInt(GeoGridAggregationBuilder::size, FIELD_SIZE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

import java.io.IOException;
Expand All @@ -60,7 +59,7 @@ public class AutoDateHistogramAggregationBuilder
private static final ObjectParser<AutoDateHistogramAggregationBuilder, Void> PARSER;
static {
PARSER = new ObjectParser<>(AutoDateHistogramAggregationBuilder.NAME);
ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, true);
PARSER.declareInt(AutoDateHistogramAggregationBuilder::setNumBuckets, NUM_BUCKETS_FIELD);
PARSER.declareStringOrNull(AutoDateHistogramAggregationBuilder::setMinimumIntervalExpression, MINIMUM_INTERVAL_FIELD);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand Down Expand Up @@ -94,7 +93,7 @@ public class DateHistogramAggregationBuilder extends ValuesSourceAggregationBuil
private static final ObjectParser<DateHistogramAggregationBuilder, Void> PARSER;
static {
PARSER = new ObjectParser<>(DateHistogramAggregationBuilder.NAME);
ValuesSourceParserHelper.declareAnyFields(PARSER, true, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, true);

DateIntervalWrapper.declareIntervalFields(PARSER);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand Down Expand Up @@ -65,7 +64,7 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder<
private static final ObjectParser<HistogramAggregationBuilder, Void> PARSER;
static {
PARSER = new ObjectParser<>(HistogramAggregationBuilder.NAME);
ValuesSourceParserHelper.declareAnyFields(PARSER, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);

PARSER.declareDouble(HistogramAggregationBuilder::interval, Histogram.INTERVAL_FIELD);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

import java.io.IOException;
Expand All @@ -44,7 +43,7 @@ public class MissingAggregationBuilder extends ValuesSourceAggregationBuilder<Mi
public static final ObjectParser<MissingAggregationBuilder, String> PARSER =
ObjectParser.fromBuilder(NAME, MissingAggregationBuilder::new);
static {
ValuesSourceParserHelper.declareAnyFields(PARSER, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);
}

public MissingAggregationBuilder(String name) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
import org.elasticsearch.search.aggregations.AggregatorFactory;
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand All @@ -43,7 +43,7 @@ public class DateRangeAggregationBuilder extends AbstractRangeBuilder<DateRangeA
private static final ObjectParser<DateRangeAggregationBuilder, Void> PARSER;
static {
PARSER = new ObjectParser<>(DateRangeAggregationBuilder.NAME);
ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, true);
PARSER.declareBoolean(DateRangeAggregationBuilder::keyed, RangeAggregator.KEYED_FIELD);

PARSER.declareObjectArray((agg, ranges) -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

import java.io.IOException;
Expand All @@ -61,7 +60,7 @@ public class GeoDistanceAggregationBuilder extends ValuesSourceAggregationBuilde
private static final ObjectParser<GeoDistanceAggregationBuilder, Void> PARSER;
static {
PARSER = new ObjectParser<>(GeoDistanceAggregationBuilder.NAME);
ValuesSourceParserHelper.declareGeoFields(PARSER, true, false);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, false, false);

PARSER.declareBoolean(GeoDistanceAggregationBuilder::keyed, RangeAggregator.KEYED_FIELD);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

import java.io.IOException;
Expand All @@ -61,7 +60,7 @@ public final class IpRangeAggregationBuilder
private static final ObjectParser<IpRangeAggregationBuilder, Void> PARSER;
static {
PARSER = new ObjectParser<>(IpRangeAggregationBuilder.NAME);
ValuesSourceParserHelper.declareBytesFields(PARSER, false, false);
ValuesSourceAggregationBuilder.declareFields(PARSER, false, false, false);

PARSER.declareBoolean(IpRangeAggregationBuilder::keyed, RangeAggregator.KEYED_FIELD);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
import org.elasticsearch.search.aggregations.AggregatorFactory;
import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;

import java.io.IOException;
Expand All @@ -41,7 +41,7 @@ public class RangeAggregationBuilder extends AbstractRangeBuilder<RangeAggregati
private static final ObjectParser<RangeAggregationBuilder, Void> PARSER;
static {
PARSER = new ObjectParser<>(RangeAggregationBuilder.NAME);
ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);
PARSER.declareBoolean(RangeAggregationBuilder::keyed, RangeAggregator.KEYED_FIELD);

PARSER.declareObjectArray((agg, ranges) -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

import java.io.IOException;
Expand All @@ -46,7 +45,7 @@ public class DiversifiedAggregationBuilder extends ValuesSourceAggregationBuilde
public static final ObjectParser<DiversifiedAggregationBuilder, String> PARSER =
ObjectParser.fromBuilder(NAME, DiversifiedAggregationBuilder::new);
static {
ValuesSourceParserHelper.declareAnyFields(PARSER, true, false);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, false, false);
PARSER.declareInt(DiversifiedAggregationBuilder::shardSize, SamplerAggregator.SHARD_SIZE_FIELD);
PARSER.declareInt(DiversifiedAggregationBuilder::maxDocsPerValue, SamplerAggregator.MAX_DOCS_PER_VALUE_FIELD);
PARSER.declareString(DiversifiedAggregationBuilder::executionHint, SamplerAggregator.EXECUTION_HINT_FIELD);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand All @@ -67,7 +66,7 @@ public class SignificantTermsAggregationBuilder extends ValuesSourceAggregationB
SignificantTermsAggregationBuilder.NAME,
SignificanceHeuristic.class, SignificantTermsAggregationBuilder::significanceHeuristic, null);
static {
ValuesSourceParserHelper.declareAnyFields(PARSER, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);

PARSER.declareInt(SignificantTermsAggregationBuilder::shardSize, TermsAggregationBuilder.SHARD_SIZE_FIELD_NAME);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand All @@ -50,7 +49,7 @@ public class RareTermsAggregationBuilder extends ValuesSourceAggregationBuilder<
public static final ObjectParser<RareTermsAggregationBuilder, String> PARSER =
ObjectParser.fromBuilder(NAME, RareTermsAggregationBuilder::new);
static {
ValuesSourceParserHelper.declareAnyFields(PARSER, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);
PARSER.declareLong(RareTermsAggregationBuilder::maxDocCount, MAX_DOC_COUNT_FIELD_NAME);

PARSER.declareField((b, v) -> b.includeExclude(IncludeExclude.merge(v, b.includeExclude())),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand All @@ -67,7 +66,7 @@ public class TermsAggregationBuilder extends ValuesSourceAggregationBuilder<Term
public static final ObjectParser<TermsAggregationBuilder, String> PARSER =
ObjectParser.fromBuilder(NAME, TermsAggregationBuilder::new);
static {
ValuesSourceParserHelper.declareAnyFields(PARSER, true, true);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);

PARSER.declareBoolean(TermsAggregationBuilder::showTermDocCountError,
TermsAggregationBuilder.SHOW_TERM_DOC_COUNT_ERROR);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import org.elasticsearch.search.aggregations.AggregatorFactories;
import org.elasticsearch.search.aggregations.support.ValuesSource;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;

import java.io.IOException;
import java.util.Arrays;
Expand Down Expand Up @@ -105,7 +104,7 @@ public static <T extends AbstractPercentilesAggregationBuilder<T>> ConstructingO
return ctor.apply(name, values, percentilesConfig);
});

ValuesSourceParserHelper.declareAnyFields(parser, true, true);
ValuesSourceAggregationBuilder.declareFields(parser, true, true, false);
parser.declareDoubleArray(ConstructingObjectParser.optionalConstructorArg(), valuesField);
parser.declareBoolean(T::keyed, KEYED_FIELD);
parser.declareObject(ConstructingObjectParser.optionalConstructorArg(), PercentilesMethod.TDIGEST_PARSER,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSource;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand All @@ -43,7 +42,7 @@ public class AvgAggregationBuilder extends ValuesSourceAggregationBuilder.LeafOn

public static final ObjectParser<AvgAggregationBuilder, String> PARSER = ObjectParser.fromBuilder(NAME, AvgAggregationBuilder::new);
static {
ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);
}

public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSource;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

Expand All @@ -52,7 +51,7 @@ public final class CardinalityAggregationBuilder
public static final ObjectParser<CardinalityAggregationBuilder, String> PARSER =
ObjectParser.fromBuilder(NAME, CardinalityAggregationBuilder::new);
static {
ValuesSourceParserHelper.declareAnyFields(PARSER, true, false);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, false, false);
PARSER.declareLong(CardinalityAggregationBuilder::precisionThreshold, CardinalityAggregationBuilder.PRECISION_THRESHOLD_FIELD);
PARSER.declareLong((b, v) -> {/*ignore*/}, REHASH);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.elasticsearch.search.aggregations.support.ValuesSource;
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;

import java.io.IOException;
Expand All @@ -45,7 +44,7 @@ public class ExtendedStatsAggregationBuilder
public static final ObjectParser<ExtendedStatsAggregationBuilder, String> PARSER =
ObjectParser.fromBuilder(NAME, ExtendedStatsAggregationBuilder::new);
static {
ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false);
ValuesSourceAggregationBuilder.declareFields(PARSER, true, true, false);
PARSER.declareDouble(ExtendedStatsAggregationBuilder::sigma, ExtendedStatsAggregator.SIGMA_FIELD);
}

Expand Down
Loading