Skip to content
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.

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

Added getProperty method to Aggregations #8421

Merged
merged 1 commit into from Nov 25, 2014

Conversation

Projects
None yet
3 participants
@colings86
Copy link
Member

commented Nov 10, 2014

This allows arbitrary properties to be retrieved from an aggregation tree. The property is specified using the same syntax as the
order parameter in the terms aggregation. If a property path contians a multi-bucket aggregation the property values from each bucket will be returned in an array.

} else if (aggName.equals("_key")) {
return getKey();
}
InternalAggregation aggregation = aggregations.get(aggName);

This comment has been minimized.

Copy link
@brwe

brwe Nov 10, 2014

Contributor

I am wondering if it makes sense to implement getProperty() for Aggregations as well and not just for Aggregation. For example in a test I would write something like

Aggregations agg = searchResponse.getAggregations();
Object o =agg.get("aggname").getProperty("path");

but if Aggregations also implemented getProperty() I would save another line and it is needed anyway here internally.

This comment has been minimized.

Copy link
@colings86

colings86 Nov 10, 2014

Author Member

I like this idea, and I can't think of a reason why it wouldn't work or not to do it. So I'll give it a go and add it to the PR.

default:
throw new ElasticsearchIllegalArgumentException("Found unknown path element [" + cornerString + "] in [" + getName() + "]");
}
String latLonString = path.get(0);

This comment has been minimized.

Copy link
@brwe

brwe Nov 10, 2014

Contributor

should this be get(1)? Also, is there a test for it?

This comment has been minimized.

Copy link
@colings86

colings86 Nov 10, 2014

Author Member

Yes, good catch. There should be a test for this as I thought I had tests for all aggregations. I will investigate

This comment has been minimized.

Copy link
@colings86

colings86 Nov 13, 2014

Author Member

This can't be tested or used as yet because of the aggregation name problem you mentioned below. When we have resolved #8434 we can test and use this functionality

@brwe

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2014

Not sure if this is related to this pull requests, but when I name my aggregations with for example "a.b" then getProperty will fail:

 @Test
    public void testNestedBucketsWithGetProperty() throws IOException, ExecutionException, InterruptedException {
        indexDocs();
        String firstAgg = "a.b";
        String secondAgg = "c.d";
        SearchResponse searchResponse = client().prepareSearch("index").setQuery(matchAllQuery())
                .addAggregation(terms("class").field("class").subAggregation(histogram(firstAgg).field("num").interval(1).subAggregation(avg(secondAgg).field("num")).subAggregation(avg("max").field("num"))))
                .get();
        assertThat(searchResponse.getHits().getTotalHits(), equalTo(30l));

        Aggregations agg = searchResponse.getAggregations();
        Object o = agg.get("class").getProperty(firstAgg + "." + secondAgg);
    }

    private void indexDocs() throws IOException, ExecutionException, InterruptedException {
        List<IndexRequestBuilder> indexRequests = new ArrayList<>();
        for (int i = 0; i< 3; i++) {
            for (int j = 0; j< 10; j++) {
                indexRequests.add(client().prepareIndex().setSource(jsonBuilder().startObject().field("class", Integer.toString(i)).field("num", j).endObject()).setIndex("index").setType("type"));
            }
        }
        indexRandom(true, indexRequests);
        ensureGreen("index");
    }
@colings86

This comment has been minimized.

Copy link
Member Author

commented Nov 10, 2014

@brwe I was talking to @clintongormley about this the other day. This will need to be fixed but outside of this PR as it also affects sort order for aggregations. For example, currently you cannot order a terms aggregation using the 99.9th percentile from the percentiles aggregation as you would need to specify your order as percents.99.9 which would be interpreted as an aggregation named percents.99 and a value named 9.

@@ -30,6 +30,8 @@
*/
String getName();

Object getProperty(String path);

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 10, 2014

Contributor

Can you add some javadocs since it's a public API?

return getDocCount();
} else if (aggName.equals("_key")) {
return getKey();
}

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 10, 2014

Contributor

Should the two above cases fail if the path contains more than one element?

@Override
public Object getProperty(List<String> path) {
if (path.isEmpty() || path.size() == 1 && "value".equals(path.get(0))) {
return value();

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 10, 2014

Contributor

Should it rather return "this" when the path is empty?

This comment has been minimized.

Copy link
@colings86

colings86 Nov 10, 2014

Author Member

I did think about this but in the order path of a terms aggregation does this for single value numeric metric aggregations and I thought we would want to keep the same functionality?

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 11, 2014

Contributor

I see... But then it is not consistent with other aggs that return themselves when the path is empty?

Would it solve the issue to modify ordering so that it calls value() when the path point to a single-value agg since there is no ambiguity in that case?

This comment has been minimized.

Copy link
@colings86

colings86 Nov 13, 2014

Author Member

I'm not sure I follow you here. Can you explain?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Nov 10, 2014

I like this new API as it is very convenient. I left a couple of comments/questions.

I know it's outside of the scope of this PR but I'm wondering if we should make the path syntax compatible with some standard like json path for the next major release, in a way that it would be compatible with calling the same json path expression on the json response from elasticsearch. In addition to making it easier to approach to new users, it would also allow to address data that would be stored in a specific bucket, which is not supported today (although I could imagine sorting or reducers make use of it)?

@colings86

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2014

I like the idea of supporting JSON path expressions. From looking at the spec for JSON path, we are not that far away in what we do already, although we need to replace the '>' character with '.' in our current syntax. One thing we would need to sort out is what happens if the aggregation name contains a dot. This currently causes issues and we need to come up with a resolution order for resolving names and values with a dot, or remove support for agg names with a dot.

@colings86

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2014

Added a new issue for the JSONPath suggest here: #8434

@colings86

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2014

@brwe @jpountz thanks for reviewing, I have addressed/replied to your comments and pushed a new commit. Would you mind taking another look?

@@ -392,6 +396,9 @@ public void singleValuedField_WithSubAggregation() throws Exception {
assertThat(range, notNullValue());
assertThat(range.getName(), equalTo("range"));
assertThat(range.getBuckets().size(), equalTo(3));
Object[] propertiesKeys = (Object[]) range.getProperty("_key");
Object[] propertiesDocCounts = (Object[]) range.getProperty("_count");
Object[] propertiesCounts = (Object[]) range.getProperty("sum");

This comment has been minimized.

Copy link
@brwe

brwe Nov 17, 2014

Contributor

I think this should be Object[] propertiesCounts = (Object[]) histo.getProperty("sum.value"); else the tests don't pass.

if (aggregation == null) {
throw new ElasticsearchIllegalArgumentException("Cannot find an aggregation named [" + aggName + "] in [" + getName() + "]");
}
return aggregation.getProperty(path.subList(1, path.size()));

This comment has been minimized.

Copy link
@brwe

brwe Nov 17, 2014

Contributor

I wonder if it would make sense to wrap the result for a SingleBucketAggregation in an array with only one entry. This would make it easier to process the result of getProperty in case the underlying aggregation type changes. Here is an example of what I mean:

brwe@1cd7aae

In the test, if I had to process the result of getProperty() I would have to distinguish if the second level aggregation is single or multi buckets aggregation. If single bucket aggregation would wrap it in an array then the dimension of the result array would only depend on the depth of the aggregation and not on the aggregation type. This would make post processing more convenient.

I think this is a general inconsistency with aggregations, it also goes for json output. Also, I think SingleBucketAggregations should have a getBuckets() method which would then only return one bucket. Maybe this is worth a separate issue?

@jpountz

View changes

src/main/java/org/elasticsearch/search/aggregations/bucket/InternalSingleBucketAggregation.java Outdated
} else {
String aggName = path.get(0);
if (aggName.equals("_count")) {
return getDocCount();

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 25, 2014

Contributor

Can you check that there are no more elements in the path before returning?

@jpountz

View changes

src/main/java/org/elasticsearch/search/aggregations/InternalAggregations.java Outdated
}

public Object getProperty(List<String> path) {
String aggName = path.get(0);

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 25, 2014

Contributor

Should it return itself if the path is empty?

@jpountz

View changes

src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java Outdated
public static abstract class InternalBucket implements Bucket {
public Object getProperty(String containingAggName, List<String> path) {
Aggregations aggregations = getAggregations();
String aggName = path.get(0);

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 25, 2014

Contributor

Should it return itself if the path is empty?

@jpountz

View changes

src/main/java/org/elasticsearch/search/aggregations/bucket/terms/InternalOrder.java Outdated
final Aggregator aggregator = path.resolveAggregator(termsAggregator);
final String key = path.tokens[path.tokens.length - 1].key;
final String key = path.getPathElements().get(path.getPathElements().size() - 1).key;

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 25, 2014

Contributor

Can we use the lastPathElement method here?

@jpountz

View changes

...main/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetric.java Outdated
@@ -112,6 +113,14 @@ public Type type() {
return TYPE;
}

public Object getProperty(List<String> path) {
if (path.isEmpty() || path.size() == 1 && "value".equals(path.get(0))) {
return aggregation;

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 25, 2014

Contributor

Should it rather return this when the path is empty?

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2014

LGTM

@colings86 colings86 force-pushed the colings86:feature/aggsGetProperty branch 2 times, most recently Nov 25, 2014

@colings86 colings86 removed the review label Nov 25, 2014

@colings86 colings86 force-pushed the colings86:feature/aggsGetProperty branch Nov 25, 2014

@colings86 colings86 force-pushed the colings86:feature/aggsGetProperty branch Nov 25, 2014

Aggregations: Added getProperty method to Aggregations
This allows arbitrary properties to be retrieved from an aggregation tree. The property is specified using the same syntax as the
order parameter in the terms aggregation. If a property path contians a multi-bucket aggregation the property values from each bucket will be returned in an array.

@colings86 colings86 force-pushed the colings86:feature/aggsGetProperty branch to c420a17 Nov 25, 2014

@colings86 colings86 merged commit c420a17 into elastic:master Nov 25, 2014

@colings86 colings86 deleted the colings86:feature/aggsGetProperty branch Nov 25, 2014

@clintongormley clintongormley changed the title Aggregations: Added getProperty method to Aggregations Added getProperty method to Aggregations Jun 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.