Skip to content

Add more missing AggregationBuilder getters #25198

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 14, 2017
Merged

Add more missing AggregationBuilder getters #25198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2859da
Add more missing AggregationBuilder getters
polyfractal May 18, 2017
1a3e4f2
Return an unmodifiable metadata map
polyfractal Jun 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions core/src/main/java/org/elasticsearch/search/aggregations/AbstractAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.search.internal.SearchContext;

import java.io.IOException;
import java.util.Collections;
import java.util.Map;
import java.util.Objects;

Expand Down Expand Up @@ -115,6 +116,11 @@ public AB setMetaData(Map<String, Object> metaData) {
return (AB) this;
}

@Override
public Map<String, Object> getMetaData() {
Copy link
Member

@cbuescher cbuescher Jun 14, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@polyfractal polyfractal Jun 14, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@cbuescher cbuescher Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@cbuescher cbuescher Jun 14, 2017

Choose a reason for hiding this comment

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

Talked f2f, makes sense to get a way to read the metaData here.

return Collections.unmodifiableMap(metaData);
}

@Override
public final String getWriteableName() {
// We always use the type of the aggregation as the writeable name
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ public String getName() {
@Override
public abstract AggregationBuilder setMetaData(Map<String, Object> metaData);

/** Return any associated metadata with this {@link AggregationBuilder}. */
public abstract Map<String, Object> getMetaData();

/** Add a sub aggregation to this builder. */
public abstract AggregationBuilder subAggregation(AggregationBuilder aggregation);

Expand Down
28 changes: 28 additions & 0 deletions ...main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,13 @@ public TermsAggregationBuilder size(int size) {
return this;
}

/**
* Returns the number of term buckets currently configured
*/
public int size() {
return bucketCountThresholds.getRequiredSize();
}

/**
* Sets the shard_size - indicating the number of term buckets each shard
* will return to the coordinating node (the node that coordinates the
Expand All @@ -163,6 +170,13 @@ public TermsAggregationBuilder shardSize(int shardSize) {
return this;
}

/**
* Returns the number of term buckets per shard that are currently configured
*/
public int shardSize() {
return bucketCountThresholds.getShardSize();
}

/**
* Set the minimum document count terms should have in order to appear in
* the response.
Expand All @@ -176,6 +190,13 @@ public TermsAggregationBuilder minDocCount(long minDocCount) {
return this;
}

/**
* Returns the minimum document count required per term
*/
public long minDocCount() {
return bucketCountThresholds.getMinDocCount();
}

/**
* Set the minimum document count terms should have on the shard in order to
* appear in the response.
Expand All @@ -189,6 +210,13 @@ public TermsAggregationBuilder shardMinDocCount(long shardMinDocCount) {
return this;
}

/**
* Returns the minimum document count required per term, per shard
*/
public long shardMinDocCount() {
return bucketCountThresholds.getShardMinDocCount();
}

/** Set a new order on this builder and return the builder so that calls
* can be chained. A tie-breaker may be added to avoid non-deterministic ordering. */
public TermsAggregationBuilder order(BucketOrder order) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/elasticsearch/search/aggregations/metrics/sum/InternalSum.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
public class InternalSum extends InternalNumericMetricsAggregation.SingleValue implements Sum {
private final double sum;

InternalSum(String name, double sum, DocValueFormat formatter, List<PipelineAggregator> pipelineAggregators,
public InternalSum(String name, double sum, DocValueFormat formatter, List<PipelineAggregator> pipelineAggregators,
Map<String, Object> metaData) {
super(name, pipelineAggregators, metaData);
this.sum = sum;
Expand Down