Add more missing AggregationBuilder getters #25198
Conversation
- getMetadata for all aggs - various getters on TermsAggBuilder (without "get" prefix to maintain convention) - Also makes InternalSum's ctor public, to follow suit of other metrics (min/max/avg/etc)
There was a problem hiding this comment.
@polyfractal thanks, I left a question around the need to access the metaData field from outside, the rest looks good.
| @@ -116,6 +116,11 @@ public AB setMetaData(Map<String, Object> metaData) { | |||
| } | |||
|
|
|||
| @Override | |||
| public Map<String, Object> getMetaData() { | |||
There was a problem hiding this comment.
Why does the metaData need to be accessible for anything other than subclasses? My current understanding is that it is used to associate state in the request with the aggregation response, so I'm not sure if we need to expose these properties.
There was a problem hiding this comment.
If the user is attempting to add metadata to a request, and doesn't have all the metadata in one place in their application, it wouldn't be uncommon to do something like:
Map<String, Object> newMetadata = new HashMap<>();
newMetadata.putAll(aggBuilder.getMetadata());
newMetadata.put("foo", "bar");
aggBuilder.setMetadata(newMetadata);E.g. without a way to get metadata, the metadata has to be compiled into a single location in the user's application so it can be set all at once, which makes building agg trees cumbersome.
newMetadata.
Also, being able to get the metadata would is useful where aggs are built programmatically and you want to verify the final agg builder in a test.
There was a problem hiding this comment.
I was trying to understand if there is a specific reason already to do this. My understanding of "metaData" in aggegation is that it is something that is piggy backed from the request to the response and currently is mostly used by our own clients, so it shouldn't change once it has been set once. Since this it the aggregation builder it might be okay to allow this at this point. Maybe @colings86 has an opinion on this?
There was a problem hiding this comment.
Talked f2f, makes sense to get a way to read the metaData here.
| @@ -116,6 +116,11 @@ public AB setMetaData(Map<String, Object> metaData) { | |||
| } | |||
|
|
|||
| @Override | |||
| public Map<String, Object> getMetaData() { | |||
| return metaData; | |||
There was a problem hiding this comment.
If we need to expose this, maybe return an unmodifyable map here.
There was a problem hiding this comment.
Sure, I can add that. Do we have a policy that we don't want to allow side-effects on builders, and force a get/set cycle rather than modifying in-place?
There was a problem hiding this comment.
There's no definite policy, but in the Query builders we often try to limit accress to internal maps etc... once they are set. The reason there is that those objects are not only client facing "builders" any more but also our internal representation of things. This might be different in the aggregation builder case, I think its still safer to allow only read access to the metaData once it has been set.
There was a problem hiding this comment.
I'm okay if we can make this an unmodifiable map.
polyfractal
force-pushed
the
aggBuilderGetters
branch
from
ddd2840 to
1a3e4f2
Compare
|
Thanks @cbuescher! :) |
* master: Upgrade icu4j for the ICU analysis plugin to 59.1 (elastic#25243) move assertBusy to use CheckException (elastic#25246) Use SPI in High Level Rest Client to load XContent parsers (elastic#25098) [TEST] test that low level REST client leaves path untouched (elastic#25193) Speed up PK lookups at index time. (elastic#19856) [Docs] Fix documentation for percentiles bucket aggregation (elastic#25229) Upgrade to lucene-7.0.0-snapshot-92b1783. (elastic#25222) Build: Add master flag for disabling bwc tests (elastic#25230) Scripting: Rename SearchScript.needsScores to needs_score (elastic#25235) Support script context stateful factory in Painless. (elastic#25233) FastVectorHighlighter should not cache the field query globally (elastic#25197) Remove QUERY_AND_FETCH BWC for pre-5.3.0 nodes (elastic#25223) Add more missing AggregationBuilder getters (elastic#25198) Extract the snapshot/restore full cluster restart tests from the translog full cluster restart tests (elastic#25204)
* master: (44 commits) Upgrade icu4j for the ICU analysis plugin to 59.1 (elastic#25243) move assertBusy to use CheckException (elastic#25246) Use SPI in High Level Rest Client to load XContent parsers (elastic#25098) [TEST] test that low level REST client leaves path untouched (elastic#25193) Speed up PK lookups at index time. (elastic#19856) [Docs] Fix documentation for percentiles bucket aggregation (elastic#25229) Upgrade to lucene-7.0.0-snapshot-92b1783. (elastic#25222) Build: Add master flag for disabling bwc tests (elastic#25230) Scripting: Rename SearchScript.needsScores to needs_score (elastic#25235) Support script context stateful factory in Painless. (elastic#25233) FastVectorHighlighter should not cache the field query globally (elastic#25197) Remove QUERY_AND_FETCH BWC for pre-5.3.0 nodes (elastic#25223) Add more missing AggregationBuilder getters (elastic#25198) Extract the snapshot/restore full cluster restart tests from the translog full cluster restart tests (elastic#25204) Refactor TransportShardBulkAction.executeUpdateRequest and add tests Make sure range queries are correctly profiled. (elastic#25108) Test: allow setting socket timeout for rest client (elastic#25221) Migration docs for elastic#25080 (elastic#25218) Remove `discovery.type` BWC layer from the EC2/Azure/GCE plugins elastic#25080 When stopping via systemd only kill the JVM, not its control group (elastic#25195) ...
Some more minor tweaks to make agg builders more consistent.
@cbuescher would you mind taking a look at this? Want to make sure I'm not breaking anything related to the java client project. :)