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

Aggregations cleanup #5614

Closed
wants to merge 1 commit into from

Conversation

@uboness
Copy link
Contributor

commented Mar 31, 2014

  • removed an abstraction layer that handles the values source (consolidated values source with field data source)
  • better handling of value parser/formatter in range & histogram aggs
  • the buckets key will now be shown by default in range agg
@jpountz
jpountz reviewed Mar 31, 2014
View changes
src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParser.java Outdated
@@ -96,7 +94,7 @@ public AggregatorFactory parse(String aggregationName, XContentParser parser, Se
shardSize = requiredSize;
}

ValuesSourceConfig<GeoPointValuesSource> config = new ValuesSourceConfig<>(GeoPointValuesSource.class);
ValuesSourceConfig<ValuesSource.GeoPoint> config = new ValuesSourceConfig<ValuesSource.GeoPoint>(ValuesSource.GeoPoint.class);

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2014

Contributor

Please don't undo the migration to the diamond operator. :-)

@jpountz
jpountz reviewed Mar 31, 2014
View changes
src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeParser.java Outdated
@@ -52,7 +52,7 @@ public String type() {
@Override
public AggregatorFactory parse(String aggregationName, XContentParser parser, SearchContext context) throws IOException {

ValuesSourceConfig<NumericValuesSource> config = new ValuesSourceConfig<>(NumericValuesSource.class);
ValuesSourceConfig<ValuesSource.Numeric> config = new ValuesSourceConfig<ValuesSource.Numeric>(ValuesSource.Numeric.class);

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2014

Contributor

diamond

@jpountz
jpountz reviewed Mar 31, 2014
View changes
...n/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceParser.java Outdated
@@ -178,7 +177,7 @@ public AggregatorFactory parse(String aggregationName, XContentParser parser, Se
throw new SearchParseException(context, "Missing [origin] in geo_distance aggregator [" + aggregationName + "]");
}

ValuesSourceConfig<GeoPointValuesSource> config = new ValuesSourceConfig<>(GeoPointValuesSource.class);
ValuesSourceConfig<ValuesSource.GeoPoint> config = new ValuesSourceConfig<ValuesSource.GeoPoint>(ValuesSource.GeoPoint.class);

This comment has been minimized.

Copy link
@jpountz

jpountz Mar 31, 2014

Contributor

diamond

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2014

Is the change in the range agg backward compatible?

I spotted a lines where you reverted the migration to the diamond operator but otherwise I really really like this change!

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2014

@jpountz the change in the range agg should be bwc... the main thing there is the addition of the key to the bucket by default (so it's more of an extension of the api rather than a break)

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2014

and sure... I'll give you back your diamonds

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2014

Perfect!

- removed an abstraction layer that handles the values source (consolidated values source with field data source)
- better handling of value parser/formatter in range & histogram aggs
- the buckets key will now be shown by default in range agg
@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2014

closed by d6636fc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.