From e454463790e79961db65b2afb32358f4a515618b Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 10 Mar 2020 09:23:44 -0400 Subject: [PATCH 1/3] Fix composite agg sort bug (#53296) When an composite aggregation is run against an index with a sort that *starts* with the "source" fields from the composite but has additional fields it'd blow up in while trying to decide if it could use the sort. This changes it to decide that it *can* use the sort. Closes #52480 --- .../test/search.aggregation/230_composite.yml | 84 +++++++++++++++++++ .../bucket/composite/CompositeAggregator.java | 3 +- .../composite/CompositeAggregatorTests.java | 74 +++++++++------- 3 files changed, 129 insertions(+), 32 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml index d2378084f3b52..8aa04dde5564a 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml @@ -885,3 +885,87 @@ setup: - length: { aggregations.test.buckets: 1 } - match: { aggregations.test.buckets.0.key.date: "2017-10-21T00:00:00.000-02:00" } - match: { aggregations.test.buckets.0.doc_count: 2 } + +--- +"Terms source from sorted": + - do: + indices.create: + index: sorted_test + body: + settings: + sort.field: keyword + mappings: + properties: + keyword: + type: keyword + long: + type: long + + + - do: + index: + index: sorted_test + id: 2 + refresh: true + body: { "keyword": "foo", "long": 1 } + + - do: + search: + index: sorted_test + body: + aggregations: + test: + composite: + sources: + - keyword: + terms: + field: keyword + + - match: {hits.total.value: 1} + - length: { aggregations.test.buckets: 1 } + - match: { aggregations.test.buckets.0.key.keyword: "foo" } + - match: { aggregations.test.buckets.0.doc_count: 1 } + +--- +"Terms source from part of sorted": + - skip: + version: " - 7.99.99" + reason: fixed in 8.0.0. will be backported to 7.7.0. + + - do: + indices.create: + index: sorted_test + body: + settings: + sort.field: [keyword, long] + mappings: + properties: + keyword: + type: keyword + long: + type: long + + + - do: + index: + index: sorted_test + id: 2 + refresh: true + body: { "keyword": "foo", "long": 1 } + + - do: + search: + index: sorted_test + body: + aggregations: + test: + composite: + sources: + - keyword: + terms: + field: keyword + + - match: {hits.total.value: 1} + - length: { aggregations.test.buckets: 1 } + - match: { aggregations.test.buckets.0.key.keyword: "foo" } + - match: { aggregations.test.buckets.0.doc_count: 1 } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java index 0e01615492939..b4c72b8529c0f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java @@ -202,7 +202,8 @@ private Sort buildIndexSortPrefix(LeafReaderContext context) throws IOException return null; } List sortFields = new ArrayList<>(); - for (int i = 0; i < indexSort.getSort().length; i++) { + int end = Math.min(indexSort.getSort().length, sourceConfigs.length); + for (int i = 0; i < end; i++) { CompositeValuesSourceConfig sourceConfig = sourceConfigs[i]; SingleDimensionValuesSource source = sources[i]; SortField indexSortField = indexSort.getSort()[i]; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java index 7685cd9067ddc..4481069ee8c22 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java @@ -2077,40 +2077,52 @@ private static long asLong(String dateTime) { private static Sort buildIndexSort(List> sources, Map fieldTypes) { List sortFields = new ArrayList<>(); + Map remainingFieldTypes = new HashMap<>(fieldTypes); for (CompositeValuesSourceBuilder source : sources) { - MappedFieldType type = fieldTypes.get(source.field()); - if (type instanceof KeywordFieldMapper.KeywordFieldType) { - sortFields.add(new SortedSetSortField(type.name(), false)); - } else if (type instanceof DateFieldMapper.DateFieldType) { - sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.LONG, false)); - } else if (type instanceof NumberFieldMapper.NumberFieldType) { - boolean comp = false; - switch (type.typeName()) { - case "byte": - case "short": - case "integer": - comp = true; - sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.INT, false)); - break; - - case "long": - sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.LONG, false)); - break; - - case "float": - case "double": - comp = true; - sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.DOUBLE, false)); - break; - - default: - break; - } - if (comp == false) { - break; - } + MappedFieldType type = fieldTypes.remove(source.field()); + remainingFieldTypes.remove(source.field()); + SortField sortField = sortFieldFrom(type); + if (sortField == null) { + break; + } + sortFields.add(sortField); + if (sortField instanceof SortedNumericSortField && ((SortedNumericSortField) sortField).getType() == SortField.Type.LONG) { + break; + } + } + while (remainingFieldTypes.size() > 0 && randomBoolean()) { + // Add extra unused sorts + List fields = new ArrayList<>(remainingFieldTypes.keySet()); + Collections.sort(fields); + String field = fields.get(between(0, fields.size() - 1)); + SortField sortField = sortFieldFrom(remainingFieldTypes.remove(field)); + if (sortField != null) { + sortFields.add(sortField); } } return sortFields.size() > 0 ? new Sort(sortFields.toArray(new SortField[0])) : null; } + + private static SortField sortFieldFrom(MappedFieldType type) { + if (type instanceof KeywordFieldMapper.KeywordFieldType) { + return new SortedSetSortField(type.name(), false); + } else if (type instanceof DateFieldMapper.DateFieldType) { + return new SortedNumericSortField(type.name(), SortField.Type.LONG, false); + } else if (type instanceof NumberFieldMapper.NumberFieldType) { + switch (type.typeName()) { + case "byte": + case "short": + case "integer": + return new SortedNumericSortField(type.name(), SortField.Type.INT, false); + case "long": + return new SortedNumericSortField(type.name(), SortField.Type.LONG, false); + case "float": + case "double": + return new SortedNumericSortField(type.name(), SortField.Type.DOUBLE, false); + default: + return null; + } + } + return null; + } } From 66a32402761f61168f97fc9e34df611b8320915f Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 10 Mar 2020 09:46:03 -0400 Subject: [PATCH 2/3] Update skip --- .../rest-api-spec/test/search.aggregation/230_composite.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml index 8aa04dde5564a..4a639d5a6468a 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml @@ -929,8 +929,8 @@ setup: --- "Terms source from part of sorted": - skip: - version: " - 7.99.99" - reason: fixed in 8.0.0. will be backported to 7.7.0. + version: " - 7.6.99" + reason: fixed in 7.7.0 - do: indices.create: From 984b8c1decf4cd75060605960c37e579fdd50c39 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 10 Mar 2020 10:17:19 -0400 Subject: [PATCH 3/3] BWC!!! --- .../rest-api-spec/test/search.aggregation/230_composite.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml index 4a639d5a6468a..29780b3ae91e5 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml @@ -912,6 +912,7 @@ setup: - do: search: index: sorted_test + rest_total_hits_as_int: true body: aggregations: test: @@ -921,7 +922,7 @@ setup: terms: field: keyword - - match: {hits.total.value: 1} + - match: {hits.total: 1} - length: { aggregations.test.buckets: 1 } - match: { aggregations.test.buckets.0.key.keyword: "foo" } - match: { aggregations.test.buckets.0.doc_count: 1 }