Meta data support with each aggregation request/response #8279

Mpdreamz · 2014-10-29T21:07:14Z

This commit adds the ability to associate a bit of state with each individual aggregation.

The aggregation response can be hard to stitch back together without having a reference to the aggregation request. In many cases this is not available, many json serializer frameworks cache types globally or have a static deserialisation override mechanism. In these cases making the original request available, if at all possible, would be a hack.

The old facets returned _type which was just enough metadata to know what the originating facet type in the request was.

This PR takes _type one step further by introducing ANY arbitrary meta data. This could be further abused for instance by generic/automated aggregations that include UI state (color information, thresholds, user input states, etc) per aggregation.

Here's a gist of a sense session you can use to review this branch

This PR supersedes #6465 which got stale and horribly out of date with all the new aggregations in 1.2-1.4.

Tested on a cluster of multiple nodes as well.

Example request:

"aggs": {
    "name": {
      "terms": {
        "field": "title"
      },
      "meta": { "number_of_clicks" : 2 },
      "aggs": {
        "viewport" : {
          "meta" : { 
              "hasColor" : true 
          },
          "geo_bounds" : {
            "field" : "location"
          }
        },
        "empty_agg" : {
          "meta" : { 
             "string" : "returns"
          },
          "sum": {
            "field": "i_do_not_exist"
          }
        },

Example response:

"aggregations": {
      "name": {
         "meta": {
            "number_of_clicks": 2
         },
         "doc_count_error_upper_bound": 0,
         "sum_other_doc_count": 0,
         "buckets": [
            {
               "key": "banner",
               "doc_count": 1,
               "viewport": {
                  "meta": {
                     "hasColor": true
                  },
                  "bounds": {
                     "top_left": {
                        "lat": 28,
                        "lon": 21
                     },
       <redacted />

jpountz · 2014-11-02T20:52:55Z

src/main/java/org/elasticsearch/common/io/stream/StreamInput.java

+            }
+        }
+        return null;
+    }


The second method looks unnecessary? I mean you could just cast the result from readOptionalGenericValue? (null pointers can be casted to any class)

Thats good to know (not the case in C# :)), since this method is called in a bunch of places i did not want to introduce to many nullchecks and casts. Will change it to casts on readOptionalGenericValue()

jpountz · 2014-11-02T21:05:03Z

Hey @Mpdreamz the change looks good. The comment on the PR seems to suggest that any json document can be used as metadata (eg. a number) but the Java API requires a hash, is it intended?

Mpdreamz · 2014-11-02T21:07:50Z

Thanks for the review @jpountz !

No this is an oversight from copying it over from the previous PR that did allow any Json construct as meta, but now it has to be a hash (based on feedback on that PR).

Mpdreamz · 2014-11-03T08:40:53Z

@jpountz incorporated your feedback in the PR.

jpountz · 2014-11-03T09:01:10Z

config/elasticsearch.yml

@@ -222,7 +222,8 @@
 # Set a custom port to listen for HTTP traffic:
 #
 #http.port: 9200
-
+http.cors.enabled: true
+http.cors.allow-origin: http://localhost:8000


Left-over? :)

Argh! good catch, i'm an idiot :)

jpountz · 2014-11-03T09:25:19Z

I tried to look at how likely it would be to forget adding support to custom metadata, and I like the fact that you made the metadata a compulsory constructor argument of the aggregators, and that the parsing logic is shared. I think my only concern is that it would be easy to forget to read the metadata in the InternalAggregation objects, so maybe we could try to refactor InternalAggregation so that you could never forget to serialize/deserialize the metadata? I'm thinking about something like:

class InternalAggregation {

  public final void writeTo(StreamOutput out) { // final so that sub-classes cannot extend
    out.writeGenericValue(metaData);
    doWriteTo(out);
  }

  protected abstract void doWriteTo(StreamOutput out); // this is the method sub-classes should extend instead

}

(and similarly for readFrom)

Mpdreamz · 2014-11-03T11:49:11Z

++ I briefly discussed this with @spinscale but opted out for now to keep the PR simple, happy to refactor it! Should I also bump writing and reading the name to this method?

jpountz · 2014-11-03T11:51:45Z

Should I also bump writing and reading the name to this method?

That would be great!

jpountz · 2014-11-03T17:22:20Z

src/test/java/org/elasticsearch/search/aggregations/MetaDataTests.java

+import org.elasticsearch.test.ElasticsearchIntegrationTest;
+import org.hamcrest.Matchers;
+import org.junit.Test;
+import sun.security.util.BigInt;


jpountz · 2014-11-03T17:35:49Z

I just left two minor comments, but other than that it looks good to me, so feel free to push without further reviews.

Can you update the example request in the description of the PR to replace 2 with a json document since a map is now enforced?

This commit adds the ability to associate a bit of state with each individual aggregation. The aggregation response can be hard to stitch back together without having a reference to the aggregation request. In many cases this is not available, many json serializer frameworks cache types globally or have a static deserialisation override mechanism. In these cases making the original request available, if at all possible, would be a hack. The old facets returned `_type` which was just enough metadata to know what the originating facet type in the request was. This PR takes `_type` one step further by introducing ANY arbitrary meta data. This could be further <strike>ab</strike>used for instance by generic/automated aggregations that include UI state (color information, thresholds, user input states, etc) per aggregation.

javanna · 2014-11-04T10:10:33Z

I see that this is marked for 1.5 as well but wasn't backported to 1.x. Is there any concern around backwards compatibility? Thoughts @jpountz @Mpdreamz ?

javanna · 2014-11-04T10:10:54Z

docs/reference/search/aggregations.asciidoc

+            },
+            "meta": {
+                "color": "blue"
+            },


this "," can be removed ;)

jpountz · 2014-11-04T10:36:23Z

I'd be happy either way: the change has the appropriate version checks to be compatible with 1.x releases. I'm removing the tag until the change is backported (if we backport it).

Mpdreamz mentioned this pull request Oct 29, 2014

[Aggregations] Added _meta construct to inject metadata #6465

Closed

clintongormley added review >enhancement labels Oct 30, 2014

jpountz reviewed Nov 2, 2014
View reviewed changes

jpountz reviewed Nov 3, 2014
View reviewed changes

jpountz added v2.0.0-beta1 v1.5.0 and removed review labels Nov 3, 2014

Mpdreamz merged commit 82278bb into elastic:master Nov 3, 2014

javanna reviewed Nov 4, 2014
View reviewed changes

docs/reference/search/aggregations.asciidoc

},

"meta": {

"color": "blue"

},

Copy link

Member

javanna Nov 4, 2014

Choose a reason for hiding this comment

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

this "," can be removed ;)

jpountz removed the v1.5.0 label Nov 4, 2014

Mpdreamz mentioned this pull request Nov 5, 2014

Support for raw aggregation elastic/elasticsearch-net#1016

Closed

gmarz mentioned this pull request Nov 17, 2014

Aggregations: SingleBucketAggregation should create a single bucket #8510

Closed

clintongormley mentioned this pull request Dec 30, 2014

Return the aggregation type in the response #5867

Closed

clintongormley added the :Analytics/Aggregations Aggregations label Jun 7, 2015

clintongormley changed the title ~~[Aggregations] Meta data support~~ Meta data support with each aggregation request/response Jun 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meta data support with each aggregation request/response #8279

Meta data support with each aggregation request/response #8279

Mpdreamz commented Oct 29, 2014

jpountz Nov 2, 2014

Mpdreamz Nov 2, 2014

jpountz commented Nov 2, 2014

Mpdreamz commented Nov 2, 2014

Mpdreamz commented Nov 3, 2014

jpountz Nov 3, 2014

Mpdreamz Nov 3, 2014

jpountz commented Nov 3, 2014

Mpdreamz commented Nov 3, 2014

jpountz commented Nov 3, 2014

jpountz Nov 3, 2014

jpountz commented Nov 3, 2014

javanna commented Nov 4, 2014

javanna Nov 4, 2014

jpountz commented Nov 4, 2014

Meta data support with each aggregation request/response #8279

Meta data support with each aggregation request/response #8279

Conversation

Mpdreamz commented Oct 29, 2014

jpountz Nov 2, 2014

Choose a reason for hiding this comment

Mpdreamz Nov 2, 2014

Choose a reason for hiding this comment

jpountz commented Nov 2, 2014

Mpdreamz commented Nov 2, 2014

Mpdreamz commented Nov 3, 2014

jpountz Nov 3, 2014

Choose a reason for hiding this comment

Mpdreamz Nov 3, 2014

Choose a reason for hiding this comment

jpountz commented Nov 3, 2014

Mpdreamz commented Nov 3, 2014

jpountz commented Nov 3, 2014

jpountz Nov 3, 2014

Choose a reason for hiding this comment

jpountz commented Nov 3, 2014

javanna commented Nov 4, 2014

javanna Nov 4, 2014

Choose a reason for hiding this comment

jpountz commented Nov 4, 2014