Add parsing to some single value aggregations #24085

cbuescher · 2017-04-13T10:34:05Z

Similar to #23973 this adds parsing from xContent for the Min, Max, Avg, Sum and ValueCount aggregation.
I tried to pull out common functionality to ParsedNumericMetricsAggregation.SingleValue. Since InternalCardinality
and InternalValueCount store a long value instead of a double, parsing and getters/setters for the value are slightly
different so I didn't include their parsed counterparts under ParsedNumericMetricsAggregation.SingleValue for now.
Also included one case where I tried pulling out common xContent rendering into the shared interface (in Sum) for discussion.
I'm unsure if I like that one but it would help reduce some code duplication.

This PR is WIP against a feature branch.

tlrx

I had a first look, I'm happy to see that many things can be mutualized. I don't really like how the parsed Cardinality/ValueCount extends the ParsedNumericMetricsAggregation.SingleValue and don't use the double there and provide their own long value. I'm wondering if we could let SingleValue take care of the stringified value and have a Long and Double version of SingleValue where all parsing logic and XContent rendering would be placed.

I have to admit that I'm not sure if that's possible at all, so I'll take a look at this too

tlrx · 2017-04-14T08:34:32Z

core/src/main/java/org/elasticsearch/search/aggregations/metrics/avg/ParsedAvg.java

I tend to use the current class name, because this name is only used in log/error traces so that one can easily spot the class to look at. It seems that there's no convention in the codebase for that, so let's fix one at least for aggregations.

I'm +1 on ParsedAvg.class.getSimpleName() (but I'm OK to use AvgAggregationBuilder.NAME if both @javanna and you prefer)

I don't have a strong opinion here, I don't think it makes a whole lot of difference

Will according to suggestion by @tlrx than, since I also don't mind much.

tlrx · 2017-04-14T08:37:59Z

...main/java/org/elasticsearch/search/aggregations/metrics/ParsedNumericMetricsAggregation.java

nit: Maybe declareSingeValueFields ?

tlrx · 2017-04-14T08:39:21Z

...main/java/org/elasticsearch/search/aggregations/metrics/ParsedNumericMetricsAggregation.java

Maybe declareAggregationFields()? I think that "common" doesn't have much sense where there's 2-3 levels of inheritance..

tlrx · 2017-04-14T08:41:09Z

...main/java/org/elasticsearch/search/aggregations/metrics/ParsedNumericMetricsAggregation.java

I think it can be shortened to
(parser, context) -> parseValue(parser, defaultNullValue)

tlrx · 2017-04-14T09:04:58Z

core/src/main/java/org/elasticsearch/search/aggregations/metrics/max/ParsedMax.java

Can we move all statics down? (after doXContentBody())

javanna

left a few comments but LGTM

javanna · 2017-04-18T08:21:32Z

...main/java/org/elasticsearch/search/aggregations/metrics/ParsedNumericMetricsAggregation.java

do we need this class? and why does SingleValue have to be an inner class?

I don't know, the idea was to pull out value()/getValueAsString() and the internal fields for the Min/Max/Sum/Avg aggregation, maybe some others of the single value aggregations that just store a double. I tried to mimic the inheritance structure of InternalNumericMetricsAggregation here, which contains inner classes for SingleValue and MultiValue. Maybe we don't need it, but I'd like to keep it for now and revisit this decision once we are done with all the single value aggregations.

I think this hierarchy makes little sense and we should not copy it to our objects unless there are good reasons to do so. Is the only reason consistency?

Is sharing value/valueAsString() a good reason to keep ParsedNumericMetricsAggregation? Or rename it to ParsedSingleValueMetricsAggregation and remove the inner SingleValue class? My suspicion at this point is that we might also reuse some stuff for the MultiValue case but I cannot tell until we start working on those.

I do not see what ParsedNumericMetricsAggregation holds. Seems like it is used only as an ancestor for its inner class? In that case let's have ParsedSingleValueNumericMetricsAggregation as a top level class?

javanna · 2017-04-18T08:22:11Z

...main/java/org/elasticsearch/search/aggregations/metrics/ParsedNumericMetricsAggregation.java

should we try and do without depending on DocValueFormat here? We can just do what this method does?

I replaced this with Double.toString(value).

javanna · 2017-04-18T08:24:36Z

core/src/main/java/org/elasticsearch/search/aggregations/metrics/avg/ParsedAvg.java

I don't have a strong opinion here, I don't think it makes a whole lot of difference

javanna · 2017-04-18T08:32:42Z

core/src/main/java/org/elasticsearch/search/aggregations/metrics/avg/ParsedAvg.java

where is this logic in InternalAvg?

In InternalAvg the value field is only rendered if the avg normalizer (count) is not 0. (count != 0 ? getValue() : null). I choose to parse back null as Double.POSITIVE_INFINITY because that is what you get back when you divide a positive double by 0L. So I check for that value here to get the same xContent output like InternalAvg.

sounds good! leave a comment?

I added a comment.

javanna · 2017-04-18T08:34:02Z

core/src/main/java/org/elasticsearch/search/aggregations/metrics/sum/Sum.java

what's left todo here? the static method is already in the interface?

This was an example of how we could share the xContent rendering logic between InternalXY and ParsedXY aggregations. I don't know if it would work in all places. I only did it for SUM, I could try to do it for Min/Max/Avg etc but that means slight changes in the existing internal aggregations. Not sure which way to go here atm, maybe you have some opinion?

I think we should do it only when it's very simple to do. It is a bit funky to have this static method in the interface but that is the best place for it. @tlrx what do you think?

I tried doing this for the aggregations in this PR in 1048678, but I'm not sure I like it. It avoids some code duplication but makes reading and reasoning about the code much harder in my opinion. Maybe we should only do it when it really saves a lot of duplication (like really long xContent methods), wdyt?

yea I tend to agree, I am +0 on this :) I defer to Tanguy

javanna · 2017-04-18T08:35:39Z

...src/main/java/org/elasticsearch/search/aggregations/metrics/valuecount/ParsedValueCount.java

here too, shall we try and not depend on the formatter?

I changed this to Long.toString(valueCount).

javanna · 2017-04-18T08:35:50Z

...src/main/java/org/elasticsearch/search/aggregations/metrics/valuecount/ParsedValueCount.java

s/formated/formatted

javanna · 2017-04-18T08:36:21Z

core/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalMaxTests.java

javanna · 2017-04-18T08:36:53Z

core/src/test/java/org/elasticsearch/search/aggregations/metrics/InternalMaxTests.java

complete POSITIVE with POSITIVE_INFINITY ?

cbuescher · 2017-04-18T11:03:21Z

@tlrx @javanna thanks for the reviews, I added changes according to most of your comments, left two discussions open for now before I squash/merge this later

tlrx · 2017-04-18T15:16:59Z

Maybe we should only do it when it really saves a lot of duplication (like really long xContent methods), wdyt?

Yes, I agree. I think we should only do it when it saves a lot of duplication with custom logic. In this case it saves 1 line but added many more, and it also "force" people to look at what the Sum.renderXContent() is doing. I think we should not add those.

cbuescher · 2017-04-18T16:12:36Z

@tlrx thanks, I reverted the commit where I pulled the xContent rendering into the interfaces, rebased and reordered the PR so changes are more self contained. I think I will merge this one manually to the feature branch without squashing so that the history better reflects the individual changes to each aggregation.

cbuescher added :Java High Level REST Client WIP labels Apr 13, 2017

cbuescher requested review from javanna and tlrx April 13, 2017 10:34

cbuescher mentioned this pull request Apr 13, 2017

Add parsing for percentiles ranks #23974

Merged

tlrx reviewed Apr 14, 2017

View reviewed changes

javanna approved these changes Apr 18, 2017

View reviewed changes

cbuescher removed the WIP label Apr 18, 2017

cbuescher force-pushed the addParsing-singleValueAggs branch from 0762331 to 9e6d11d Compare April 18, 2017 11:02

cbuescher added 3 commits April 18, 2017 17:38

Adding parsing for InternalMax and InternalMin

75fdc94

Adding parsing for InternalSum

695b285

Adding parsing for InternalAvg

5f96972

cbuescher force-pushed the addParsing-singleValueAggs branch from 1048678 to a6112a5 Compare April 18, 2017 16:01

cbuescher added 2 commits April 18, 2017 18:09

Adding parsing for InternalValueCount

bc646cf

Minor changes in ParsedCardinality

210e101

cbuescher force-pushed the addParsing-singleValueAggs branch from a6112a5 to 210e101 Compare April 18, 2017 16:10

cbuescher merged commit 210e101 into elastic:feature/client_aggs_parsing Apr 18, 2017

This was referenced Apr 18, 2017

Add parsing for InternalSimpleValue and InternalDerivative #24162

Merged

Add parsing for InternalBucketMetricValue #24182

Merged

Add parsing for InternalStats #24239

Merged

Add parsing for InternalExtendedStats #24284

Merged

cbuescher mentioned this pull request Apr 27, 2017

Add parsing for InternalGeoBounds #24365

Merged

This was referenced May 22, 2017

Add aggs parsers for high level REST Client #24824

Merged

Backport aggs parsers for high level REST Client #24844

Merged

Add parsing to some single value aggregations #24085

Add parsing to some single value aggregations #24085

Uh oh!

Conversation

cbuescher commented Apr 13, 2017

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher Apr 18, 2017 •

edited

Loading