From 0a18e615e0c434224ea366c8cbc6353357728203 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 11 Nov 2025 10:43:53 +0000 Subject: [PATCH 1/2] Split index-time and search-time sort definitions At search time, we can now always use our custom comparators that allow pruning on secondary sorts. At index time, we need to always use Lucene SortFields, which is fine as pruning isn't used at this point in any case. --- .../elasticsearch/index/IndexSortConfig.java | 4 +- .../index/fielddata/IndexFieldData.java | 13 +++- .../fielddata/IndexNumericFieldData.java | 13 +++- .../HalfFloatValuesComparatorSource.java | 6 ++ .../XUpdateableDocIdSetIterator.java | 4 + .../search/sort/FieldSortBuilder.java | 4 +- .../index/mapper/DateFieldMapperTests.java | 11 ++- .../index/mapper/NumberFieldTypeTests.java | 2 +- .../search/sort/FieldSortBuilderTests.java | 74 ++++++------------- .../index/mapper/MapperTestCase.java | 30 +++++--- 10 files changed, 91 insertions(+), 70 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/IndexSortConfig.java b/server/src/main/java/org/elasticsearch/index/IndexSortConfig.java index fd445d470837c..39475a1bf89f8 100644 --- a/server/src/main/java/org/elasticsearch/index/IndexSortConfig.java +++ b/server/src/main/java/org/elasticsearch/index/IndexSortConfig.java @@ -375,7 +375,7 @@ public Sort buildIndexSort( if (fieldData == null) { throw new IllegalArgumentException("docvalues not found for index sort field:[" + sortSpec.field + "]"); } - sortFields[i] = fieldData.sortField(this.indexCreatedVersion, sortSpec.missingValue, mode, null, reverse); + sortFields[i] = fieldData.indexSort(this.indexCreatedVersion, sortSpec.missingValue, mode, reverse); validateIndexSortField(sortFields[i]); } return new Sort(sortFields); @@ -430,6 +430,8 @@ public static SortField.Type getSortFieldType(SortField sortField) { return SortField.Type.STRING; } else if (sortField instanceof SortedNumericSortField) { return ((SortedNumericSortField) sortField).getNumericType(); + } else if (sortField.getComparatorSource() instanceof IndexFieldData.XFieldComparatorSource fcs) { + return fcs.sortType(); } else { return sortField.getType(); } diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/IndexFieldData.java b/server/src/main/java/org/elasticsearch/index/fielddata/IndexFieldData.java index 5f6cc93063d87..8fad08a9bd8f8 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/IndexFieldData.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/IndexFieldData.java @@ -69,13 +69,20 @@ public interface IndexFieldData { * Returns the {@link SortField} to use for sorting depending on the version of the index. */ default SortField sortField( + boolean indexSort, IndexVersion indexCreatedVersion, @Nullable Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse ) { - return sortField(missingValue, sortMode, nested, reverse); + return indexSort + ? indexSort(indexCreatedVersion, missingValue, sortMode, reverse) + : sortField(missingValue, sortMode, nested, reverse); + } + + default SortField indexSort(IndexVersion indexCreatedVersion, @Nullable Object missingValue, MultiValueMode sortMode, boolean reverse) { + return sortField(missingValue, sortMode, null, reverse); } /** @@ -231,6 +238,10 @@ public Object missingObject(Object missingValue, boolean reversed) { public abstract SortField.Type reducedType(); + public SortField.Type sortType() { + return reducedType(); + } + /** * Return a missing value that is understandable by {@link SortField#setMissingValue(Object)}. * Most implementations return null because they already replace the value at the fielddata level. diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java b/server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java index 53a8bcf2776aa..0294ae24199c3 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java @@ -86,6 +86,7 @@ public final ValuesSourceType getValuesSourceType() { * match the field's numericType. */ public final SortField sortField( + boolean indexSort, NumericType targetNumericType, Object missingValue, MultiValueMode sortMode, @@ -104,7 +105,7 @@ public final SortField sortField( boolean requiresCustomComparator = nested != null || (sortMode != MultiValueMode.MAX && sortMode != MultiValueMode.MIN) || targetNumericType != getNumericType(); - if (sortRequiresCustomComparator() || requiresCustomComparator) { + if (indexSort == false || sortRequiresCustomComparator()) { SortField sortField = new SortField(getFieldName(), source, reverse); sortField.setOptimizeSortWithPoints(requiresCustomComparator == false && canUseOptimizedSort(indexType())); return sortField; @@ -138,18 +139,24 @@ private static boolean canUseOptimizedSort(IndexType indexType) { @Override public final SortField sortField(Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse) { - return sortField(getNumericType(), missingValue, sortMode, nested, reverse); + return sortField(false, getNumericType(), missingValue, sortMode, nested, reverse); + } + + @Override + public SortField indexSort(IndexVersion indexCreatedVersion, Object missingValue, MultiValueMode sortMode, boolean reverse) { + return sortField(true, indexCreatedVersion, missingValue, sortMode, null, reverse); } @Override public SortField sortField( + boolean indexSort, IndexVersion indexCreatedVersion, Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse ) { - SortField sortField = sortField(missingValue, sortMode, nested, reverse); + SortField sortField = sortField(indexSort, getNumericType(), missingValue, sortMode, nested, reverse); if (getNumericType() == NumericType.DATE_NANOSECONDS && indexCreatedVersion.before(IndexVersions.V_7_14_0) && missingValue.equals("_last") diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java index 6dd060bd86f45..15b01ab6ed8ba 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java @@ -13,6 +13,7 @@ import org.apache.lucene.search.FieldComparator; import org.apache.lucene.search.LeafFieldComparator; import org.apache.lucene.search.Pruning; +import org.apache.lucene.search.SortField; import org.elasticsearch.core.Nullable; import org.elasticsearch.index.fielddata.DenseDoubleValues; import org.elasticsearch.index.fielddata.IndexNumericFieldData; @@ -33,6 +34,11 @@ public HalfFloatValuesComparatorSource( super(indexFieldData, missingValue, sortMode, nested); } + @Override + public SortField.Type sortType() { + return SortField.Type.CUSTOM; + } + @Override public FieldComparator newComparator(String fieldname, int numHits, Pruning enableSkipping, boolean reversed) { assert indexFieldData == null || fieldname.equals(indexFieldData.getFieldName()); diff --git a/server/src/main/java/org/elasticsearch/lucene/comparators/XUpdateableDocIdSetIterator.java b/server/src/main/java/org/elasticsearch/lucene/comparators/XUpdateableDocIdSetIterator.java index d7ce698d0c7af..7166737c01056 100644 --- a/server/src/main/java/org/elasticsearch/lucene/comparators/XUpdateableDocIdSetIterator.java +++ b/server/src/main/java/org/elasticsearch/lucene/comparators/XUpdateableDocIdSetIterator.java @@ -69,4 +69,8 @@ public int docIDRunEnd() throws IOException { return super.docIDRunEnd(); } } + + public DocIdSetIterator getDelegate() { + return in; + } } diff --git a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java index 8758aa6dfbe1a..1a4e2e4eea0cf 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java @@ -355,10 +355,10 @@ public SortFieldAndFormat build(SearchExecutionContext context) throws IOExcepti } IndexNumericFieldData numericFieldData = (IndexNumericFieldData) fieldData; NumericType resolvedType = resolveNumericType(numericType); - field = numericFieldData.sortField(resolvedType, missing, localSortMode(), nested, reverse); + field = numericFieldData.sortField(false, resolvedType, missing, localSortMode(), nested, reverse); isNanosecond = resolvedType == NumericType.DATE_NANOSECONDS; } else { - field = fieldData.sortField(context.indexVersionCreated(), missing, localSortMode(), nested, reverse); + field = fieldData.sortField(false, context.indexVersionCreated(), missing, localSortMode(), nested, reverse); if (fieldData instanceof IndexNumericFieldData) { isNanosecond = ((IndexNumericFieldData) fieldData).getNumericType() == NumericType.DATE_NANOSECONDS; } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java index 5a117765424ab..b05226e8975d5 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java @@ -16,6 +16,7 @@ import org.apache.lucene.index.IndexWriterConfig; import org.apache.lucene.index.IndexableField; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.DocIdSetIterator; import org.apache.lucene.search.Sort; import org.apache.lucene.search.SortField; import org.apache.lucene.store.Directory; @@ -34,6 +35,7 @@ import org.elasticsearch.index.codec.tsdb.es819.ES819TSDBDocValuesFormat; import org.elasticsearch.index.mapper.DateFieldMapper.DateFieldType; import org.elasticsearch.index.mapper.blockloader.docvalues.LongsBlockLoader; +import org.elasticsearch.lucene.comparators.XUpdateableDocIdSetIterator; import org.elasticsearch.script.DateFieldScript; import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.DocValueFormat; @@ -48,6 +50,7 @@ import java.time.ZonedDateTime; import java.util.Comparator; import java.util.List; +import java.util.function.Consumer; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -915,6 +918,12 @@ public void testSingletonLongBulkBlockReadingManyValues() throws Exception { } } + private static final Consumer checkClass = disi -> { + assertThat(disi, instanceOf(XUpdateableDocIdSetIterator.class)); + XUpdateableDocIdSetIterator iterator = (XUpdateableDocIdSetIterator) disi; + assertThat(iterator.getDelegate().getClass().getName(), containsString("SecondarySortIterator")); + }; + @Override protected List getSortShortcutSupport() { return List.of( @@ -925,7 +934,7 @@ protected List getSortShortcutSupport() { b -> b.field("type", "date").field("ignore_malformed", false), b -> b.startObject("host.name").field("type", "keyword").endObject(), b -> b.field("@timestamp", "2025-10-30T00:00:00").field("host.name", "foo"), - true + checkClass ), new SortShortcutSupport(b -> b.field("type", "date"), b -> b.field("field", "2025-10-30T00:00:00"), true), new SortShortcutSupport(b -> b.field("type", "date_nanos"), b -> b.field("field", "2025-10-30T00:00:00"), true), diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java index 735a61a25eabe..c40e720360c2a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java @@ -723,7 +723,7 @@ public void doTestIndexSortRangeQueries(NumberType type, Supplier valueS NumberFieldType fieldType = new NumberFieldType("field", type); IndexNumericFieldData fielddata = (IndexNumericFieldData) fieldType.fielddataBuilder(FieldDataContext.noRuntimeFields("test")) .build(null, null); - SortField sortField = fielddata.sortField(null, MultiValueMode.MIN, null, randomBoolean()); + SortField sortField = fielddata.indexSort(IndexVersion.current(), null, MultiValueMode.MIN, randomBoolean()); IndexWriterConfig writerConfig = new IndexWriterConfig(); writerConfig.setIndexSort(new Sort(sortField)); diff --git a/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java b/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java index f79d02b8c29af..1f2a369f17e21 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -22,8 +22,6 @@ import org.apache.lucene.index.Term; import org.apache.lucene.sandbox.document.HalfFloatPoint; import org.apache.lucene.search.SortField; -import org.apache.lucene.search.SortedNumericSelector; -import org.apache.lucene.search.SortedNumericSortField; import org.apache.lucene.search.SortedSetSelector; import org.apache.lucene.search.SortedSetSortField; import org.apache.lucene.search.TermQuery; @@ -151,36 +149,26 @@ public void testBuildSortFieldMissingValue() throws IOException { SearchExecutionContext searchExecutionContext = createMockSearchExecutionContext(); FieldSortBuilder fieldSortBuilder = new FieldSortBuilder("value").missing("_first"); SortField sortField = fieldSortBuilder.build(searchExecutionContext).field(); - SortedNumericSortField expectedSortField = new SortedNumericSortField("value", SortField.Type.DOUBLE); - expectedSortField.setMissingValue(Double.NEGATIVE_INFINITY); - assertEquals(expectedSortField, sortField); + assertMissingValue(sortField, Double.NEGATIVE_INFINITY); fieldSortBuilder = new FieldSortBuilder("value").missing("_last"); sortField = fieldSortBuilder.build(searchExecutionContext).field(); - expectedSortField = new SortedNumericSortField("value", SortField.Type.DOUBLE); - expectedSortField.setMissingValue(Double.POSITIVE_INFINITY); - assertEquals(expectedSortField, sortField); + assertMissingValue(sortField, Double.POSITIVE_INFINITY); Double randomDouble = randomDouble(); fieldSortBuilder = new FieldSortBuilder("value").missing(randomDouble); sortField = fieldSortBuilder.build(searchExecutionContext).field(); - expectedSortField = new SortedNumericSortField("value", SortField.Type.DOUBLE); - expectedSortField.setMissingValue(randomDouble); - assertEquals(expectedSortField, sortField); + assertMissingValue(sortField, randomDouble); fieldSortBuilder = new FieldSortBuilder("value").missing(randomDouble.toString()); sortField = fieldSortBuilder.build(searchExecutionContext).field(); - expectedSortField = new SortedNumericSortField("value", SortField.Type.DOUBLE); - expectedSortField.setMissingValue(randomDouble); - assertEquals(expectedSortField, sortField); + assertMissingValue(sortField, randomDouble); } - // Specialised assertEquals to handle subclasses of SortedNumericSortField - public static void assertEquals(SortedNumericSortField expected, SortField actual) { - assertEquals(expected.getField(), actual.getField()); - assertEquals(expected.getMissingValue(), actual.getMissingValue()); - assertEquals(expected.getType(), actual.getType()); - assertEquals(expected.getReverse(), actual.getReverse()); + private static void assertMissingValue(SortField sortField, Object missingValue) { + assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); + XFieldComparatorSource xFieldComparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); + assertEquals(xFieldComparatorSource.missingObject(missingValue, true), missingValue); } /** @@ -190,21 +178,15 @@ public void testBuildSortFieldOrder() throws IOException { SearchExecutionContext searchExecutionContext = createMockSearchExecutionContext(); FieldSortBuilder fieldSortBuilder = new FieldSortBuilder("value"); SortField sortField = fieldSortBuilder.build(searchExecutionContext).field(); - SortedNumericSortField expectedSortField = new SortedNumericSortField("value", SortField.Type.DOUBLE, false); - expectedSortField.setMissingValue(Double.POSITIVE_INFINITY); - assertEquals(expectedSortField, sortField); + assertMissingValue(sortField, Double.POSITIVE_INFINITY); fieldSortBuilder = new FieldSortBuilder("value").order(SortOrder.ASC); sortField = fieldSortBuilder.build(searchExecutionContext).field(); - expectedSortField = new SortedNumericSortField("value", SortField.Type.DOUBLE, false); - expectedSortField.setMissingValue(Double.POSITIVE_INFINITY); - assertEquals(expectedSortField, sortField); + assertMissingValue(sortField, Double.POSITIVE_INFINITY); fieldSortBuilder = new FieldSortBuilder("value").order(SortOrder.DESC); sortField = fieldSortBuilder.build(searchExecutionContext).field(); - expectedSortField = new SortedNumericSortField("value", SortField.Type.DOUBLE, true, SortedNumericSelector.Type.MAX); - expectedSortField.setMissingValue(Double.NEGATIVE_INFINITY); - assertEquals(expectedSortField, sortField); + assertMissingValue(sortField, Double.NEGATIVE_INFINITY); } /** @@ -215,46 +197,38 @@ public void testMultiValueMode() throws IOException { FieldSortBuilder sortBuilder = new FieldSortBuilder("value").sortMode(SortMode.MIN); SortField sortField = sortBuilder.build(searchExecutionContext).field(); - assertThat(sortField, instanceOf(SortedNumericSortField.class)); - SortedNumericSortField numericSortField = (SortedNumericSortField) sortField; - assertEquals(SortedNumericSelector.Type.MIN, numericSortField.getSelector()); + assertMode(sortField, MultiValueMode.MIN); sortBuilder = new FieldSortBuilder("value").sortMode(SortMode.MAX); sortField = sortBuilder.build(searchExecutionContext).field(); - assertThat(sortField, instanceOf(SortedNumericSortField.class)); - numericSortField = (SortedNumericSortField) sortField; - assertEquals(SortedNumericSelector.Type.MAX, numericSortField.getSelector()); + assertMode(sortField, MultiValueMode.MAX); sortBuilder = new FieldSortBuilder("value").sortMode(SortMode.SUM); sortField = sortBuilder.build(searchExecutionContext).field(); - assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); - XFieldComparatorSource comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); - assertEquals(MultiValueMode.SUM, comparatorSource.sortMode()); + assertMode(sortField, MultiValueMode.SUM); sortBuilder = new FieldSortBuilder("value").sortMode(SortMode.AVG); sortField = sortBuilder.build(searchExecutionContext).field(); - assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); - comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); - assertEquals(MultiValueMode.AVG, comparatorSource.sortMode()); + assertMode(sortField, MultiValueMode.AVG); sortBuilder = new FieldSortBuilder("value").sortMode(SortMode.MEDIAN); sortField = sortBuilder.build(searchExecutionContext).field(); - assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); - comparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); - assertEquals(MultiValueMode.MEDIAN, comparatorSource.sortMode()); + assertMode(sortField, MultiValueMode.MEDIAN); // sort mode should also be set by build() implicitly to MIN or MAX if not set explicitly on builder sortBuilder = new FieldSortBuilder("value"); sortField = sortBuilder.build(searchExecutionContext).field(); - assertThat(sortField, instanceOf(SortedNumericSortField.class)); - numericSortField = (SortedNumericSortField) sortField; - assertEquals(SortedNumericSelector.Type.MIN, numericSortField.getSelector()); + assertMode(sortField, MultiValueMode.MIN); sortBuilder = new FieldSortBuilder("value").order(SortOrder.DESC); sortField = sortBuilder.build(searchExecutionContext).field(); - assertThat(sortField, instanceOf(SortedNumericSortField.class)); - numericSortField = (SortedNumericSortField) sortField; - assertEquals(SortedNumericSelector.Type.MAX, numericSortField.getSelector()); + assertMode(sortField, MultiValueMode.MAX); + } + + private static void assertMode(SortField sortField, MultiValueMode mode) { + assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); + XFieldComparatorSource xFieldComparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); + assertEquals(mode, xFieldComparatorSource.sortMode()); } /** diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java index 7e32e5e2b1c00..3e7bea4a91139 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java @@ -20,6 +20,7 @@ import org.apache.lucene.index.LeafReader; import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.NoMergePolicy; +import org.apache.lucene.search.DocIdSetIterator; import org.apache.lucene.search.FieldExistsQuery; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.Pruning; @@ -76,6 +77,7 @@ import org.elasticsearch.xcontent.XContentFactory; import org.elasticsearch.xcontent.json.JsonXContent; import org.hamcrest.Matcher; +import org.junit.Assert; import java.io.IOException; import java.util.ArrayList; @@ -1709,6 +1711,14 @@ protected T compileOtherScript(Script script, ScriptContext context) { */ protected abstract List getSortShortcutSupport(); + private static Consumer checkNotNull(boolean supportsShortcut) { + if (supportsShortcut) { + return Assert::assertNotNull; + } else { + return Assert::assertNull; + } + } + public record SortShortcutSupport( IndexVersion indexVersion, Settings settings, @@ -1716,14 +1726,14 @@ public record SortShortcutSupport( CheckedConsumer fieldMapping, CheckedConsumer additionalMappings, CheckedConsumer document, - boolean supportsShortcut + Consumer competitiveIteratorCheck ) { public SortShortcutSupport( CheckedConsumer mappings, CheckedConsumer document, boolean supportsShortcut ) { - this(IndexVersion.current(), SETTINGS, "field", mappings, b -> {}, document, supportsShortcut); + this(IndexVersion.current(), SETTINGS, "field", mappings, b -> {}, document, checkNotNull(supportsShortcut)); } public SortShortcutSupport( @@ -1732,7 +1742,7 @@ public SortShortcutSupport( CheckedConsumer document, boolean supportsShortcut ) { - this(indexVersion, SETTINGS, "field", mappings, b -> {}, document, supportsShortcut); + this(indexVersion, SETTINGS, "field", mappings, b -> {}, document, checkNotNull(supportsShortcut)); } } @@ -1750,7 +1760,7 @@ public final void testSortShortcuts() throws IOException { Set::of, MappedFieldType.FielddataOperation.SEARCH ) - ).build(null, null).sortField(IndexVersion.current(), null, MultiValueMode.MIN, null, false); + ).build(null, null).sortField(false, IndexVersion.current(), null, MultiValueMode.MIN, null, false); }); } else { for (SortShortcutSupport sortShortcutSupport : tests) { @@ -1778,14 +1788,12 @@ public final void testSortShortcuts() throws IOException { throw new UnsupportedOperationException(); }, Set::of, MappedFieldType.FielddataOperation.SEARCH)) .build(null, null) - .sortField(getVersion(), null, MultiValueMode.MIN, null, false); - var comparator = sortField.getComparator(10, Pruning.GREATER_THAN_OR_EQUAL_TO); + .sortField(false, getVersion(), null, MultiValueMode.MIN, null, false); + var comparator = sortField.getComparator(1, Pruning.GREATER_THAN_OR_EQUAL_TO); var leafComparator = comparator.getLeafComparator(searcher.getLeafContexts().getFirst()); - if (sortShortcutSupport.supportsShortcut) { - assertNotNull(leafComparator.competitiveIterator()); - } else { - assertNull(leafComparator.competitiveIterator()); - } + leafComparator.setHitsThresholdReached(); + leafComparator.setBottom(0); + sortShortcutSupport.competitiveIteratorCheck.accept(leafComparator.competitiveIterator()); }); } } From adcd577a4046d76383bdcf0ee255cb992555a302 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 12 Nov 2025 15:53:21 +0000 Subject: [PATCH 2/2] Correctly use missing value in points-based pruning --- .../DoubleValuesComparatorSource.java | 4 +- .../FloatValuesComparatorSource.java | 2 +- .../HalfFloatValuesComparatorSource.java | 4 +- .../IntValuesComparatorSource.java | 4 +- .../LongValuesComparatorSource.java | 4 +- .../search/sort/FieldSortBuilderTests.java | 79 +++++++++++++++---- 6 files changed, 67 insertions(+), 30 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/DoubleValuesComparatorSource.java b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/DoubleValuesComparatorSource.java index d01ba7dc44734..a002357399c3a 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/DoubleValuesComparatorSource.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/DoubleValuesComparatorSource.java @@ -78,9 +78,7 @@ public FieldComparator newComparator(String fieldname, int numHits, Pruning e assert indexFieldData == null || fieldname.equals(indexFieldData.getFieldName()); final double dMissingValue = (Double) missingObject(missingValue, reversed); - // NOTE: it's important to pass null as a missing value in the constructor so that - // the comparator doesn't check docsWithField since we replace missing values in select() - return new DoubleComparator(numHits, fieldname, null, reversed, enableSkipping) { + return new DoubleComparator(numHits, fieldname, dMissingValue, reversed, enableSkipping) { @Override public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException { return new DoubleLeafComparator(context) { diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/FloatValuesComparatorSource.java b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/FloatValuesComparatorSource.java index 5a084fa55a859..8840aafd63595 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/FloatValuesComparatorSource.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/FloatValuesComparatorSource.java @@ -73,7 +73,7 @@ public FieldComparator newComparator(String fieldname, int numHits, Pruning e final float fMissingValue = (Float) missingObject(missingValue, reversed); // NOTE: it's important to pass null as a missing value in the constructor so that // the comparator doesn't check docsWithField since we replace missing values in select() - return new FloatComparator(numHits, fieldname, null, reversed, enableSkipping) { + return new FloatComparator(numHits, fieldname, fMissingValue, reversed, enableSkipping) { @Override public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException { return new FloatLeafComparator(context) { diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java index 15b01ab6ed8ba..0b4df5dac4c9a 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/HalfFloatValuesComparatorSource.java @@ -44,9 +44,7 @@ public FieldComparator newComparator(String fieldname, int numHits, Pruning e assert indexFieldData == null || fieldname.equals(indexFieldData.getFieldName()); final float fMissingValue = (Float) missingObject(missingValue, reversed); - // NOTE: it's important to pass null as a missing value in the constructor so that - // the comparator doesn't check docsWithField since we replace missing values in select() - return new HalfFloatComparator(numHits, fieldname, null, reversed, enableSkipping) { + return new HalfFloatComparator(numHits, fieldname, fMissingValue, reversed, enableSkipping) { @Override public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException { return new HalfFloatLeafComparator(context) { diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/IntValuesComparatorSource.java b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/IntValuesComparatorSource.java index d2fdd2f2900a6..4c16f2caf7d2c 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/IntValuesComparatorSource.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/IntValuesComparatorSource.java @@ -52,9 +52,7 @@ public FieldComparator newComparator(String fieldname, int numHits, Pruning e assert indexFieldData == null || fieldname.equals(indexFieldData.getFieldName()); final int iMissingValue = (Integer) missingObject(missingValue, reversed); - // NOTE: it's important to pass null as a missing value in the constructor so that - // the comparator doesn't check docsWithField since we replace missing values in select() - return new IntComparator(numHits, fieldname, null, reversed, enableSkipping) { + return new IntComparator(numHits, fieldname, iMissingValue, reversed, enableSkipping) { @Override public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException { return new IntLeafComparator(context) { diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/LongValuesComparatorSource.java b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/LongValuesComparatorSource.java index da1272a6f38d9..0f3179b546a88 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/LongValuesComparatorSource.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/fieldcomparator/LongValuesComparatorSource.java @@ -105,9 +105,7 @@ public FieldComparator newComparator(String fieldname, int numHits, Pruning e assert indexFieldData == null || fieldname.equals(indexFieldData.getFieldName()); final long lMissingValue = (Long) missingObject(missingValue, reversed); - // NOTE: it's important to pass null as a missing value in the constructor so that - // the comparator doesn't check docsWithField since we replace missing values in select() - return new XLongComparator(numHits, fieldname, null, reversed, enableSkipping) { + return new XLongComparator(numHits, fieldname, lMissingValue, reversed, enableSkipping) { @Override public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException { final int maxDoc = context.reader().maxDoc(); diff --git a/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java b/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java index 1f2a369f17e21..e4daad10fd71b 100644 --- a/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java @@ -145,30 +145,78 @@ protected void sortFieldAssertions(FieldSortBuilder builder, SortField sortField /** * Test that missing values get transferred correctly to the SortField */ - public void testBuildSortFieldMissingValue() throws IOException { + public void testBuildSortFieldDoubleMissingValue() throws IOException { SearchExecutionContext searchExecutionContext = createMockSearchExecutionContext(); FieldSortBuilder fieldSortBuilder = new FieldSortBuilder("value").missing("_first"); - SortField sortField = fieldSortBuilder.build(searchExecutionContext).field(); - assertMissingValue(sortField, Double.NEGATIVE_INFINITY); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Double.NEGATIVE_INFINITY); fieldSortBuilder = new FieldSortBuilder("value").missing("_last"); - sortField = fieldSortBuilder.build(searchExecutionContext).field(); - assertMissingValue(sortField, Double.POSITIVE_INFINITY); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Double.POSITIVE_INFINITY); Double randomDouble = randomDouble(); fieldSortBuilder = new FieldSortBuilder("value").missing(randomDouble); - sortField = fieldSortBuilder.build(searchExecutionContext).field(); - assertMissingValue(sortField, randomDouble); + assertMissingValue(fieldSortBuilder, searchExecutionContext, randomDouble); fieldSortBuilder = new FieldSortBuilder("value").missing(randomDouble.toString()); - sortField = fieldSortBuilder.build(searchExecutionContext).field(); - assertMissingValue(sortField, randomDouble); + assertMissingValue(fieldSortBuilder, searchExecutionContext, randomDouble); } - private static void assertMissingValue(SortField sortField, Object missingValue) { + public void testBuildSortFieldIntegerMissingValue() throws IOException { + SearchExecutionContext searchExecutionContext = createMockSearchExecutionContext(); + String fieldName = "custom-integer"; + FieldSortBuilder fieldSortBuilder = new FieldSortBuilder(fieldName).missing("_first"); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Integer.MIN_VALUE); + + fieldSortBuilder = new FieldSortBuilder(fieldName).missing("_last"); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Integer.MAX_VALUE); + + Integer randomInt = randomInt(); + fieldSortBuilder = new FieldSortBuilder(fieldName).missing(randomInt); + assertMissingValue(fieldSortBuilder, searchExecutionContext, randomInt); + + fieldSortBuilder = new FieldSortBuilder(fieldName).missing(randomInt.toString()); + assertMissingValue(fieldSortBuilder, searchExecutionContext, randomInt); + } + + public void testBuildSortFieldShortMissingValue() throws IOException { + SearchExecutionContext searchExecutionContext = createMockSearchExecutionContext(); + String fieldName = "custom-short"; + FieldSortBuilder fieldSortBuilder = new FieldSortBuilder(fieldName).missing("_first"); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Integer.MIN_VALUE); + + fieldSortBuilder = new FieldSortBuilder(fieldName).missing("_last"); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Integer.MAX_VALUE); + + Integer randomInt = randomInt(); + fieldSortBuilder = new FieldSortBuilder(fieldName).missing(randomInt); + assertMissingValue(fieldSortBuilder, searchExecutionContext, randomInt); + + fieldSortBuilder = new FieldSortBuilder(fieldName).missing(randomInt.toString()); + assertMissingValue(fieldSortBuilder, searchExecutionContext, randomInt); + } + + public void testBuildSortFieldByteMissingValue() throws IOException { + SearchExecutionContext searchExecutionContext = createMockSearchExecutionContext(); + String fieldName = "custom-byte"; + FieldSortBuilder fieldSortBuilder = new FieldSortBuilder(fieldName).missing("_first"); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Integer.MIN_VALUE); + + fieldSortBuilder = new FieldSortBuilder(fieldName).missing("_last"); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Integer.MAX_VALUE); + + Byte randomByte = randomByte(); + fieldSortBuilder = new FieldSortBuilder(fieldName).missing(randomByte); + assertMissingValue(fieldSortBuilder, searchExecutionContext, (int) randomByte); + + fieldSortBuilder = new FieldSortBuilder(fieldName).missing(randomByte.toString()); + assertMissingValue(fieldSortBuilder, searchExecutionContext, (int) randomByte); + } + + private static void assertMissingValue(FieldSortBuilder fsb, SearchExecutionContext ctx, Object missingValue) throws IOException { + SortField sortField = fsb.build(ctx).field(); assertThat(sortField.getComparatorSource(), instanceOf(XFieldComparatorSource.class)); XFieldComparatorSource xFieldComparatorSource = (XFieldComparatorSource) sortField.getComparatorSource(); - assertEquals(xFieldComparatorSource.missingObject(missingValue, true), missingValue); + assertEquals(xFieldComparatorSource.missingObject(fsb.missing(), fsb.order() == SortOrder.DESC), missingValue); } /** @@ -177,16 +225,13 @@ private static void assertMissingValue(SortField sortField, Object missingValue) public void testBuildSortFieldOrder() throws IOException { SearchExecutionContext searchExecutionContext = createMockSearchExecutionContext(); FieldSortBuilder fieldSortBuilder = new FieldSortBuilder("value"); - SortField sortField = fieldSortBuilder.build(searchExecutionContext).field(); - assertMissingValue(sortField, Double.POSITIVE_INFINITY); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Double.POSITIVE_INFINITY); fieldSortBuilder = new FieldSortBuilder("value").order(SortOrder.ASC); - sortField = fieldSortBuilder.build(searchExecutionContext).field(); - assertMissingValue(sortField, Double.POSITIVE_INFINITY); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Double.POSITIVE_INFINITY); fieldSortBuilder = new FieldSortBuilder("value").order(SortOrder.DESC); - sortField = fieldSortBuilder.build(searchExecutionContext).field(); - assertMissingValue(sortField, Double.NEGATIVE_INFINITY); + assertMissingValue(fieldSortBuilder, searchExecutionContext, Double.NEGATIVE_INFINITY); } /**