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

HLRC support for string_stats #52163

Merged
merged 9 commits into from
Feb 12, 2020
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Feb 10, 2020

This adds a builder and parsed results for the string_stats
aggregation directly to the high level rest client. Without this the
HLRC can't access the string_stats API without the elastic licensed
analytics module.

While I'm in there this adds a few of our usual unit tests and
modernizes the parsing.

This adds a builder and parsed results for the `string_stats`
aggregation directly to the high level rest client. Without this the
HLRC can't access the `string_stats` API without the elastic licensed
`analytics` module.

While I'm in there this adds a few of our usual unit tests and
modernizes the parsing.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@nik9000
Copy link
Member Author

nik9000 commented Feb 10, 2020

This is a work in progress because I don't have the horsepower to run all the tests at the moment. So I'm making Jenkins do it. I think this is how I want to expose analytics aggs to the HLRC, though the hack with the builder is unfortunate.

@nik9000 nik9000 changed the title WIP: HLRC support for string_stats HLRC support for string_stats Feb 11, 2020
@nik9000
Copy link
Member Author

nik9000 commented Feb 11, 2020

I've removed WIP because it seems to mostly work. I'm going to figure out what the test failure is this morning.

@nik9000
Copy link
Member Author

nik9000 commented Feb 11, 2020

@imotov you mentioned a while back that you were interested in talking about HLRC support for the analytics aggs. This is my take on explicit support for the string_stats agg. What do you think?

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

Looks nice! Even though that doesn't solve an issue with AggregationBuilders.stringStats() method, but I think it is a good step forward.

Map<String, Long> charOccurrences = instance.getCharOccurrences();
boolean showDistribution = instance.getShowDistribution();
switch (between(0, 6)) {
case 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

That indentation looks a bit strange to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the true indentation for case statements!

Copy link
Contributor

Choose a reason for hiding this comment

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

It is indeed, we just don't use it anywhere else :) that's why it looks strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the Eclipse standard but not the intellij standard. I prefer this was but I'm not picky.

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasEntry;

public class StringStatsIT extends ESRestHighLevelClientTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wonder if we should rename this to AnalyticsAggsIT or something like this, so we can add other agg tests from analytics plugin here as we add convert them to work with the rest client.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

@nik9000
Copy link
Member Author

nik9000 commented Feb 12, 2020

Looks nice! Even though that doesn't solve an issue with AggregationBuilders.stringStats() method, but I think it is a good step forward.

Do you think we should move AggregationBuilders into the HLRC in 8.0? And just make the aggs by hand outside of HLRC? Personally I'm not a super huge fan of the builder methods anyway, but I want to do right by them.

@nik9000
Copy link
Member Author

nik9000 commented Feb 12, 2020

Do you think we should move AggregationBuilders into the HLRC in 8.0? And just make the aggs by hand outside of HLRC? Personally I'm not a super huge fan of the builder methods anyway, but I want to do right by them.

Like, as part of a separate change. Not this one.

@imotov
Copy link
Contributor

imotov commented Feb 12, 2020

That's the challenge *AggregationBuilders are playing dual role at the moment. On one side they are the client facing aggregation builders, on another side they are transport classes that know how to serialize/deserialize aggs over transport interface as well as parse them. There are ways to separate these functions, but they are not trivial, unfortunately.

@nik9000
Copy link
Member Author

nik9000 commented Feb 12, 2020

There are ways to separate these functions, but they are not trivial, unfortunately.

+2

@nik9000 nik9000 merged commit 75d83db into elastic:master Feb 12, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Feb 12, 2020
This adds a builder and parsed results for the `string_stats`
aggregation directly to the high level rest client. Without this the
HLRC can't access the `string_stats` API without the elastic licensed
`analytics` module.

While I'm in there this adds a few of our usual unit tests and
modernizes the parsing.
nik9000 added a commit that referenced this pull request Feb 13, 2020
This adds a builder and parsed results for the `string_stats`
aggregation directly to the high level rest client. Without this the
HLRC can't access the `string_stats` API without the elastic licensed
`analytics` module.

While I'm in there this adds a few of our usual unit tests and
modernizes the parsing.
@nik9000 nik9000 deleted the hlrc_string_stats branch February 13, 2020 00:25
@nik9000
Copy link
Member Author

nik9000 commented Feb 13, 2020

Thanks @imotov !

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.

None yet

4 participants