Backport aggs parsers for high level REST Client #24844

javanna · 2017-05-23T14:51:34Z

This PR is the backport of #24824 for 5.x. It required a separate PR because in the original feature branch we relied on the aggs test infra that has been recently expanded on master only as part of #22278. Some test classes were missing and are now introduced as part of this PR, yet only the pieces that are needed to test the aggs parsing code rather than all the work done on master. For instance InternalAggregationTestCase is added but it extends ESTestCase rather than AbstractWireSerializingTestCase. The reduce and serialization of aggs is still tested only on master while This PR introduces the testFromXContent test method only, which tests the new parsing code.

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

Add ParsedAggregation as base Aggregation impl for high level client (Add ParsedAggregation as base Aggregation impl for high level client #23965)
Adding ParsedCardinality (Adding ParsedCardinality #23973)
Add parsing for percentiles ranks (Add parsing for percentiles ranks #23974)
Add parsing to some single value aggregations (Add parsing to some single value aggregations #24085)
AbstractParsedPercentiles should use Percentile class (AbstractParsedPercentiles should use Percentile class #24160)
Add parsing for InternalSimpleValue and InternalDerivative (Add parsing for InternalSimpleValue and InternalDerivative #24162)
Add parsing for InternalBucketMetricValue (Add parsing for InternalBucketMetricValue #24182)
Add parsing methods for Percentiles aggregations (Add parsing methods for Percentiles aggregations #24183)
[Test] Always check the XContent equivalent when parsing aggregations ( [Test] Always check the XContent equivalent when parsing aggregations #24208)
Add parsing methods for InternalDateHistogram and InternalHistogram (Add parsing methods for InternalDateHistogram and InternalHistogram #24213)
Add parsing for InternalStats (Add parsing for InternalStats #24239)
Add parsing for InternalExtendedStats (Add parsing for InternalExtendedStats #24284)
Add parsing for InternalStatsBucket and InternalExtendedStatsBucket (Add parsing for InternalStatsBucket and InternalExtendedStatsBucket #24312)
Add parsing for InternalPercentilesBucket (Add parsing for InternalPercentilesBucket #24330)
Add parsing for InternalGeoBounds (Add parsing for InternalGeoBounds #24365)
Add parsing for InternalGeoCentroid (Add parsing for InternalGeoCentroid #24371)
Introduce SearchResponseSections base class (Introduce SearchResponseSections base class #24442)
Add parsing for String/Long/Double Terms aggregations (Add parsing for String/Long/Double Terms aggregations #24521)
[TEST] Add test for Aggregations#fromXContent ([TEST] Add test for Aggregations#fromXContent #24524)
Add parsing for single bucket aggregations (Add parsing for single bucket aggregations #24564)
Add parsing methods to Range aggregations (Add parsing methods to Range aggregations #24583)
Add parsing method to GeoHashGrid aggregation (Add parsing method to GeoHashGrid aggregation #24589)
Add parsing for InternalFilters aggregation (Add parsing for InternalFilters aggregation #24648)
Revert changing the InternalSampler type constant (Revert changing the InternalSampler type constant #24667)
Small improvement in InternalAggregationTestCase test setup (Small improvement in InternalAggregationTestCase test setup #24675)
Add parsing to Significant Terms aggregations (Add parsing to Significant Terms aggregations #24682)
Add parsing for InternalAdjacencyMatrix aggregation (Add parsing for InternalAdjacencyMatrix aggregation #24700)
Add parsing method for binary range aggregation (Add parsing method for binary range aggregation #24706)
Add parsing method for Top Hits aggregation (Add parsing method for Top Hits aggregation #24717)
Add fromXContent method to SearchResponse (Add fromXContent method to SearchResponse #24720)
Add parsing for InternalScriptedMetric aggregation (Add parsing for InternalScriptedMetric aggregation #24738)
Add parsing method for Matrix Stats (Add parsing method for Matrix Stats #24746)
Remove //norelease and cleans up some aggregations tests (Remove //norelease and cleans up some aggregations tests #24789)
[Test] Remove ParsedAggregationTests ([Test] Remove ParsedAggregationTests #24791)
SearchResponse#fromXContent to not require START_OBJECT as current token (SearchResponse#fromXContent to not require START_OBJECT as current token #24794)
Move getType to Aggregation interface (Move getType to Aggregation interface #24822)

In addition to these, the following PRs have been partially backported too:

Improve unit test coverage of aggs (Improve unit test coverage of aggs #22668)
Adds tests for cardinality and filter aggregations (Adds tests for cardinality and filter aggregations #23826)
[Test] Use appropriate DocValueFormats in Aggregations tests ([Test] Use appropriate DocValueFormats in Aggregations tests #24155)
Remove getCountAsString() from InternalStats and Stats interface (Remove getCountAsString() from InternalStats and Stats interface #24291)
Added unit tests for InternalMatrixStats (Added unit tests for InternalMatrixStats #24559)

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.

…kes a context

Note that it only extends ESTestCase, it will be used for fromXContent tests only in 5.x This is a partial backport of elastic#22668

This is a partial backport of elastic#23826

Adding parsing of InternalCardinality xContent output. Parsing method will return a new implementation of the Cardinality interface, ParsedCardinality.

…DocValueFormat dep

This commit adds the logic for parsing the percentiles ranks aggregations.

Now the Percentile interface has been merged with the InternalPercentile class in core (elastic#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

…4162)

…#24155) Some aggregations (like Min, Max etc) use a wrong DocValueFormat in tests (like IP or GeoHash). We should not test aggregations that expect a numeric value with a DocValueFormat like IP. Such wrong DocValueFormat can also prevent the aggregation to be rendered as ToXContent, and this will be an issue for the High Level Rest Client tests which expect to be able to parse back aggregations.

elastic#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.

…stic#24291) The `count` value in the stats aggregation represents a simple doc count that doesn't require a formatted version. We didn't render an "as_string" version for count in the rest response, so the method should also be removed in favour of just using String.valueOf(getCount()) if a string version of the count is needed. Closes elastic#24287

…lastic#24312)

This adds parsing to all implementations of SingleBucketAggregations. They are mostly similar, so they share the common base class `ParsedSingleBucketAggregation` and the shared base test `InternalSingleBucketAggregationTestCase`.

… make use of it from other modules. This is a partial backport of elastic#24559

…nges in master (elastic#24675)

This adds parsing to the InternalFilters aggregation.

Related to elastic#23331

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 elastic#23331

Related to elastic#23331

)

This method has been removed in core (see elastic#24714)

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 elastic#23965

…ken (elastic#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.

cbuescher

LGTM allthough due to the size I just glanced over the ParsedAggregation implementations. I left two really small nits on a method in InternalStats that was deprecated in 5.x, so it would be nice to keep it in 5 and let it be removed only in 6.

cbuescher · 2017-05-23T16:12:11Z

core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/InternalStats.java

@@ -112,11 +112,6 @@ public double getSum() {
    }

    @Override
-    public String getCountAsString() {


Nit: This was only deprecated in 5, so maybe we can leave this in.

cbuescher · 2017-05-23T16:12:31Z

core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/Stats.java

-     * @deprecated use String.valueOf(getCount()) instead if the count is needed as a string
-     */
-    @Deprecated
-    String getCountAsString();


Nit: This was only deprecated in 5, so maybe we can leave this in.

We would need to add this deprecated method to ParsedStats too, and I am not sure what it would return. I don't think it is worth the trouble. That is why I removed it, it was not necessary to deprecate it in the first place.

Ok, I just wanted to ask if it was a concious decision or not.

++ good call

tlrx

LGTM

javanna and others added 30 commits May 22, 2017 13:54

Add ParsedAggregation as base Aggregation impl for high level client

519ddb6

ParsedAggregation is the base Aggregation implementation for the high level client, which parses aggs responses into java objects.

add class to suppressions list for line length check

861f413

[TEST] replace FareRestRequest usage with ToXContent.MapParams

39c92e0

Remove TODO on un-deprecating NamedContentRegistry.Entry ctor that ta…

3e472ae

…kes a context

remove some more TODOs from ParsedAggregation

853427f

[TEST] backport InternalAggregationTestCase

ae18c2e

Note that it only extends ESTestCase, it will be used for fromXContent tests only in 5.x This is a partial backport of elastic#22668

[TEST] backport InternalCardinalityTests

eb16ef6

This is a partial backport of elastic#23826

Adding ParsedCardinality (elastic#23973)

a13b461

Adding parsing of InternalCardinality xContent output. Parsing method will return a new implementation of the Cardinality interface, ParsedCardinality.

fix typo in ParsedCardinality comment and add //norelease comment on …

6125b44

…DocValueFormat dep

Add parsing for percentiles ranks (elastic#23974)

b450c9d

This commit adds the logic for parsing the percentiles ranks aggregations.

Adding parsing for InternalMax and InternalMin

8d6384f

Adding parsing for InternalSum

b4bde5b

Adding parsing for InternalAvg

58f2a84

Adding parsing for InternalValueCount

24e4796

Minor changes in ParsedCardinality

d8943b9

Fix checkstyle violation

3abe97a

Add parsing for InternalSimpleValue and InternalDerivative (elastic#2…

a36967a

…4162)

Add parsing methods for Percentiles aggregations (elastic#24183)

e0956b6

Remove @repeat(iterations = 1000) in tests

d061588

Add parsing for InternalBucketMetricValue (elastic#24182)

bf21b1d

[Test] Expose AbstractPercentilesTestCase.randomPercents()

3e89987

Add parsing for InternalStats (elastic#24239)

73abbe3

Add parsing for InternalExtendedStats (elastic#24284)

bf1c4c7

Correcting 140 character line length

1e33567

Add parsing for InternalStatsBucket and InternalExtendedStatsBucket (e…

8b7fdb6

…lastic#24312)

cbuescher and others added 22 commits May 23, 2017 11:58

Move InternalAggregationTestCase to test-framework module in order to…

6d653f4

… make use of it from other modules. This is a partial backport of elastic#24559

Add parsing method to GeoHashGrid aggregation (elastic#24589)

ec57ecc

Add parsing methods to Range aggregations (elastic#24583)

3b17729

Fix msearch rest test using typed_keys

4d44a3a

Revert changing the InternalSampler type constant (elastic#24667)

c77735e

Small improvement in InternalAggregationTestCase test setup after cha…

e65f956

…nges in master (elastic#24675)

Add parsing for InternalFilters aggregation (elastic#24648)

1ced949

This adds parsing to the InternalFilters aggregation.

Add parsing for InternalAdjacencyMatrix aggregation (elastic#24700)

97aff40

Add parsing to Significant Terms aggregations (elastic#24682)

e4c3107

Related to elastic#23331

Add parsing for InternalScriptedMetric aggregation (elastic#24738)

eb5e83b

Add parsing method for binary range aggregation (elastic#24706)

5413bf2

Related to elastic#23331

Add parsing method for Top Hits aggregation (elastic#24717)

d59df10

Related to elastic#23331

Fix checkstyle violation in ParsedScriptedMetric

7b1f1ae

Add parsing method for Matrix Stats (elastic#24746)

24a9ee9

Related to elastic#23331

Remove //norelease and cleans up somet aggregations tests (elastic#24789

cc8273f

)

Remove compareTerm() method in parsed Significant Terms aggregations

4cf10df

This method has been removed in core (see elastic#24714)

Fix InternalExtendedStatsTests setUp

d50bbac

[Test] Remove ParsedAggregationTests (elastic#24791)

e6e125a

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 elastic#23965

Move getType to Aggregation interface (elastic#24822)

2b90795

Given that both InternalAggregation and ParsedAggregation have this method, it makes sense to move it to the interface they both implement.

javanna added :Java High Level REST Client v5.5.0 labels May 23, 2017

javanna requested review from tlrx and cbuescher May 23, 2017 14:51

cbuescher approved these changes May 23, 2017

View reviewed changes

tlrx approved these changes May 23, 2017

View reviewed changes

javanna merged commit 59179ce into elastic:5.x May 24, 2017

clintongormley added the >enhancement label Jun 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport aggs parsers for high level REST Client #24844

Backport aggs parsers for high level REST Client #24844

javanna commented May 23, 2017

cbuescher left a comment

cbuescher May 23, 2017

cbuescher May 23, 2017

javanna May 23, 2017

cbuescher May 24, 2017

javanna May 24, 2017

tlrx left a comment

Backport aggs parsers for high level REST Client #24844

Backport aggs parsers for high level REST Client #24844

Conversation

javanna commented May 23, 2017

cbuescher left a comment

Choose a reason for hiding this comment

cbuescher May 23, 2017

Choose a reason for hiding this comment

cbuescher May 23, 2017

Choose a reason for hiding this comment

javanna May 23, 2017

Choose a reason for hiding this comment

cbuescher May 24, 2017

Choose a reason for hiding this comment

javanna May 24, 2017

Choose a reason for hiding this comment

tlrx left a comment

Choose a reason for hiding this comment