Added getProperty method to Aggregations

[colings86]

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.

[brwe]

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.

[colings86]

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.

[brwe]

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]

@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.

[jpountz]

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

[jpountz]

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]

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]

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

[colings86]

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

[brwe]

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

[jpountz]

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

[jpountz]

LGTM