Skip to content

Commit a86c88c

Browse files
committed
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 elastic#52480
1 parent ab48256 commit a86c88c

File tree

3 files changed

+126
-33
lines changed

3 files changed

+126
-33
lines changed

rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,3 +850,83 @@ setup:
850850
- length: { aggregations.test.buckets: 1 }
851851
- match: { aggregations.test.buckets.0.key.date: "2017-10-21T00:00:00.000-02:00" }
852852
- match: { aggregations.test.buckets.0.doc_count: 2 }
853+
854+
---
855+
"Terms source from sorted":
856+
- do:
857+
indices.create:
858+
index: sorted_test
859+
body:
860+
settings:
861+
sort.field: keyword
862+
mappings:
863+
properties:
864+
keyword:
865+
type: keyword
866+
long:
867+
type: long
868+
869+
870+
- do:
871+
index:
872+
index: sorted_test
873+
id: 2
874+
refresh: true
875+
body: { "keyword": "foo", "long": 1 }
876+
877+
- do:
878+
search:
879+
index: sorted_test
880+
body:
881+
aggregations:
882+
test:
883+
composite:
884+
sources:
885+
- keyword:
886+
terms:
887+
field: keyword
888+
889+
- match: {hits.total.value: 1}
890+
- length: { aggregations.test.buckets: 1 }
891+
- match: { aggregations.test.buckets.0.key.keyword: "foo" }
892+
- match: { aggregations.test.buckets.0.doc_count: 1 }
893+
894+
---
895+
"Terms source from part of sorted":
896+
- do:
897+
indices.create:
898+
index: sorted_test
899+
body:
900+
settings:
901+
sort.field: [keyword, long]
902+
mappings:
903+
properties:
904+
keyword:
905+
type: keyword
906+
long:
907+
type: long
908+
909+
910+
- do:
911+
index:
912+
index: sorted_test
913+
id: 2
914+
refresh: true
915+
body: { "keyword": "foo", "long": 1 }
916+
917+
- do:
918+
search:
919+
index: sorted_test
920+
body:
921+
aggregations:
922+
test:
923+
composite:
924+
sources:
925+
- keyword:
926+
terms:
927+
field: keyword
928+
929+
- match: {hits.total.value: 1}
930+
- length: { aggregations.test.buckets: 1 }
931+
- match: { aggregations.test.buckets.0.key.keyword: "foo" }
932+
- match: { aggregations.test.buckets.0.doc_count: 1 }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,8 @@ private Sort buildIndexSortPrefix(LeafReaderContext context) throws IOException
202202
return null;
203203
}
204204
List<SortField> sortFields = new ArrayList<>();
205-
for (int i = 0; i < indexSort.getSort().length; i++) {
205+
int max = Math.min(indexSort.getSort().length, sourceConfigs.length);
206+
for (int i = 0; i < max; i++) {
206207
CompositeValuesSourceConfig sourceConfig = sourceConfigs[i];
207208
SingleDimensionValuesSource<?> source = sources[i];
208209
SortField indexSortField = indexSort.getSort()[i];

server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java

Lines changed: 44 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2077,40 +2077,52 @@ private static long asLong(String dateTime) {
20772077

20782078
private static Sort buildIndexSort(List<CompositeValuesSourceBuilder<?>> sources, Map<String, MappedFieldType> fieldTypes) {
20792079
List<SortField> sortFields = new ArrayList<>();
2080+
Map<String, MappedFieldType> remainingFieldTypes = new HashMap<>(fieldTypes);
20802081
for (CompositeValuesSourceBuilder<?> source : sources) {
2081-
MappedFieldType type = fieldTypes.get(source.field());
2082-
if (type instanceof KeywordFieldMapper.KeywordFieldType) {
2083-
sortFields.add(new SortedSetSortField(type.name(), false));
2084-
} else if (type instanceof DateFieldMapper.DateFieldType) {
2085-
sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.LONG, false));
2086-
} else if (type instanceof NumberFieldMapper.NumberFieldType) {
2087-
boolean comp = false;
2088-
switch (type.typeName()) {
2089-
case "byte":
2090-
case "short":
2091-
case "integer":
2092-
comp = true;
2093-
sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.INT, false));
2094-
break;
2095-
2096-
case "long":
2097-
sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.LONG, false));
2098-
break;
2099-
2100-
case "float":
2101-
case "double":
2102-
comp = true;
2103-
sortFields.add(new SortedNumericSortField(type.name(), SortField.Type.DOUBLE, false));
2104-
break;
2105-
2106-
default:
2107-
break;
2108-
}
2109-
if (comp == false) {
2110-
break;
2111-
}
2082+
MappedFieldType type = fieldTypes.remove(source.field());
2083+
remainingFieldTypes.remove(source.field());
2084+
SortField sortField = sortFieldFrom(type);
2085+
if (sortField == null) {
2086+
break;
2087+
}
2088+
sortFields.add(sortField);
2089+
if (sortField instanceof SortedNumericSortField && ((SortedNumericSortField) sortField).getType() == SortField.Type.LONG) {
2090+
break;
2091+
}
2092+
}
2093+
while (remainingFieldTypes.size() > 0 && randomBoolean()) {
2094+
// Add extra unused sorts
2095+
List<String> fields = new ArrayList<>(remainingFieldTypes.keySet());
2096+
Collections.sort(fields);
2097+
String field = fields.get(between(0, fields.size() - 1));
2098+
SortField sortField = sortFieldFrom(remainingFieldTypes.remove(field));
2099+
if (sortField != null) {
2100+
sortFields.add(sortField);
2101+
}
2102+
}
2103+
return sortFields.size() > 0 ? new Sort(sortFields.toArray(SortField[]::new)) : null;
2104+
}
2105+
2106+
private static SortField sortFieldFrom(MappedFieldType type) {
2107+
if (type instanceof KeywordFieldMapper.KeywordFieldType) {
2108+
return new SortedSetSortField(type.name(), false);
2109+
} else if (type instanceof DateFieldMapper.DateFieldType) {
2110+
return new SortedNumericSortField(type.name(), SortField.Type.LONG, false);
2111+
} else if (type instanceof NumberFieldMapper.NumberFieldType) {
2112+
switch (type.typeName()) {
2113+
case "byte":
2114+
case "short":
2115+
case "integer":
2116+
return new SortedNumericSortField(type.name(), SortField.Type.INT, false);
2117+
case "long":
2118+
return new SortedNumericSortField(type.name(), SortField.Type.LONG, false);
2119+
case "float":
2120+
case "double":
2121+
return new SortedNumericSortField(type.name(), SortField.Type.DOUBLE, false);
2122+
default:
2123+
return null;
21122124
}
21132125
}
2114-
return sortFields.size() > 0 ? new Sort(sortFields.toArray(new SortField[0])) : null;
2126+
return null;
21152127
}
21162128
}

0 commit comments

Comments
 (0)