From 8f8d3a5556abaaabbfbfd588fb96d908cc330ff9 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 23 Aug 2018 16:15:37 -0400 Subject: [PATCH 1/3] [Rollup] Return empty response when aggs are missing (#32796) If a search request doesn't contain aggs (or an empty agg object), we should just retun an empty response. This is how the normal search API works if you specify zero hits and empty aggs. The existing behavior throws an exception because it tries to send an empty msearch. Closes #32256 --- .../en/rest-api/rollup/rollup-search.asciidoc | 2 + .../rollup/RollupResponseTranslator.java | 44 ++++++++++++++----- .../action/TransportRollupSearchAction.java | 17 ++++--- .../RollupResponseTranslationTests.java | 9 ++-- .../rollup/action/SearchActionTests.java | 13 +++--- .../test/rollup/rollup_search.yml | 14 ++++++ 6 files changed, 74 insertions(+), 25 deletions(-) diff --git a/x-pack/docs/en/rest-api/rollup/rollup-search.asciidoc b/x-pack/docs/en/rest-api/rollup/rollup-search.asciidoc index f595d52ec10..115ef8fb043 100644 --- a/x-pack/docs/en/rest-api/rollup/rollup-search.asciidoc +++ b/x-pack/docs/en/rest-api/rollup/rollup-search.asciidoc @@ -101,6 +101,7 @@ GET /sensor_rollup/_rollup_search -------------------------------------------------- // CONSOLE // TEST[setup:sensor_prefab_data] +// TEST[s/_rollup_search/_rollup_search?filter_path=took,timed_out,terminated_early,_shards,hits,aggregations/] The query is targeting the `sensor_rollup` data, since this contains the rollup data as configured in the job. A `max` aggregation has been used on the `temperature` field, yielding the following response: @@ -194,6 +195,7 @@ GET sensor-1,sensor_rollup/_rollup_search <1> -------------------------------------------------- // CONSOLE // TEST[continued] +// TEST[s/_rollup_search/_rollup_search?filter_path=took,timed_out,terminated_early,_shards,hits,aggregations/] <1> Note the URI now searches `sensor-1` and `sensor_rollup` at the same time When the search is executed, the Rollup Search endpoint will do two things: diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupResponseTranslator.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupResponseTranslator.java index 4042e98ef93..a38adf5d9de 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupResponseTranslator.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupResponseTranslator.java @@ -238,11 +238,23 @@ public class RollupResponseTranslator { ? (InternalAggregations)liveResponse.getAggregations() : InternalAggregations.EMPTY; - rolledResponses.forEach(r -> { - if (r == null || r.getAggregations() == null || r.getAggregations().asList().size() == 0) { - throw new RuntimeException("Expected to find aggregations in rollup response, but none found."); + int missingRollupAggs = rolledResponses.stream().mapToInt(searchResponse -> { + if (searchResponse == null + || searchResponse.getAggregations() == null + || searchResponse.getAggregations().asList().size() == 0) { + return 1; } - }); + return 0; + }).sum(); + + // We had no rollup aggs, so there is nothing to process + if (missingRollupAggs == rolledResponses.size()) { + // Return an empty response, but make sure we include all the shard, failure, etc stats + return mergeFinalResponse(liveResponse, rolledResponses, InternalAggregations.EMPTY); + } else if (missingRollupAggs > 0 && missingRollupAggs != rolledResponses.size()) { + // We were missing some but not all the aggs, unclear how to handle this. Bail. + throw new RuntimeException("Expected to find aggregations in rollup response, but none found."); + } // The combination process returns a tree that is identical to the non-rolled // which means we can use aggregation's reduce method to combine, just as if @@ -275,27 +287,39 @@ public class RollupResponseTranslator { new InternalAggregation.ReduceContext(reduceContext.bigArrays(), reduceContext.scriptService(), true)); } - // TODO allow profiling in the future - InternalSearchResponse combinedInternal = new InternalSearchResponse(SearchHits.empty(), currentTree, null, null, - rolledResponses.stream().anyMatch(SearchResponse::isTimedOut), - rolledResponses.stream().anyMatch(SearchResponse::isTimedOut), - rolledResponses.stream().mapToInt(SearchResponse::getNumReducePhases).sum()); + return mergeFinalResponse(liveResponse, rolledResponses, currentTree); + } + + private static SearchResponse mergeFinalResponse(SearchResponse liveResponse, List rolledResponses, + InternalAggregations aggs) { int totalShards = rolledResponses.stream().mapToInt(SearchResponse::getTotalShards).sum(); int sucessfulShards = rolledResponses.stream().mapToInt(SearchResponse::getSuccessfulShards).sum(); int skippedShards = rolledResponses.stream().mapToInt(SearchResponse::getSkippedShards).sum(); long took = rolledResponses.stream().mapToLong(r -> r.getTook().getMillis()).sum() ; + boolean isTimedOut = rolledResponses.stream().anyMatch(SearchResponse::isTimedOut); + boolean isTerminatedEarly = rolledResponses.stream() + .filter(r -> r.isTerminatedEarly() != null) + .anyMatch(SearchResponse::isTerminatedEarly); + int numReducePhases = rolledResponses.stream().mapToInt(SearchResponse::getNumReducePhases).sum(); + if (liveResponse != null) { totalShards += liveResponse.getTotalShards(); sucessfulShards += liveResponse.getSuccessfulShards(); skippedShards += liveResponse.getSkippedShards(); took = Math.max(took, liveResponse.getTook().getMillis()); + isTimedOut = isTimedOut && liveResponse.isTimedOut(); + isTerminatedEarly = isTerminatedEarly && liveResponse.isTerminatedEarly(); + numReducePhases += liveResponse.getNumReducePhases(); } + InternalSearchResponse combinedInternal = new InternalSearchResponse(SearchHits.empty(), aggs, null, null, + isTimedOut, isTerminatedEarly, numReducePhases); + // Shard failures are ignored atm, so returning an empty array is fine return new SearchResponse(combinedInternal, null, totalShards, sucessfulShards, skippedShards, - took, ShardSearchFailure.EMPTY_ARRAY, rolledResponses.get(0).getClusters()); + took, ShardSearchFailure.EMPTY_ARRAY, rolledResponses.get(0).getClusters()); } /** diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java index c63ab96fa25..ea0319c3432 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/action/TransportRollupSearchAction.java @@ -155,6 +155,18 @@ public class TransportRollupSearchAction extends TransportAction validatedCaps = new HashSet<>(); sourceAgg.getAggregatorFactories() @@ -248,11 +260,6 @@ public class TransportRollupSearchAction extends TransportAction jobCaps) { diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java index 35d9f0d133a..73a4d0665c4 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java @@ -198,10 +198,11 @@ public class RollupResponseTranslationTests extends AggregatorTestCase { BigArrays bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService()); ScriptService scriptService = mock(ScriptService.class); - Exception e = expectThrows(RuntimeException.class, - () -> RollupResponseTranslator.combineResponses(msearch, - new InternalAggregation.ReduceContext(bigArrays, scriptService, true))); - assertThat(e.getMessage(), equalTo("Expected to find aggregations in rollup response, but none found.")); + SearchResponse response = RollupResponseTranslator.combineResponses(msearch, + new InternalAggregation.ReduceContext(bigArrays, scriptService, true)); + assertNotNull(response); + Aggregations responseAggs = response.getAggregations(); + assertThat(responseAggs.asList().size(), equalTo(0)); } public void testMissingRolledIndex() { diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java index 069e23e4093..3cc6190db30 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/action/SearchActionTests.java @@ -307,21 +307,22 @@ public class SearchActionTests extends ESTestCase { assertThat(e.getMessage(), equalTo("Rollup search does not support explaining.")); } - public void testNoAgg() { - String[] normalIndices = new String[]{randomAlphaOfLength(10)}; + public void testNoRollupAgg() { + String[] normalIndices = new String[]{}; String[] rollupIndices = new String[]{randomAlphaOfLength(10)}; TransportRollupSearchAction.RollupSearchContext ctx = new TransportRollupSearchAction.RollupSearchContext(normalIndices, rollupIndices, Collections.emptySet()); SearchSourceBuilder source = new SearchSourceBuilder(); source.query(new MatchAllQueryBuilder()); source.size(0); - SearchRequest request = new SearchRequest(normalIndices, source); + SearchRequest request = new SearchRequest(rollupIndices, source); NamedWriteableRegistry registry = mock(NamedWriteableRegistry.class); - Exception e = expectThrows(IllegalArgumentException.class, - () -> TransportRollupSearchAction.createMSearchRequest(request, registry, ctx)); - assertThat(e.getMessage(), equalTo("Rollup requires at least one aggregation to be set.")); + MultiSearchRequest msearch = TransportRollupSearchAction.createMSearchRequest(request, registry, ctx); + assertThat(msearch.requests().size(), equalTo(1)); + assertThat(msearch.requests().get(0), equalTo(request)); } + public void testNoLiveNoRollup() { String[] normalIndices = new String[0]; String[] rollupIndices = new String[0]; diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/rollup_search.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/rollup_search.yml index d401d5c69ba..e2f1174665e 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/rollup_search.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/rollup/rollup_search.yml @@ -152,6 +152,20 @@ setup: - match: { aggregations.histo.buckets.3.key_as_string: "2017-01-01T08:00:00.000Z" } - match: { aggregations.histo.buckets.3.doc_count: 20 } +--- +"Empty aggregation": + + - do: + xpack.rollup.rollup_search: + index: "foo_rollup" + body: + size: 0 + aggs: {} + + - length: { hits.hits: 0 } + - match: { hits.total: 0 } + - is_false: aggregations + --- "Search with Metric": From fdff8f3db0093fa15cfa161f7dec80b715a48a43 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Thu, 23 Aug 2018 16:46:47 -0400 Subject: [PATCH 2/3] Do NOT allow termvectors on nested fields (#32728) Requesting _termvectors on a nested field or any sub-fields of a nested field returns empty results. Closes #21625 --- docs/reference/docs/termvectors.asciidoc | 4 ++ .../test/termvectors/50_nested.yml | 49 +++++++++++++++++++ .../index/termvectors/TermVectorsService.java | 17 +++++-- 3 files changed, 67 insertions(+), 3 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/termvectors/50_nested.yml diff --git a/docs/reference/docs/termvectors.asciidoc b/docs/reference/docs/termvectors.asciidoc index 3cd21b21df4..0e6078ad7b2 100644 --- a/docs/reference/docs/termvectors.asciidoc +++ b/docs/reference/docs/termvectors.asciidoc @@ -30,6 +30,10 @@ in similar way to the <> [WARNING] Note that the usage of `/_termvector` is deprecated in 2.0, and replaced by `/_termvectors`. +[WARNING] +Term Vectors API doesn't work on nested fields. `/_termvectors` on a nested +field and any sub-fields of a nested field returns empty results. + [float] === Return values diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/termvectors/50_nested.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/termvectors/50_nested.yml new file mode 100644 index 00000000000..a10fc7b504b --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/termvectors/50_nested.yml @@ -0,0 +1,49 @@ +setup: + - do: + indices.create: + index: testidx + body: + mappings: + _doc: + properties: + nested1: + type : nested + properties: + nested1-text: + type: text + object1: + properties: + object1-text: + type: text + object1-nested1: + type: nested + properties: + object1-nested1-text: + type: text + - do: + index: + index: testidx + type: _doc + id: 1 + body: + "nested1" : [{ "nested1-text": "text1" }] + "object1" : [{ "object1-text": "text2" }, "object1-nested1" : [{"object1-nested1-text" : "text3"}]] + + - do: + indices.refresh: {} + +--- +"Termvectors on nested fields should return empty results": + + - do: + termvectors: + index: testidx + type: _doc + id: 1 + fields: ["nested1", "nested1.nested1-text", "object1.object1-nested1", "object1.object1-nested1.object1-nested1-text", "object1.object1-text"] + + - is_false: term_vectors.nested1 + - is_false: term_vectors.nested1\.nested1-text # escaping as the field name contains dot + - is_false: term_vectors.object1\.object1-nested1 + - is_false: term_vectors.object1\.object1-nested1\.object1-nested1-text + - is_true: term_vectors.object1\.object1-text diff --git a/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java b/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java index bc77626b942..43f1a278f54 100644 --- a/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java +++ b/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java @@ -45,6 +45,7 @@ import org.elasticsearch.index.mapper.DocumentMapperForType; import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.index.mapper.ObjectMapper; import org.elasticsearch.index.mapper.ParseContext; import org.elasticsearch.index.mapper.ParsedDocument; import org.elasticsearch.index.mapper.SourceFieldMapper; @@ -160,7 +161,7 @@ public class TermVectorsService { request.selectedFields(fieldNames.toArray(Strings.EMPTY_ARRAY)); } - private static boolean isValidField(MappedFieldType fieldType) { + private static boolean isValidField(MappedFieldType fieldType, IndexShard indexShard) { // must be a string if (fieldType instanceof StringFieldType == false) { return false; @@ -169,6 +170,16 @@ public class TermVectorsService { if (fieldType.indexOptions() == IndexOptions.NONE) { return false; } + // and must not be under nested field + int dotIndex = fieldType.name().indexOf('.'); + while (dotIndex > -1) { + String parentField = fieldType.name().substring(0, dotIndex); + ObjectMapper mapper = indexShard.mapperService().getObjectMapper(parentField); + if (mapper != null && mapper.nested().isNested()) { + return false; + } + dotIndex = fieldType.name().indexOf('.', dotIndex + 1); + } return true; } @@ -177,7 +188,7 @@ public class TermVectorsService { Set validFields = new HashSet<>(); for (String field : selectedFields) { MappedFieldType fieldType = indexShard.mapperService().fullName(field); - if (!isValidField(fieldType)) { + if (isValidField(fieldType, indexShard) == false) { continue; } // already retrieved, only if the analyzer hasn't been overridden at the field @@ -284,7 +295,7 @@ public class TermVectorsService { Collection documentFields = new HashSet<>(); for (IndexableField field : doc.getFields()) { MappedFieldType fieldType = indexShard.mapperService().fullName(field.name()); - if (!isValidField(fieldType)) { + if (isValidField(fieldType, indexShard) == false) { continue; } if (request.selectedFields() != null && !request.selectedFields().contains(field.name())) { From 8f16696fe12bd406327b0f715f513a33c642ec37 Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Thu, 23 Aug 2018 15:45:25 -0500 Subject: [PATCH 3/3] Add versions 5.6.12 and 6.4.1 --- server/src/main/java/org/elasticsearch/Version.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/Version.java b/server/src/main/java/org/elasticsearch/Version.java index a815a9711d0..1afe88f8d43 100644 --- a/server/src/main/java/org/elasticsearch/Version.java +++ b/server/src/main/java/org/elasticsearch/Version.java @@ -122,6 +122,8 @@ public class Version implements Comparable, ToXContentFragment { public static final Version V_5_6_10 = new Version(V_5_6_10_ID, org.apache.lucene.util.Version.LUCENE_6_6_1); public static final int V_5_6_11_ID = 5061199; public static final Version V_5_6_11 = new Version(V_5_6_11_ID, org.apache.lucene.util.Version.LUCENE_6_6_1); + public static final int V_5_6_12_ID = 5061299; + public static final Version V_5_6_12 = new Version(V_5_6_12_ID, org.apache.lucene.util.Version.LUCENE_6_6_1); public static final int V_6_0_0_alpha1_ID = 6000001; public static final Version V_6_0_0_alpha1 = new Version(V_6_0_0_alpha1_ID, org.apache.lucene.util.Version.LUCENE_7_0_0); @@ -174,10 +176,10 @@ public class Version implements Comparable, ToXContentFragment { public static final Version V_6_3_1 = new Version(V_6_3_1_ID, org.apache.lucene.util.Version.LUCENE_7_3_1); public static final int V_6_3_2_ID = 6030299; public static final Version V_6_3_2 = new Version(V_6_3_2_ID, org.apache.lucene.util.Version.LUCENE_7_3_1); - public static final int V_6_3_3_ID = 6030399; - public static final Version V_6_3_3 = new Version(V_6_3_3_ID, org.apache.lucene.util.Version.LUCENE_7_3_1); public static final int V_6_4_0_ID = 6040099; public static final Version V_6_4_0 = new Version(V_6_4_0_ID, org.apache.lucene.util.Version.LUCENE_7_4_0); + public static final int V_6_4_1_ID = 6040199; + public static final Version V_6_4_1 = new Version(V_6_4_1_ID, org.apache.lucene.util.Version.LUCENE_7_4_0); public static final int V_6_5_0_ID = 6050099; public static final Version V_6_5_0 = new Version(V_6_5_0_ID, org.apache.lucene.util.Version.LUCENE_7_5_0); public static final int V_7_0_0_alpha1_ID = 7000001; @@ -200,10 +202,10 @@ public class Version implements Comparable, ToXContentFragment { return V_7_0_0_alpha1; case V_6_5_0_ID: return V_6_5_0; + case V_6_4_1_ID: + return V_6_4_1; case V_6_4_0_ID: return V_6_4_0; - case V_6_3_3_ID: - return V_6_3_3; case V_6_3_2_ID: return V_6_3_2; case V_6_3_1_ID: @@ -246,6 +248,8 @@ public class Version implements Comparable, ToXContentFragment { return V_6_0_0_alpha2; case V_6_0_0_alpha1_ID: return V_6_0_0_alpha1; + case V_5_6_12_ID: + return V_5_6_12; case V_5_6_11_ID: return V_5_6_11; case V_5_6_10_ID: