Skip to content

Commit 393279a

Browse files
committed
fix: Sort Processor does not have proper behavior with targetField
to specify a `targetField`. This results in some interesting behavior that was missed in the review. This processor sorts in-place, so there is a side-effect in both the original field and the target field. Another bug was that the targetField was not being set if the list being sorted was fewer than two elements. The new behavior works like this: If targetField and fieldName are not the same, we copy the list.
1 parent 68deda6 commit 393279a

File tree

2 files changed

+45
-10
lines changed

2 files changed

+45
-10
lines changed

modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/SortProcessor.java

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.elasticsearch.ingest.IngestDocument;
2525
import org.elasticsearch.ingest.Processor;
2626

27+
import java.util.ArrayList;
2728
import java.util.Collections;
2829
import java.util.List;
2930
import java.util.Map;
@@ -99,17 +100,15 @@ public void execute(IngestDocument document) {
99100
throw new IllegalArgumentException("field [" + field + "] is null, cannot sort.");
100101
}
101102

102-
if (list.size() <= 1) {
103-
return;
104-
}
103+
List<? extends Comparable> copy = new ArrayList<>(list);
105104

106105
if (order.equals(SortOrder.ASCENDING)) {
107-
Collections.sort(list);
106+
Collections.sort(copy);
108107
} else {
109-
Collections.sort(list, Collections.reverseOrder());
108+
Collections.sort(copy, Collections.reverseOrder());
110109
}
111110

112-
document.setFieldValue(targetField, list);
111+
document.setFieldValue(targetField, copy);
113112
}
114113

115114
@Override

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/SortProcessorTests.java

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ public void testSortNullValue() throws Exception {
275275
}
276276
}
277277

278-
public void testSortWithTargetField() throws Exception {
278+
public void testDescendingSortWithTargetField() throws Exception {
279279
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
280280
int numItems = randomIntBetween(1, 10);
281281
List<String> fieldValue = new ArrayList<>(numItems);
@@ -285,18 +285,54 @@ public void testSortWithTargetField() throws Exception {
285285
fieldValue.add(value);
286286
expectedResult.add(value);
287287
}
288+
289+
Collections.sort(expectedResult, Collections.reverseOrder());
290+
291+
String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue);
292+
String targetFieldName = RandomDocumentPicks.randomFieldName(random());
293+
Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName,
294+
SortOrder.DESCENDING, targetFieldName);
295+
processor.execute(ingestDocument);
296+
assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult);
297+
}
298+
299+
public void testAscendingSortWithTargetField() throws Exception {
300+
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
301+
int numItems = randomIntBetween(1, 10);
302+
List<String> fieldValue = new ArrayList<>(numItems);
303+
List<String> expectedResult = new ArrayList<>(numItems);
304+
for (int j = 0; j < numItems; j++) {
305+
String value = randomAlphaOfLengthBetween(1, 10);
306+
fieldValue.add(value);
307+
expectedResult.add(value);
308+
}
309+
310+
Collections.sort(expectedResult);
311+
312+
String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue);
313+
String targetFieldName = RandomDocumentPicks.randomFieldName(random());
314+
Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName,
315+
SortOrder.ASCENDING, targetFieldName);
316+
processor.execute(ingestDocument);
317+
assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult);
318+
}
319+
320+
public void testSortWithTargetFieldLeavesOriginalUntouched() throws Exception {
321+
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
322+
List<Integer> fieldValue = Arrays.asList(1, 5, 4);
323+
List<Integer> expectedResult = new ArrayList<>(fieldValue);
288324
Collections.sort(expectedResult);
289325

290326
SortOrder order = randomBoolean() ? SortOrder.ASCENDING : SortOrder.DESCENDING;
291327
if (order.equals(SortOrder.DESCENDING)) {
292328
Collections.reverse(expectedResult);
293329
}
294330

295-
String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue);
296-
String targetFieldName = RandomDocumentPicks.randomFieldName(random());
331+
String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, new ArrayList<>(fieldValue));
332+
String targetFieldName = fieldName + "foo";
297333
Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName, order, targetFieldName);
298334
processor.execute(ingestDocument);
299335
assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult);
336+
assertEquals(ingestDocument.getFieldValue(fieldName, List.class), fieldValue);
300337
}
301-
302338
}

0 commit comments

Comments
 (0)