From 787519ee4ced81c8408f3050685d4f591622b9b8 Mon Sep 17 00:00:00 2001
From: Adrien Grand <jpountz@gmail.com>
Date: Fri, 9 Dec 2016 09:48:48 +0100
Subject: [PATCH] Fix `other_bucket` on the `filters` agg to be enabled if a
key is set. (#21994)
Closes #21951
---
.../filters/FiltersAggregationBuilder.java | 7 +-
.../aggregations/metrics/FiltersTests.java | 50 +++++
docs/build.gradle | 1 -
.../bucket/filters-aggregation.asciidoc | 178 ++++++++----------
4 files changed, 135 insertions(+), 101 deletions(-)
diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregationBuilder.java
index 2cd4f508ccb..989dcd45353 100644
--- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregationBuilder.java
+++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregationBuilder.java
@@ -209,7 +209,7 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
XContentParser.Token token = null;
String currentFieldName = null;
String otherBucketKey = null;
- Boolean otherBucket = false;
+ Boolean otherBucket = null;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
@@ -260,8 +260,9 @@ public class FiltersAggregationBuilder extends AbstractAggregationBuilder<Filter
}
}
- if (otherBucket && otherBucketKey == null) {
- otherBucketKey = "_other_";
+ if (otherBucket == null && otherBucketKey != null) {
+ // automatically enable the other bucket if a key is set, as per the doc
+ otherBucket = true;
}
FiltersAggregationBuilder factory;
diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/FiltersTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/FiltersTests.java
index 89fc38b7cd8..a1fe372687b 100644
--- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/FiltersTests.java
+++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/FiltersTests.java
@@ -19,11 +19,23 @@
package org.elasticsearch.search.aggregations.metrics;
+import org.elasticsearch.common.ParseFieldMatcher;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.query.MatchNoneQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryBuilders;
+import org.elasticsearch.index.query.QueryParseContext;
+import org.elasticsearch.indices.query.IndicesQueriesRegistry;
import org.elasticsearch.search.aggregations.BaseAggregationTestCase;
import org.elasticsearch.search.aggregations.bucket.filters.FiltersAggregator.KeyedFilter;
+import org.elasticsearch.search.slice.SliceBuilder;
+
+import java.io.IOException;
+
import org.elasticsearch.search.aggregations.bucket.filters.FiltersAggregationBuilder;
public class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuilder> {
@@ -73,4 +85,42 @@ public class FiltersTests extends BaseAggregationTestCase<FiltersAggregationBuil
assertEquals("aaa", original[1].key());
}
+ public void testOtherBucket() throws IOException {
+ XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
+ builder.startObject();
+ builder.startArray("filters").endArray();
+ builder.endObject();
+ XContentParser parser = XContentHelper.createParser(shuffleXContent(builder).bytes());
+ parser.nextToken();
+ QueryParseContext context = new QueryParseContext(new IndicesQueriesRegistry(), parser,
+ ParseFieldMatcher.STRICT);
+ FiltersAggregationBuilder filters = FiltersAggregationBuilder.parse("agg_name", context);
+ // The other bucket is disabled by default
+ assertFalse(filters.otherBucket());
+
+ builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
+ builder.startObject();
+ builder.startArray("filters").endArray();
+ builder.field("other_bucket_key", "some_key");
+ builder.endObject();
+ parser = XContentHelper.createParser(shuffleXContent(builder).bytes());
+ parser.nextToken();
+ context = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.STRICT);
+ filters = FiltersAggregationBuilder.parse("agg_name", context);
+ // but setting a key enables it automatically
+ assertTrue(filters.otherBucket());
+
+ builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
+ builder.startObject();
+ builder.startArray("filters").endArray();
+ builder.field("other_bucket", false);
+ builder.field("other_bucket_key", "some_key");
+ builder.endObject();
+ parser = XContentHelper.createParser(shuffleXContent(builder).bytes());
+ parser.nextToken();
+ context = new QueryParseContext(new IndicesQueriesRegistry(), parser, ParseFieldMatcher.STRICT);
+ filters = FiltersAggregationBuilder.parse("agg_name", context);
+ // unless the other bucket is explicitly disabled
+ assertFalse(filters.otherBucket());
+ }
}
diff --git a/docs/build.gradle b/docs/build.gradle
index 891a8400e4c..5ba625cad94 100644
--- a/docs/build.gradle
+++ b/docs/build.gradle
@@ -28,7 +28,6 @@ buildRestTests.expectedUnconvertedCandidates = [
'reference/aggregations/bucket/daterange-aggregation.asciidoc',
'reference/aggregations/bucket/diversified-sampler-aggregation.asciidoc',
'reference/aggregations/bucket/filter-aggregation.asciidoc',
- 'reference/aggregations/bucket/filters-aggregation.asciidoc',
'reference/aggregations/bucket/geodistance-aggregation.asciidoc',
'reference/aggregations/bucket/geohashgrid-aggregation.asciidoc',
'reference/aggregations/bucket/global-aggregation.asciidoc',
diff --git a/docs/reference/aggregations/bucket/filters-aggregation.asciidoc b/docs/reference/aggregations/bucket/filters-aggregation.asciidoc
index 7ba9d22d8d9..97b7f2a50e1 100644
--- a/docs/reference/aggregations/bucket/filters-aggregation.asciidoc
+++ b/docs/reference/aggregations/bucket/filters-aggregation.asciidoc
@@ -9,62 +9,61 @@ Example:
[source,js]
--------------------------------------------------
+PUT /logs/message/_bulk?refresh
+{ "index" : { "_id" : 1 } }
+{ "body" : "warning: page could not be rendered" }
+{ "index" : { "_id" : 2 } }
+{ "body" : "authentication error" }
+{ "index" : { "_id" : 3 } }
+{ "body" : "warning: connection timed out" }
+
+GET logs/_search
{
+ "size": 0,
"aggs" : {
"messages" : {
"filters" : {
"filters" : {
- "errors" : { "term" : { "body" : "error" }},
- "warnings" : { "term" : { "body" : "warning" }}
- }
- },
- "aggs" : {
- "monthly" : {
- "histogram" : {
- "field" : "timestamp",
- "interval" : "1M"
- }
+ "errors" : { "match" : { "body" : "error" }},
+ "warnings" : { "match" : { "body" : "warning" }}
}
}
}
}
}
--------------------------------------------------
+// CONSOLE
In the above example, we analyze log messages. The aggregation will build two
collection (buckets) of log messages - one for all those containing an error,
-and another for all those containing a warning. And for each of these buckets
-it will break them down by month.
+and another for all those containing a warning.
Response:
[source,js]
--------------------------------------------------
-...
- "aggs" : {
- "messages" : {
- "buckets" : {
- "errors" : {
- "doc_count" : 34,
- "monthly" : {
- "buckets" : [
- ... // the histogram monthly breakdown
- ]
- }
+{
+ "took": 9,
+ "timed_out": false,
+ "_shards": ...,
+ "hits": ...,
+ "aggregations": {
+ "messages": {
+ "buckets": {
+ "errors": {
+ "doc_count": 1
},
- "warnings" : {
- "doc_count" : 439,
- "monthly" : {
- "buckets" : [
- ... // the histogram monthly breakdown
- ]
- }
+ "warnings": {
+ "doc_count": 2
}
}
}
}
-...
+}
--------------------------------------------------
+// TESTRESPONSE[s/"took": 9/"took": $body.took/]
+// TESTRESPONSE[s/"_shards": \.\.\./"_shards": $body._shards/]
+// TESTRESPONSE[s/"hits": \.\.\./"hits": $body.hits/]
==== Anonymous filters
@@ -73,58 +72,51 @@ following request:
[source,js]
--------------------------------------------------
+GET logs/_search
{
+ "size": 0,
"aggs" : {
"messages" : {
"filters" : {
"filters" : [
- { "term" : { "body" : "error" }},
- { "term" : { "body" : "warning" }}
+ { "match" : { "body" : "error" }},
+ { "match" : { "body" : "warning" }}
]
- },
- "aggs" : {
- "monthly" : {
- "histogram" : {
- "field" : "timestamp",
- "interval" : "1M"
- }
- }
}
}
}
}
--------------------------------------------------
+// CONSOLE
+// TEST[continued]
The filtered buckets are returned in the same order as provided in the
request. The response for this example would be:
[source,js]
--------------------------------------------------
-...
- "aggs" : {
- "messages" : {
- "buckets" : [
+{
+ "took": 4,
+ "timed_out": false,
+ "_shards": ...,
+ "hits": ...,
+ "aggregations": {
+ "messages": {
+ "buckets": [
{
- "doc_count" : 34,
- "monthly" : {
- "buckets" : [
- ... // the histogram monthly breakdown
- ]
- }
+ "doc_count": 1
},
{
- "doc_count" : 439,
- "monthly" : {
- "buckets" : [
- ... // the histogram monthly breakdown
- ]
- }
+ "doc_count": 2
}
]
}
}
-...
+}
--------------------------------------------------
+// TESTRESPONSE[s/"took": 4/"took": $body.took/]
+// TESTRESPONSE[s/"_shards": \.\.\./"_shards": $body._shards/]
+// TESTRESPONSE[s/"hits": \.\.\./"hits": $body.hits/]
==== `Other` Bucket
@@ -142,64 +134,56 @@ The following snippet shows a response where the `other` bucket is requested to
[source,js]
--------------------------------------------------
+PUT logs/message/4?refresh
{
+ "body": "info: user Bob logged out"
+}
+
+GET logs/_search
+{
+ "size": 0,
"aggs" : {
"messages" : {
"filters" : {
"other_bucket_key": "other_messages",
"filters" : {
- "errors" : { "term" : { "body" : "error" }},
- "warnings" : { "term" : { "body" : "warning" }}
- }
- },
- "aggs" : {
- "monthly" : {
- "histogram" : {
- "field" : "timestamp",
- "interval" : "1M"
- }
+ "errors" : { "match" : { "body" : "error" }},
+ "warnings" : { "match" : { "body" : "warning" }}
}
}
}
}
}
--------------------------------------------------
+// CONSOLE
+// TEST[continued]
The response would be something like the following:
[source,js]
--------------------------------------------------
-...
- "aggs" : {
- "messages" : {
- "buckets" : {
- "errors" : {
- "doc_count" : 34,
- "monthly" : {
- "buckets" : [
- ... // the histogram monthly breakdown
- ]
- }
- },
- "warnings" : {
- "doc_count" : 439,
- "monthly" : {
- "buckets" : [
- ... // the histogram monthly breakdown
- ]
- }
- },
- "other_messages" : {
- "doc_count" : 237,
- "monthly" : {
- "buckets" : [
- ... // the histogram monthly breakdown
- ]
- }
- }
+{
+ "took": 3,
+ "timed_out": false,
+ "_shards": ...,
+ "hits": ...,
+ "aggregations": {
+ "messages": {
+ "buckets": {
+ "errors": {
+ "doc_count": 1
+ },
+ "warnings": {
+ "doc_count": 2
+ },
+ "other_messages": {
+ "doc_count": 1
}
}
}
}
-...
+}
--------------------------------------------------
+// TESTRESPONSE[s/"took": 3/"took": $body.took/]
+// TESTRESPONSE[s/"_shards": \.\.\./"_shards": $body._shards/]
+// TESTRESPONSE[s/"hits": \.\.\./"hits": $body.hits/]