Teach ObjectParser a happy pattern #50691
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We *very* commonly have object with ctors like: ``` public Foo(String name) ``` And then declare a bunch of setters on the object. Every aggregation works like this, for example. This change teaches `ObjectParser` how to build these aggregations all on its own, without any help. This'll make it much cleaner to parse aggs, and, probably, a bunch of other things. It'll let us remove lots of wrapping. I've used this new power for the `avg` aggregation just to prove that it works outside of a unit test.
nik9000
added
>non-issue
:Core/Infra/Core
Collaborator
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
nik9000
commented
Jan 7, 2020
|
|
||
| public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { | ||
| return PARSER.parse(parser, new AvgAggregationBuilder(aggregationName), null); | ||
| return PARSER.parse(parser, aggregationName); |
Member
Author
There was a problem hiding this comment.
From here the payoff doesn't look that good, but we can remove the parses method entirely in a follow-up and call the parser directly. We can't do it right now because the order of the arguments is backwards.
nik9000
commented
Jan 7, 2020
| private static final ParseField SCORE_MODE_FIELD = new ParseField("score_mode"); | ||
|
|
||
| private static final ObjectParser<InnerBuilder, Void> QUERY_RESCORE_PARSER = new ObjectParser<>(NAME, null); | ||
| private static final ObjectParser<InnerBuilder, Void> QUERY_RESCORE_PARSER = new ObjectParser<>(NAME); |
Member
Author
There was a problem hiding this comment.
All of these are because it couldn't compile now that there are two methods that take two args. It is better not to have the null anyway.
nik9000
commented
Jan 7, 2020
| assertThat(ex.getMessage(), containsString("[foo] failed to parse field [int_array]")); | ||
| } | ||
|
|
||
| private static final Supplier<AtomicReference<String>> NEW_EMPTY_STRING_REF = AtomicReference::new; |
Member
Author
There was a problem hiding this comment.
This is because there are two ctors on atomic reference. One that looks like a function and one that looks like a supplier. The compiler was confused when I passed ::new to the ctor.
romseygeek
approved these changes
Jan 7, 2020
Contributor
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
LGTM, this will make a whole bunch of stuff much cleaner.
Member
Author
|
Oh neat! These compile failures don't show up in Eclipse. |
Member
Author
OK! I have a solution. A static method instead of a ctor. Testing locally. This is really a place where the builder pattern could be nice, but we've historically not liked it so I'm not using it. |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Jan 7, 2020
We *very* commonly have object with ctors like: ``` public Foo(String name) ``` And then declare a bunch of setters on the object. Every aggregation works like this, for example. This change teaches `ObjectParser` how to build these aggregations all on its own, without any help. This'll make it much cleaner to parse aggs, and, probably, a bunch of other things. It'll let us remove lots of wrapping. I've used this new power for the `avg` aggregation just to prove that it works outside of a unit test.
nik9000
added a commit
that referenced
this pull request
Jan 7, 2020
We *very* commonly have object with ctors like: ``` public Foo(String name) ``` And then declare a bunch of setters on the object. Every aggregation works like this, for example. This change teaches `ObjectParser` how to build these aggregations all on its own, without any help. This'll make it much cleaner to parse aggs, and, probably, a bunch of other things. It'll let us remove lots of wrapping. I've used this new power for the `avg` aggregation just to prove that it works outside of a unit test.
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
We *very* commonly have object with ctors like: ``` public Foo(String name) ``` And then declare a bunch of setters on the object. Every aggregation works like this, for example. This change teaches `ObjectParser` how to build these aggregations all on its own, without any help. This'll make it much cleaner to parse aggs, and, probably, a bunch of other things. It'll let us remove lots of wrapping. I've used this new power for the `avg` aggregation just to prove that it works outside of a unit test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We very commonly have object with ctors like:
And then declare a bunch of setters on the object. Every aggregation
works like this, for example. This change teaches
ObjectParserhow tobuild these aggregations all on its own, without any help. This'll make
it much cleaner to parse aggs, and, probably, a bunch of other things.
It'll let us remove lots of wrapping. I've used this new power for the
avgaggregation just to prove that it works outside of a unit test.