honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: Sort Processor does not have proper behavior with targetField (#25237) 

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.
This commit is contained in:
Tal Levy 2017-06-15 05:28:54 -07:00 committed by GitHub
parent 1b90c46a53
commit 2cd771a230
2 changed files with 45 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -24,6 +24,7 @@ import org.elasticsearch.ingest.ConfigurationUtils;
import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.IngestDocument;
import org.elasticsearch.ingest.Processor; import org.elasticsearch.ingest.Processor;
import java.util.ArrayList;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
@ -99,17 +100,15 @@ public final class SortProcessor extends AbstractProcessor {
throw new IllegalArgumentException("field [" + field + "] is null, cannot sort."); throw new IllegalArgumentException("field [" + field + "] is null, cannot sort.");
} }
if (list.size() <= 1) { List<? extends Comparable> copy = new ArrayList<>(list);
return;
}
if (order.equals(SortOrder.ASCENDING)) { if (order.equals(SortOrder.ASCENDING)) {
Collections.sort(list); Collections.sort(copy);
} else { } else {
Collections.sort(list, Collections.reverseOrder()); Collections.sort(copy, Collections.reverseOrder());
} }
document.setFieldValue(targetField, list); document.setFieldValue(targetField, copy);
} }
@Override @Override

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

@ -275,7 +275,7 @@ public class SortProcessorTests extends ESTestCase {
} }
} }
public void testSortWithTargetField() throws Exception { public void testDescendingSortWithTargetField() throws Exception {
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random()); IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
int numItems = randomIntBetween(1, 10); int numItems = randomIntBetween(1, 10);
List<String> fieldValue = new ArrayList<>(numItems); List<String> fieldValue = new ArrayList<>(numItems);
@ -285,6 +285,42 @@ public class SortProcessorTests extends ESTestCase {
fieldValue.add(value); fieldValue.add(value);
expectedResult.add(value); expectedResult.add(value);
} }
Collections.sort(expectedResult, Collections.reverseOrder());
String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue);
String targetFieldName = RandomDocumentPicks.randomFieldName(random());
Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName,
SortOrder.DESCENDING, targetFieldName);
processor.execute(ingestDocument);
assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult);
}
public void testAscendingSortWithTargetField() throws Exception {
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
int numItems = randomIntBetween(1, 10);
List<String> fieldValue = new ArrayList<>(numItems);
List<String> expectedResult = new ArrayList<>(numItems);
for (int j = 0; j < numItems; j++) {
String value = randomAlphaOfLengthBetween(1, 10);
fieldValue.add(value);
expectedResult.add(value);
}
Collections.sort(expectedResult);
String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue);
String targetFieldName = RandomDocumentPicks.randomFieldName(random());
Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName,
SortOrder.ASCENDING, targetFieldName);
processor.execute(ingestDocument);
assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult);
}
public void testSortWithTargetFieldLeavesOriginalUntouched() throws Exception {
IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random());
List<Integer> fieldValue = Arrays.asList(1, 5, 4);
List<Integer> expectedResult = new ArrayList<>(fieldValue);
Collections.sort(expectedResult); Collections.sort(expectedResult);
SortOrder order = randomBoolean() ? SortOrder.ASCENDING : SortOrder.DESCENDING; SortOrder order = randomBoolean() ? SortOrder.ASCENDING : SortOrder.DESCENDING;
@ -292,11 +328,11 @@ public class SortProcessorTests extends ESTestCase {
Collections.reverse(expectedResult); Collections.reverse(expectedResult);
} }
String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, fieldValue); String fieldName = RandomDocumentPicks.addRandomField(random(), ingestDocument, new ArrayList<>(fieldValue));
String targetFieldName = RandomDocumentPicks.randomFieldName(random()); String targetFieldName = fieldName + "foo";
Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName, order, targetFieldName); Processor processor = new SortProcessor(randomAlphaOfLength(10), fieldName, order, targetFieldName);
processor.execute(ingestDocument); processor.execute(ingestDocument);
assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult); assertEquals(ingestDocument.getFieldValue(targetFieldName, List.class), expectedResult);
assertEquals(ingestDocument.getFieldValue(fieldName, List.class), fieldValue);
} }
} }