From a86c88cfb465e8a28693828447215b458a5eb79b Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 9 Mar 2020 10:56:08 -0400 Subject: [PATCH 1/6] Fix composite agg sort bug 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 | 80 +++++++++++++++++++ .../bucket/composite/CompositeAggregator.java | 3 +- .../composite/CompositeAggregatorTests.java | 76 ++++++++++-------- 3 files changed, 126 insertions(+), 33 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 49afbb8afc714..626f2e4bf3f91 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 @@ -850,3 +850,83 @@ 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": + - 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..7aebe26a04961 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 max = Math.min(indexSort.getSort().length, sourceConfigs.length); + for (int i = 0; i < max; 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..dbea1d7f2729e 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(SortField[]::new)) : 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 sortFields.size() > 0 ? new Sort(sortFields.toArray(new SortField[0])) : null; + return null; } } From 4cd354f4594062cac633de3cd810228ebcd0078a Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 9 Mar 2020 13:22:06 -0400 Subject: [PATCH 2/6] Randomly shorten the sort as well --- .../bucket/composite/CompositeAggregatorTests.java | 3 +++ 1 file changed, 3 insertions(+) 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 dbea1d7f2729e..8e195c8c1fa36 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 @@ -2089,6 +2089,9 @@ private static Sort buildIndexSort(List> sources if (sortField instanceof SortedNumericSortField && ((SortedNumericSortField) sortField).getType() == SortField.Type.LONG) { break; } + if (randomBoolean()) { + break; + } } while (remainingFieldTypes.size() > 0 && randomBoolean()) { // Add extra unused sorts From d47387bbbbf46af6ab0e02acd688587d8e1fb5a7 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 9 Mar 2020 13:24:14 -0400 Subject: [PATCH 3/6] Revert "Randomly shorten the sort as well" This reverts commit 4cd354f4594062cac633de3cd810228ebcd0078a. --- .../bucket/composite/CompositeAggregatorTests.java | 3 --- 1 file changed, 3 deletions(-) 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 8e195c8c1fa36..dbea1d7f2729e 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 @@ -2089,9 +2089,6 @@ private static Sort buildIndexSort(List> sources if (sortField instanceof SortedNumericSortField && ((SortedNumericSortField) sortField).getType() == SortField.Type.LONG) { break; } - if (randomBoolean()) { - break; - } } while (remainingFieldTypes.size() > 0 && randomBoolean()) { // Add extra unused sorts From 9b8dbe1e19fa40468f07813b2bd78244197cc78c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 9 Mar 2020 16:36:52 -0400 Subject: [PATCH 4/6] Rename --- .../rest-api-spec/test/search.aggregation/230_composite.yml | 4 ++++ .../aggregations/bucket/composite/CompositeAggregator.java | 4 ++-- 2 files changed, 6 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 626f2e4bf3f91..eea3d7792c7f6 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 @@ -893,6 +893,10 @@ 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. + - do: indices.create: index: sorted_test 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 7aebe26a04961..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,8 +202,8 @@ private Sort buildIndexSortPrefix(LeafReaderContext context) throws IOException return null; } List sortFields = new ArrayList<>(); - int max = Math.min(indexSort.getSort().length, sourceConfigs.length); - for (int i = 0; i < max; 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]; From 523708dcaea417fe0aa57f73cf5c2e5c09f656e9 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 9 Mar 2020 16:37:24 -0400 Subject: [PATCH 5/6] Skip? --- .../rest-api-spec/test/search.aggregation/230_composite.yml | 2 +- 1 file changed, 1 insertion(+), 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 eea3d7792c7f6..0153bb4f0dee0 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 @@ -894,7 +894,7 @@ setup: --- "Terms source from part of sorted": - skip: - version: " - 7.99.99" + version: " - 7.6.99" reason: fixed in 8.0.0. will be backported to 7.7.0. - do: From e3e352c7b8491bb2b25d3e3fb6f19ac5d5045ed0 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 9 Mar 2020 16:53:23 -0400 Subject: [PATCH 6/6] Updateskip --- .../rest-api-spec/test/search.aggregation/230_composite.yml | 2 +- 1 file changed, 1 insertion(+), 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 0153bb4f0dee0..eea3d7792c7f6 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 @@ -894,7 +894,7 @@ setup: --- "Terms source from part of sorted": - skip: - version: " - 7.6.99" + version: " - 7.99.99" reason: fixed in 8.0.0. will be backported to 7.7.0. - do: