Fix `other_bucket` on the `filters` agg to be enabled if a key is set. (#21994)

Closes #21951

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;

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());
+    }
 }

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',

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/]