Skip to content

Conversation

@javanna
Copy link
Member

@javanna javanna commented May 22, 2017

This PR contains the work that has been done on adding parsers for aggregations responses in the feature/client_aggs_parsing branch. All of the aggregations are now supported hence the branch can be merged to master, later on #24796 can be merged to master too so that the high level REST client finally supports _search.

Rather than reusing the classes that already existed for aggregations (which we have done for other apis, or other sections of search), we went for implementing the aggregations interfaces but creating our own classes that we parse responses into. This is because the existing classes are internal classes that hold quite some internal state which would not be available when parsing response back from the REST layer.

The following are all the PRs that were merged to the branch:

javanna and others added 30 commits April 7, 2017 11:11
ParsedAggregation is the base Aggregation implementation for the high level client, which parses aggs responses into java objects.
Empty meta gets printed out, which means that if the request contains an empty meta object, that is returned with the response as well. On the other hand null, meaning when the object is not in the request, is not printed out. ParsedAggregation used to not print out empty metadata, and didn't allow the null value. Aligned behaviour to the existing behaviour from InternalAggregation.
Adding parsing of InternalCardinality xContent output. Parsing method will return a new
implementation of the Cardinality interface, ParsedCardinality.
This commit adds the logic for parsing the percentiles ranks aggregations.
Now the Percentile interface has been merged with the InternalPercentile
class in core (#24154) the AbstractParsedPercentiles should use it.

This commit also changes InternalPercentilesRanksTestCase so that it now
tests the iterator obtained from parsed percentiles ranks aggregations.

Adding this new test raised an issue in the iterators where key and
value are "swapped" in internal implementations when building the
iterators (see InternalTDigestPercentileRanks.Iter constructor that
accepts the `keys` as the first parameter named `values`, each key
being mapped to the `value` field of Percentile class). This is because
 percentiles ranks aggs inverts percentiles/values compared to the
 percentiles aggs.

* Add assume in InternalAggregationTestCase

* Update after Luca review
…_parsing

# Conflicts:
#	core/src/main/java/org/elasticsearch/search/DocValueFormat.java
#	core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java
#	core/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalMaxTests.java
#	core/src/test/java/org/elasticsearch/search/aggregations/metrics/avg/InternalAvgTests.java
#	core/src/test/java/org/elasticsearch/search/aggregations/metrics/min/InternalMinTests.java
#24208)

In InternalAggregationTestCase, we can check that the internal aggregation and the parsed aggregation always produce the same XContent even if the original internal aggregation has been shuffled or not.
…_parsing

# Conflicts:
#	core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/InternalPercentilesTestCase.java
#	core/src/test/java/org/elasticsearch/search/aggregations/metrics/percentiles/hdr/InternalHDRPercentilesTests.java
tlrx and others added 17 commits May 16, 2017 14:54
SearchResponse#fromXContent allows to parse a search response, including search hits, aggregations, suggestions and profile results. Only the aggs that we can parse today are supported (which means all of them but a couple that are left to support). SearchResponseTests reuses the existing test infra to randomize aggregations, suggestions and profile response.

Relates to #23331
Now the Java High Level Rest Client has tests to parse all aggregations,
 this test is not needed anymore. We have better tests like
 AggregationsTests and sub classes of InternalAggregationTestCase.

 Related to #23965
…ken (#24794)

The method should rather advance one token and only then require a START_OBJECT as the current token. This allows to parse given a parser that's at the beginning of the response, where the initial/current token is null.
Given that both InternalAggregation and ParsedAggregation have this method, it makes sense to move it to the interface they both implement.
@s1monw
Copy link
Contributor

s1monw commented May 22, 2017

formal LGTM - lets get this in!

@javanna javanna changed the title Add aggs parser for high level REST Client Add aggs parsers for high level REST Client May 22, 2017
@javanna javanna merged commit c584c2f into master May 22, 2017
@javanna javanna deleted the feature/client_aggs_parsing branch May 24, 2017 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants