Refactor Percentiles/Ranks aggregation builders and factories #51887

polyfractal · 2020-02-04T20:35:23Z

This PR consolidates HDR/TDigest factories into a single factory, and consolidates most of the HDR/TDigest builder into an abstract builder. The only difference in percentiles/ranks builder is the parse field for the percents or values field. It does this by introduce a new PercentileMethod.Config object which carries both algo method and algo-specific settings. This has several advantages, but a few disadvantages too.

I'm about 70/30 on this PR. I like the refactor, but it turned a bit more verbose than my liking. Open to suggestions, or just shelving this PR and ignoring the slightly trapping percentiles behavior today as a "wont-fix" due to complexity of changing the API.

Pros

Builders and factories share most code now, much less c/p. Some of this is already merged in the VS refactor too
Single object to hold method and algo-specific settings (again, already in VS refactor)
Consistent behavior between ranks and percentiles. Before Percentiles would reject a request with hdr and tdigest, while ranks would accept it and just use the last-parsed object
Disallows java users from configuring a setting that will be ignored (e.g. configuring compression on an HDRHisto, which previously would be silently ignored)
Removes the one-off private "InternalBuilder' on percentiles

Cons

Breaking-java: we now throw an exception if the user configures cross-algo settings. This is, however, a client-side exception so it should be caught with simple unit tests.
To maintain BWC, there are some extra gymnastics (method(), compression(), numSigFig() are retained but deprecated, default handling from old semantics is retained but a little tricky, etc)
PercentileMethod.Config is a bit of a strange arrangement, but we have to keep the Method enum around for BWC so it seemed reasonable to put the new config object there too. It's also a bit "bloated" for essentially a method + single setting.

- Consolidates HDR/TDigest factories into a single factory - Consolidates most HDR/TDigest builder into an abstract builder - Deprecates method(), compression(), numSigFig() in favor of a new unified PercentileMethod.Config object - Disallows setting algo options that don't apply to current algo The unified config method carries both the method and algo-specific setting. This provides a mechanism to reject settings that apply to the wrong algorithm. For BWC the old methods are retained but marked as deprecated, and can be removed in future versions.

elasticmachine · 2020-02-04T20:35:26Z

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

not-napoleon

As mentioned here and discusses in Slack, I'd really like to see the switching on enum logic refactored into enum methods, or at least document why we don't want to do that.

Someone besides me should look at the parser changes. I read them, but I don't know the parser well enough to feel confidant saying that's the right way to do it.

...ava/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java

not-napoleon · 2020-02-05T15:48:45Z

...ava/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java

+    /**
+     * Return the current algo configuration, or a default (Tdigest) otherwise
+     *
+     * This is needed because builders don't have a "build" or "finalize" method, but


This is needed because builders don't have a "build"...method

😢 😢 😢

...ava/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java

...ain/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregatorFactory.java

not-napoleon · 2020-02-05T17:23:34Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesMethod.java

+
+        public static Config fromStream(StreamInput in) throws IOException {
+            PercentilesMethod method = PercentilesMethod.readFromStream(in);
+            if (method.equals(PercentilesMethod.TDIGEST)) {


I really don't like this if(method.equals(TDIGEST) {...} else if (method.equals(HDR) {...} else { throw } pattern. We use it in a lot of places, and if we ever add another percentile method, we'll have to find and touch each of them. Plus, the throw is always unreachable now, since we'd have already thrown trying to create a bogus enum instance.

In this case, it's pretty trivial to put a method on the enum that takes a StreamInput and returns a Config, and I think we should do that. We should at least think about applying this logic to the other places we have this type of construct.

Pushed a change that takes care of the factory/builder instances of these as discussed on slack. I'm not sure this particular one is easily solved though. Note that this is a static method on the abstract Config object, one level "inside" the PercentilesMethod enum

The higher-level PercentilesMethod has a readFromStream() which is in use today to serialize the enum (as well as both compression and numSigFig in the builder). The builder already has to incorporate version checks for wire BWC, so I was hesitant to lump the Config serialization into the existing PercentilesMethod because it would also need version checks.

(Also, conceptually we're in a slightly odd place where the "new way" is to serialize the Config object which holds a PercentileMethod enum, so asking the enum to deserialize a Config seems incorrect. But that's more of an artifact of the current hierarchy which should be cleaner once things are removed post-deprecation)

Otherwise, I'm happy to move this up a level... but I think we'll still need these if/else at this point in the code. Since this isn't a NamedWriteable style situation, we have to inspect the serialized method to determine what kind of object needs to be built next. I don't see any way around that?

polyfractal · 2020-02-05T19:33:12Z

Thanks @not-napoleon. Looks like i have some json parsing issues anyhow, based on the test failures.

nik9000 · 2020-02-06T17:11:26Z

Someone besides me should look at the parser changes. I read them, but I don't know the parser well enough to feel confidant saying that's the right way to do it.

I'll take a look soon. Sorry for the delay.

nik9000 · 2020-02-06T17:25:56Z

...ava/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java

+        builder = configOrDefault().toXContent(builder, params);
+        return builder;
+    }
+


I feel like there should be some parsing code around here somewhere.

nik9000 · 2020-02-06T17:30:34Z

...in/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java

+            double[] values = ((List<Double>) objects[0]).stream().mapToDouble(Double::doubleValue).toArray();
+            PercentilesMethod.Config percentilesConfig;
+
+            if (objects[1] != null && objects[2] != null) {


If you have a method to et the config method you could use the flavor of ObjectParser that takes a categoryClass and parse these two an named xcontent. I don't believe that flavor is supported by ContructingObjectParser hough. This'll do the trick, but it is less nice.

nik9000 · 2020-02-06T17:35:15Z

...c/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesAggregationBuilder.java


+        PARSER.declareDoubleArray(ConstructingObjectParser.constructorArg(), PERCENTS_FIELD);
        PARSER.declareBoolean(PercentilesAggregationBuilder::keyed, KEYED_FIELD);
+        PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), PercentilesMethod.TDIGEST_PARSER,


I think we should have something in the superclass that helps with the parsing. I'm not sure what though.

nik9000

I do really like the change, even though BWC is tricky!

…sistency

polyfractal · 2020-02-07T21:05:11Z

Pushed some test fixes, addressed some review comments, and a bit of fiddling with how things work.

The config object is now a top-level class. Wasn't liking how it was an internal class, and this should make deprecating easier since the PercentileMethod can become an internal, private thing eventually

Adjusted how the parsers work, so now the bulk of it is handled by the super-class. Had to settle on a not-ideal ConstructingObjectParser situation (described in comments) due to the differences in Percentiles and Ranks, but I don't think it is too bad.

…_refactor

The exception message changed slightly, and rather than make two tests with version gates we can just look for a 4xx exception.

not-napoleon

LGTM, once CI is green.

not-napoleon · 2020-02-10T19:56:09Z

...ava/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java

+         * This is a compromise, in that it is a ConstructingOP which accepts all optional arguments, and then we sort
+         * out the behavior from there
+         */
+        ConstructingObjectParser<T, String> parser = new ConstructingObjectParser<>(aggName, false, (objects, name) -> {


nit: Can we just add something to the javadoc for what we expect the contents of objects to be? Especially, it looks like we're passing in two configs, at most one of which should be not null, but it's not clear if order matters for them.

not-napoleon · 2020-02-10T20:11:18Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/PercentilesConfig.java

+ * a single unified constructor for both algos, but internally switch execution
+ * depending on which algo is selected
+ */
+public abstract class PercentilesConfig implements ToXContent, Writeable {


I like how this looks as a top level. Makes more sense this way.

polyfractal · 2020-02-11T16:06:43Z

@elasticmachine run elasticsearch-ci/1

…_refactor

nik9000

I left a few small things but LGTM regardless.

...ava/org/elasticsearch/search/aggregations/metrics/AbstractPercentilesAggregationBuilder.java

...in/java/org/elasticsearch/search/aggregations/metrics/PercentileRanksAggregationBuilder.java

polyfractal · 2020-02-11T20:30:53Z

Thanks @not-napoleon @nik9000 for the reviews! <3

…c#51887) - Consolidates HDR/TDigest factories into a single factory - Consolidates most HDR/TDigest builder into an abstract builder - Deprecates method(), compression(), numSigFig() in favor of a new unified PercentileConfig object - Disallows setting algo options that don't apply to current algo The unified config method carries both the method and algo-specific setting. This provides a mechanism to reject settings that apply to the wrong algorithm. For BWC the old methods are retained but marked as deprecated, and can be removed in future versions. Co-authored-by: Mark Tozzi <[email protected]>

#54537) - Consolidates HDR/TDigest factories into a single factory - Consolidates most HDR/TDigest builder into an abstract builder - Deprecates method(), compression(), numSigFig() in favor of a new unified PercentileConfig object - Disallows setting algo options that don't apply to current algo The unified config method carries both the method and algo-specific setting. This provides a mechanism to reject settings that apply to the wrong algorithm. For BWC the old methods are retained but marked as deprecated, and can be removed in future versions. Co-authored-by: Mark Tozzi <[email protected]> Co-authored-by: Mark Tozzi <[email protected]>

$@polyfractal$ polyfractal added :Analytics/Aggregations Aggregations >breaking-java >refactoring v8.0.0 labels Feb 4, 2020

not-napoleon requested changes Feb 5, 2020

View reviewed changes

polyfractal and others added 2 commits February 5, 2020 15:50

$@polyfractal$

Address some review comments

315f546

Proposed config change

830b796

$@polyfractal$ polyfractal mentioned this pull request Feb 6, 2020

Add Boxplot Aggregation #51948

Merged

nik9000 reviewed Feb 6, 2020

View reviewed changes

polyfractal added 3 commits February 6, 2020 17:27

$@polyfractal$

Review cleanup, fixes to deal with optional defaults in tests

7c343ea

$@polyfractal$

Clarify how the parser works, rename percentileConfig methods for con…

e8a7b3d

…sistency

$@polyfractal$

Move Config to own high-level class

2e76f66

polyfractal added 4 commits February 10, 2020 11:26

$@polyfractal$

checkstyle

6e804bc

$@polyfractal$

Merge remote-tracking branch 'origin/master' into percentiles_builder…

7a92453

…_refactor

$@polyfractal$

Merge conflict

4dc1118

$@polyfractal$

Loosen exception catching on yaml tests

098e098

The exception message changed slightly, and rather than make two tests with version gates we can just look for a 4xx exception.

not-napoleon approved these changes Feb 10, 2020

View reviewed changes

$@polyfractal$

Clarify and rename args object in ConstructingObjectParser

ffa6a2c

polyfractal added 2 commits February 11, 2020 11:55

$@polyfractal$

Merge remote-tracking branch 'origin/master' into percentiles_builder…

dcd55ce

…_refactor

$@polyfractal$

Adjust to account for new validations

3672467

nik9000 approved these changes Feb 11, 2020

View reviewed changes

$@polyfractal$

Address review comments

31aaa92

$@polyfractal$ polyfractal merged commit ba9c4fb into elastic:master Feb 11, 2020

not-napoleon mentioned this pull request Mar 6, 2020

Refactor ValuesSource and related classes #42949

Closed

85 tasks

$@polyfractal$ polyfractal mentioned this pull request Mar 31, 2020

[7.x] Refactor Percentiles/Ranks aggregation builders and factories (#51887) #54537

Merged

centic9 mentioned this pull request Sep 10, 2020

Regression: NullPointerException when executing a percentile aggregation from a 7.8 Client to a 6.x Server #62199

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Refactor Percentiles/Ranks aggregation builders and factories #51887

Refactor Percentiles/Ranks aggregation builders and factories #51887

Uh oh!

Conversation

polyfractal commented Feb 4, 2020

Pros

Cons

Uh oh!

elasticmachine commented Feb 4, 2020

Uh oh!

not-napoleon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal commented Feb 5, 2020

Uh oh!

nik9000 commented Feb 6, 2020

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!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

polyfractal commented Feb 7, 2020

Uh oh!

not-napoleon 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!

polyfractal commented Feb 11, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polyfractal commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

$@polyfractal$ polyfractal commented Feb 4, 2020