Skip to content

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented Apr 18, 2017

This commit merges the Percentile interface with the InternalPercentile
class, so we don't need to maintain both.

This commit merges the Percentile interface with the InternalPercentile
class, as we don't need to maintain both.
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@tlrx tlrx merged commit f217eb8 into elastic:master Apr 18, 2017
@tlrx tlrx deleted the merge-percentile-class-interface branch April 18, 2017 12:51
tlrx added a commit that referenced this pull request Apr 18, 2017
This commit merges the Percentile interface with the InternalPercentile
class, as we don't need to maintain both.
@tlrx
Copy link
Member Author

tlrx commented Apr 18, 2017

Thanks @colings86 @javanna !

This has been backported to 5.x (3761b29) because the High Level Rest Client might need it.

@tlrx tlrx added the v5.5.0 label Apr 18, 2017
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Apr 18, 2017
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.
tlrx added a commit that referenced this pull request Apr 18, 2017
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
javanna pushed a commit to javanna/elasticsearch that referenced this pull request May 23, 2017
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
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.

3 participants