From c2ab72a855d241ca78f03c920c862ae14ef2868b Mon Sep 17 00:00:00 2001 From: Asim Mahmood Date: Mon, 25 Aug 2025 15:01:59 -0700 Subject: [PATCH] Add skip_list param for date, scaled float and token count fields * follow up to #18889, which added to NumberFieldMapper * remaining are Boolean, which I'm not if its worth adding to * next step is to enable it by default, pending benchmark * update ScaledFloat testRangeQuery to apply same rounding logic to query as indexing to reduce the flakiness Signed-off-by: Asim Mahmood --- CHANGELOG.md | 1 + .../index/mapper/ScaledFloatFieldMapper.java | 22 +- .../index/mapper/TokenCountFieldMapper.java | 20 +- .../mapper/ScaledFloatFieldMapperTests.java | 35 +++ .../mapper/ScaledFloatFieldTypeTests.java | 75 ++++++ .../mapper/TokenCountFieldMapperTests.java | 33 ++- .../index/mapper/DateFieldMapper.java | 22 +- .../index/mapper/NumberFieldMapper.java | 6 +- .../fielddata/IndexFieldDataServiceTests.java | 3 + .../index/mapper/DateFieldMapperTests.java | 221 ++++++++++++++++++ .../index/mapper/NumberFieldTypeTests.java | 6 +- .../aggregations/AggregatorBaseTests.java | 1 + .../search/collapse/CollapseBuilderTests.java | 2 + .../search/query/QueryPhaseTests.java | 2 +- .../search/query/QueryProfilePhaseTests.java | 2 +- 15 files changed, 431 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec957f0e5c0dd..1e4da144a86fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add new extensible method to DocRequest to specify type ([#19313](https://github.com/opensearch-project/OpenSearch/pull/19313)) - [Rule based auto-tagging] Add Rule based auto-tagging IT ([#18550](https://github.com/opensearch-project/OpenSearch/pull/18550)) - Add all-active ingestion as docrep equivalent in pull-based ingestion ([#19316](https://github.com/opensearch-project/OpenSearch/pull/19316)) +- Add skip_list param for date, scaled float and token count fields ([#19142](https://github.com/opensearch-project/OpenSearch/pull/19142)) ### Changed - Refactor `if-else` chains to use `Java 17 pattern matching switch expressions`(([#18965](https://github.com/opensearch-project/OpenSearch/pull/18965)) diff --git a/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/ScaledFloatFieldMapper.java b/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/ScaledFloatFieldMapper.java index e17d56fcab3b8..c2821c633c686 100644 --- a/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/ScaledFloatFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/ScaledFloatFieldMapper.java @@ -118,7 +118,13 @@ public static class Builder extends ParametrizedFieldMapper.Builder { (n, c, o) -> o == null ? null : XContentMapValues.nodeDoubleValue(o), m -> toType(m).nullValue ).acceptsNull(); - + private final Parameter skiplist = new Parameter<>( + "skip_list", + false, + () -> false, + (n, c, o) -> XContentMapValues.nodeBooleanValue(o), + m -> toType(m).skiplist + ); private final Parameter> meta = Parameter.metaParam(); public Builder(String name, Settings settings) { @@ -148,7 +154,7 @@ Builder nullValue(double nullValue) { @Override protected List> getParameters() { - return Arrays.asList(indexed, hasDocValues, stored, ignoreMalformed, meta, scalingFactor, coerce, nullValue); + return Arrays.asList(indexed, hasDocValues, stored, ignoreMalformed, meta, scalingFactor, coerce, nullValue, skiplist); } @Override @@ -158,6 +164,7 @@ public ScaledFloatFieldMapper build(BuilderContext context) { indexed.getValue(), stored.getValue(), hasDocValues.getValue(), + skiplist.getValue(), meta.getValue(), scalingFactor.getValue(), nullValue.getValue() @@ -182,23 +189,27 @@ public static final class ScaledFloatFieldType extends SimpleMappedFieldType imp private final double scalingFactor; private final Double nullValue; + private final boolean skiplist; public ScaledFloatFieldType( String name, boolean indexed, boolean stored, boolean hasDocValues, + boolean skiplist, Map meta, double scalingFactor, Double nullValue ) { super(name, indexed, stored, hasDocValues, TextSearchInfo.SIMPLE_MATCH_ONLY, meta); + this.skiplist = skiplist; this.scalingFactor = scalingFactor; this.nullValue = nullValue; } public ScaledFloatFieldType(String name, double scalingFactor) { - this(name, true, false, true, Collections.emptyMap(), scalingFactor, null); + // TODO: enable skiplist by default + this(name, true, false, true, false, Collections.emptyMap(), scalingFactor, null); } @Override @@ -384,9 +395,9 @@ public double toDoubleValue(long value) { private final boolean indexed; private final boolean hasDocValues; private final boolean stored; + private final boolean skiplist; private final Double nullValue; private final double scalingFactor; - private final boolean ignoreMalformedByDefault; private final boolean coerceByDefault; @@ -401,6 +412,7 @@ private ScaledFloatFieldMapper( this.indexed = builder.indexed.getValue(); this.hasDocValues = builder.hasDocValues.getValue(); this.stored = builder.stored.getValue(); + this.skiplist = builder.skiplist.getValue(); this.scalingFactor = builder.scalingFactor.getValue(); this.nullValue = builder.nullValue.getValue(); this.ignoreMalformed = builder.ignoreMalformed.getValue(); @@ -491,7 +503,7 @@ protected void parseCreateField(ParseContext context) throws IOException { scaledValue, indexed, hasDocValues, - false, + skiplist, stored ); context.doc().addAll(fields); diff --git a/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/TokenCountFieldMapper.java b/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/TokenCountFieldMapper.java index 4ae0dba0d7839..929a9890a9ec9 100644 --- a/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/TokenCountFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/opensearch/index/mapper/TokenCountFieldMapper.java @@ -44,6 +44,7 @@ import java.util.List; import java.util.Map; +import static org.opensearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; import static org.opensearch.common.xcontent.support.XContentMapValues.nodeIntegerValue; /** @@ -77,6 +78,13 @@ public static class Builder extends ParametrizedFieldMapper.Builder { m -> toType(m).enablePositionIncrements, true ); + private final Parameter skiplist = new Parameter<>( + "skip_list", + false, + () -> false, + (n, c, o) -> nodeBooleanValue(o), + m -> toType(m).skiplist + ); private final Parameter> meta = Parameter.metaParam(); @@ -86,7 +94,7 @@ public Builder(String name) { @Override protected List> getParameters() { - return Arrays.asList(index, hasDocValues, store, analyzer, nullValue, enablePositionIncrements, meta); + return Arrays.asList(index, hasDocValues, store, analyzer, nullValue, enablePositionIncrements, meta, skiplist); } @Override @@ -99,6 +107,7 @@ public TokenCountFieldMapper build(BuilderContext context) { index.getValue(), store.getValue(), hasDocValues.getValue(), + skiplist.getValue(), nullValue.getValue(), meta.getValue() ); @@ -113,10 +122,11 @@ static class TokenCountFieldType extends NumberFieldMapper.NumberFieldType { boolean isSearchable, boolean isStored, boolean hasDocValues, + boolean skiplist, Number nullValue, Map meta ) { - super(name, NumberFieldMapper.NumberType.INTEGER, isSearchable, isStored, hasDocValues, false, nullValue, meta); + super(name, NumberFieldMapper.NumberType.INTEGER, isSearchable, isStored, hasDocValues, skiplist, false, nullValue, meta); } @Override @@ -132,6 +142,7 @@ public ValueFetcher valueFetcher(QueryShardContext context, SearchLookup searchL private final boolean index; private final boolean hasDocValues; + private final boolean skiplist; private final boolean store; private final NamedAnalyzer analyzer; private final boolean enablePositionIncrements; @@ -150,6 +161,7 @@ protected TokenCountFieldMapper( this.nullValue = builder.nullValue.getValue(); this.index = builder.index.getValue(); this.hasDocValues = builder.hasDocValues.getValue(); + this.skiplist = builder.skiplist.getValue(); this.store = builder.store.getValue(); } @@ -174,7 +186,9 @@ protected void parseCreateField(ParseContext context) throws IOException { } context.doc() - .addAll(NumberFieldMapper.NumberType.INTEGER.createFields(fieldType().name(), tokenCount, index, hasDocValues, false, store)); + .addAll( + NumberFieldMapper.NumberType.INTEGER.createFields(fieldType().name(), tokenCount, index, hasDocValues, skiplist, store) + ); } /** diff --git a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java index 95d88cc202f84..08354f786bac8 100644 --- a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java +++ b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldMapperTests.java @@ -37,6 +37,7 @@ import org.apache.lucene.document.SortedNumericDocValuesField; import org.apache.lucene.document.StoredField; import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.DocValuesSkipIndexType; import org.apache.lucene.index.DocValuesType; import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.IndexWriterConfig; @@ -85,6 +86,7 @@ protected void registerParameters(ParameterChecker checker) throws IOException { b.field("scaling_factor", 5.0); })); checker.registerConflictCheck("doc_values", b -> b.field("doc_values", false)); + checker.registerConflictCheck("skip_list", b -> b.field("skip_list", true)); checker.registerConflictCheck("index", b -> b.field("index", false)); checker.registerConflictCheck("store", b -> b.field("store", true)); checker.registerConflictCheck("null_value", b -> b.field("null_value", 1)); @@ -489,4 +491,37 @@ public void testScaledFloatEncodePoint() { assertEquals(1051, decodedUp); // 10.5 scaled = 1050, then +1 = 1051 (represents 10.51) assertEquals(1049, decodedDown); // 10.5 scaled = 1050, then -1 = 1049 (represents 10.49) } + + public void testSkiplistParameter() throws IOException { + // Test default value (none) + DocumentMapper defaultMapper = createDocumentMapper( + fieldMapping(b -> b.field("type", "scaled_float").field("scaling_factor", 100)) + ); + ParsedDocument doc = defaultMapper.parse(source(b -> b.field("field", 123.45))); + IndexableField[] fields = doc.rootDoc().getFields("field"); + assertEquals(2, fields.length); // point field + doc values field + IndexableField dvField = fields[1]; + assertEquals(DocValuesType.SORTED_NUMERIC, dvField.fieldType().docValuesType()); + assertEquals(DocValuesSkipIndexType.NONE, dvField.fieldType().docValuesSkipIndexType()); + + // Test skiplist = "skip_list" + DocumentMapper skiplistMapper = createDocumentMapper( + fieldMapping(b -> b.field("type", "scaled_float").field("scaling_factor", 100).field("skip_list", "true")) + ); + doc = skiplistMapper.parse(source(b -> b.field("field", 123.45))); + fields = doc.rootDoc().getFields("field"); + assertEquals(2, fields.length); + dvField = fields[1]; + assertEquals(DocValuesType.SORTED_NUMERIC, dvField.fieldType().docValuesType()); + assertEquals(DocValuesSkipIndexType.RANGE, dvField.fieldType().docValuesSkipIndexType()); + + // Test invalid value + MapperParsingException e = expectThrows( + MapperParsingException.class, + () -> createDocumentMapper( + fieldMapping(b -> b.field("type", "scaled_float").field("scaling_factor", 100).field("skip_list", "invalid")) + ) + ); + assertThat(e.getMessage(), containsString("Failed to parse value [invalid] as only [true] or [false] are allowed")); + } } diff --git a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldTypeTests.java b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldTypeTests.java index 8f8e2ad17a132..10ccdc02a0690 100644 --- a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldTypeTests.java +++ b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/ScaledFloatFieldTypeTests.java @@ -33,6 +33,7 @@ package org.opensearch.index.mapper; import org.apache.lucene.document.Document; +import org.apache.lucene.document.DoublePoint; import org.apache.lucene.document.LongField; import org.apache.lucene.document.LongPoint; import org.apache.lucene.document.SortedNumericDocValuesField; @@ -82,6 +83,79 @@ public void testTermsQuery() { assertEquals(LongField.newSetQuery("scaled_float", scaledValue1, scaledValue2), ft.termsQuery(Arrays.asList(value1, value2), null)); } + public void testRangeQuery() throws IOException { + // make sure the accuracy loss of scaled floats only occurs at index time + // this test checks that searching scaled floats yields the same results as + // searching doubles that are rounded to the closest half float + ScaledFloatFieldMapper.ScaledFloatFieldType ft = new ScaledFloatFieldMapper.ScaledFloatFieldType( + "scaled_float", + true, + false, + false, + false, + Collections.emptyMap(), + 0.1 + randomDouble() * 100, + null + ); + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, new IndexWriterConfig(null)); + long[] scaled_floats = new long[1000]; + double[] doubles = new double[1000]; + final int numDocs = 1000; + for (int i = 0; i < numDocs; ++i) { + Document doc = new Document(); + double value = (randomDouble() * 2 - 1) * 10000; + long scaledValue = Math.round(value * ft.getScalingFactor()); + double rounded = scaledValue / ft.getScalingFactor(); + scaled_floats[i] = scaledValue; + doubles[i] = rounded; + doc.add(new LongPoint("scaled_float", scaledValue)); + doc.add(new DoublePoint("double", rounded)); + w.addDocument(doc); + } + final DirectoryReader reader = DirectoryReader.open(w); + w.close(); + IndexSearcher searcher = newSearcher(reader); + final int numQueries = 1000; + for (int i = 0; i < numQueries; ++i) { + Double l = randomBoolean() ? null : (randomDouble() * 2 - 1) * 10000; + Double u = randomBoolean() ? null : (randomDouble() * 2 - 1) * 10000; + boolean includeLower = randomBoolean(); + boolean includeUpper = randomBoolean(); + + // Use the same rounding logic for query bounds as used in indexing + Double queryL = l; + Double queryU = u; + if (l != null) { + long scaledL = Math.round(l * ft.getScalingFactor()); + queryL = scaledL / ft.getScalingFactor(); + } + if (u != null) { + long scaledU = Math.round(u * ft.getScalingFactor()); + queryU = scaledU / ft.getScalingFactor(); + } + + Query doubleQ = NumberFieldMapper.NumberType.DOUBLE.rangeQuery( + "double", + queryL, + queryU, + includeLower, + includeUpper, + false, + true, + MOCK_QSC + ); + Query scaledFloatQ = ft.rangeQuery(queryL, queryU, includeLower, includeUpper, MOCK_QSC); + int expectedCount = searcher.count(doubleQ); + int scaledCount = searcher.count(scaledFloatQ); + + // System.out.println("l=" + l + " queryL=" + queryL + " u=" + u + " queryU=" + queryU + " scalingFactor=" + + // ft.getScalingFactor() + " expected= "+ expectedCount + " count= " + scaledCount); + assertEquals(expectedCount, scaledCount); + } + IOUtils.close(reader, dir); + } + public void testRoundsUpperBoundCorrectly() { ScaledFloatFieldMapper.ScaledFloatFieldType ft = new ScaledFloatFieldMapper.ScaledFloatFieldType("scaled_float", 100); Query scaledFloatQ = ft.rangeQuery(null, 0.1, true, false, MOCK_QSC); @@ -229,6 +303,7 @@ public void testLargeNumberIndexingAndQuerying() throws IOException { true, false, true, + true, Collections.emptyMap(), scalingFactor, null diff --git a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/TokenCountFieldMapperTests.java b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/TokenCountFieldMapperTests.java index 7790ed12c60f0..dd0f7485c6e4f 100644 --- a/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/TokenCountFieldMapperTests.java +++ b/modules/mapper-extras/src/test/java/org/opensearch/index/mapper/TokenCountFieldMapperTests.java @@ -36,6 +36,9 @@ import org.apache.lucene.analysis.TokenStream; import org.apache.lucene.analysis.core.KeywordAnalyzer; import org.apache.lucene.analysis.standard.StandardAnalyzer; +import org.apache.lucene.index.DocValuesSkipIndexType; +import org.apache.lucene.index.DocValuesType; +import org.apache.lucene.index.IndexableField; import org.apache.lucene.tests.analysis.CannedTokenStream; import org.apache.lucene.tests.analysis.MockTokenizer; import org.apache.lucene.tests.analysis.Token; @@ -80,6 +83,7 @@ protected void registerParameters(ParameterChecker checker) throws IOException { checker.registerConflictCheck("index", b -> b.field("index", false)); checker.registerConflictCheck("store", b -> b.field("store", true)); checker.registerConflictCheck("doc_values", b -> b.field("doc_values", false)); + checker.registerConflictCheck("skip_list", b -> b.field("skip_list", true)); checker.registerConflictCheck("null_value", b -> b.field("null_value", 1)); checker.registerConflictCheck("enable_position_increments", b -> b.field("enable_position_increments", false)); checker.registerUpdateCheck(this::minimalMapping, b -> b.field("type", "token_count").field("analyzer", "standard"), m -> { @@ -150,24 +154,42 @@ public TokenStreamComponents createComponents(String fieldName) { } public void testParseNullValue() throws Exception { - DocumentMapper mapper = createIndexWithTokenCountField(); + DocumentMapper mapper = createIndexWithTokenCountField(false); ParseContext.Document doc = parseDocument(mapper, createDocument(null)); assertNull(doc.getField("test.tc")); } public void testParseEmptyValue() throws Exception { - DocumentMapper mapper = createIndexWithTokenCountField(); + DocumentMapper mapper = createIndexWithTokenCountField(false); ParseContext.Document doc = parseDocument(mapper, createDocument("")); assertEquals(0, doc.getField("test.tc").numericValue()); } public void testParseNotNullValue() throws Exception { - DocumentMapper mapper = createIndexWithTokenCountField(); + DocumentMapper mapper = createIndexWithTokenCountField(false); ParseContext.Document doc = parseDocument(mapper, createDocument("three tokens string")); assertEquals(3, doc.getField("test.tc").numericValue()); + + IndexableField[] fields = doc.getFields("test.tc"); + assertEquals(2, fields.length); + IndexableField dvField = fields[1]; + assertEquals(DocValuesType.SORTED_NUMERIC, dvField.fieldType().docValuesType()); + assertEquals(DocValuesSkipIndexType.NONE, dvField.fieldType().docValuesSkipIndexType()); + } + + public void testParseNotNullValue_withSkiplist() throws Exception { + DocumentMapper mapper = createIndexWithTokenCountField(true); + ParseContext.Document doc = parseDocument(mapper, createDocument("three tokens string")); + assertEquals(3, doc.getField("test.tc").numericValue()); + + IndexableField[] fields = doc.getFields("test.tc"); + assertEquals(2, fields.length); + IndexableField dvField = fields[1]; + assertEquals(DocValuesType.SORTED_NUMERIC, dvField.fieldType().docValuesType()); + assertEquals(DocValuesSkipIndexType.RANGE, dvField.fieldType().docValuesSkipIndexType()); } - private DocumentMapper createIndexWithTokenCountField() throws IOException { + private DocumentMapper createIndexWithTokenCountField(boolean skiplist) throws IOException { return createDocumentMapper(mapping(b -> { b.startObject("test"); { @@ -178,6 +200,9 @@ private DocumentMapper createIndexWithTokenCountField() throws IOException { { b.field("type", "token_count"); b.field("analyzer", "standard"); + if (skiplist) { + b.field("skip_list", "true"); + } } b.endObject(); } diff --git a/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java index 57877f7ce92b1..270a4606b11c6 100644 --- a/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java @@ -57,6 +57,7 @@ import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.LocaleUtils; +import org.opensearch.common.xcontent.support.XContentMapValues; import org.opensearch.index.compositeindex.datacube.DimensionType; import org.opensearch.index.fielddata.IndexFieldData; import org.opensearch.index.fielddata.IndexNumericFieldData.NumericType; @@ -269,6 +270,13 @@ public static class Builder extends ParametrizedFieldMapper.Builder { private final Parameter index = Parameter.indexParam(m -> toType(m).indexed, true); private final Parameter docValues = Parameter.docValuesParam(m -> toType(m).hasDocValues, true); private final Parameter store = Parameter.storeParam(m -> toType(m).store, false); + private final Parameter skiplist = new Parameter<>( + "skip_list", + false, + () -> false, + (n, c, o) -> XContentMapValues.nodeBooleanValue(o), + m -> toType(m).skiplist + ); private final Parameter boost = Parameter.boostParam(); private final Parameter> meta = Parameter.metaParam(); @@ -337,7 +345,7 @@ private DateFormatter buildFormatter() { @Override protected List> getParameters() { - return Arrays.asList(index, docValues, store, format, printFormat, locale, nullValue, ignoreMalformed, boost, meta); + return Arrays.asList(index, docValues, store, skiplist, format, printFormat, locale, nullValue, ignoreMalformed, boost, meta); } private Long parseNullValue(DateFieldType fieldType) { @@ -744,6 +752,7 @@ public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) { private final boolean store; private final boolean indexed; private final boolean hasDocValues; + private final boolean skiplist; private final Locale locale; private final String format; private final String printFormat; @@ -768,6 +777,7 @@ private DateFieldMapper( this.store = builder.store.getValue(); this.indexed = builder.index.getValue(); this.hasDocValues = builder.docValues.getValue(); + this.skiplist = builder.skiplist.getValue(); this.locale = builder.locale.getValue(); this.format = builder.format.getValue(); this.printFormat = builder.printFormat.getValue(); @@ -836,7 +846,11 @@ protected void parseCreateField(ParseContext context) throws IOException { context.doc().add(new LongPoint(fieldType().name(), timestamp)); } if (hasDocValues) { - context.doc().add(new SortedNumericDocValuesField(fieldType().name(), timestamp)); + if (skiplist) { + context.doc().add(SortedNumericDocValuesField.indexedField(fieldType().name(), timestamp)); + } else { + context.doc().add(new SortedNumericDocValuesField(fieldType().name(), timestamp)); + } } else if (store || indexed) { createFieldNamesField(context); } @@ -849,6 +863,10 @@ public Long getNullValue() { return nullValue; } + public boolean skiplist() { + return skiplist; + } + @Override protected Explicit ignoreMalformed() { return ignoreMalformed; diff --git a/server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java index 30281b46c7c55..a04f8888a2347 100644 --- a/server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java @@ -1902,6 +1902,7 @@ public static class NumberFieldType extends SimpleMappedFieldType implements Num private final NumberType type; private final boolean coerce; private final Number nullValue; + private final boolean skiplist; public NumberFieldType( String name, @@ -1909,11 +1910,13 @@ public NumberFieldType( boolean isSearchable, boolean isStored, boolean hasDocValues, + boolean skiplist, boolean coerce, Number nullValue, Map meta ) { super(name, isSearchable, isStored, hasDocValues, TextSearchInfo.SIMPLE_MATCH_ONLY, meta); + this.skiplist = skiplist; this.type = Objects.requireNonNull(type); this.coerce = coerce; this.nullValue = nullValue; @@ -1927,6 +1930,7 @@ public NumberFieldType( builder.indexed.getValue(), builder.stored.getValue(), builder.hasDocValues.getValue(), + builder.skiplist.getValue(), builder.coerce.getValue().value(), builder.nullValue.getValue(), builder.meta.getValue() @@ -1934,7 +1938,7 @@ public NumberFieldType( } public NumberFieldType(String name, NumberType type) { - this(name, type, true, false, true, true, null, Collections.emptyMap()); + this(name, type, true, false, true, false, true, null, Collections.emptyMap()); } @Override diff --git a/server/src/test/java/org/opensearch/index/fielddata/IndexFieldDataServiceTests.java b/server/src/test/java/org/opensearch/index/fielddata/IndexFieldDataServiceTests.java index f79f233ddbcc7..b25ba593b847f 100644 --- a/server/src/test/java/org/opensearch/index/fielddata/IndexFieldDataServiceTests.java +++ b/server/src/test/java/org/opensearch/index/fielddata/IndexFieldDataServiceTests.java @@ -501,6 +501,7 @@ public void testRequireDocValuesOnLongs() { false, false, false, + false, null, Collections.emptyMap() ) @@ -517,6 +518,7 @@ public void testRequireDocValuesOnDoubles() { false, false, false, + false, null, Collections.emptyMap() ) @@ -533,6 +535,7 @@ public void testRequireDocValuesOnUnsignedLongs() { false, false, false, + false, null, Collections.emptyMap() ) diff --git a/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java index f099a8112b179..fd9bbbf3b79cd 100644 --- a/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/DateFieldMapperTests.java @@ -88,6 +88,7 @@ protected void registerParameters(ParameterChecker checker) throws IOException { checker.registerConflictCheck("doc_values", b -> b.field("doc_values", false)); checker.registerConflictCheck("index", b -> b.field("index", false)); checker.registerConflictCheck("store", b -> b.field("store", true)); + checker.registerConflictCheck("skip_list", b -> b.field("skip_list", true)); checker.registerConflictCheck("format", b -> b.field("format", "yyyy-MM-dd")); checker.registerConflictCheck("print_format", b -> b.field("print_format", "yyyy-MM-dd")); checker.registerConflictCheck("locale", b -> b.field("locale", "es")); @@ -643,4 +644,224 @@ public void testDateEncodePoint() { decoded = LongPoint.decodeDimension(encoded, 0); assertEquals("Should decrement epoch millis", negativeEpoch - 1, decoded); } + + public void testSkipListParameterValidBooleanValues() throws IOException { + // Test skip_list=true + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", true))); + DateFieldMapper dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertTrue("skip_list should be true", dateFieldMapper.skiplist()); + + // Test skip_list=false + mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", false))); + dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertFalse("skip_list should be false", dateFieldMapper.skiplist()); + } + + public void testSkipListParameterDefaultBehavior() throws IOException { + // Test default behavior when skip_list parameter is omitted + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date"))); + DateFieldMapper dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertFalse("skip_list should default to false", dateFieldMapper.skiplist()); + } + + public void testSkipListParameterInvalidValues() throws IOException { + // Test invalid string value + MapperParsingException e = expectThrows( + MapperParsingException.class, + () -> createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", "invalid"))) + ); + assertThat(e.getMessage(), containsString("Failed to parse mapping")); + + // Test invalid numeric value + e = expectThrows( + MapperParsingException.class, + () -> createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", 123))) + ); + assertThat(e.getMessage(), containsString("Failed to parse mapping")); + + // Test null value + e = expectThrows( + MapperParsingException.class, + () -> createDocumentMapper(fieldMapping(b -> b.field("type", "date").nullField("skip_list"))) + ); + assertThat(e.getMessage(), containsString("Failed to parse mapping")); + } + + public void testSkipListParameterDateNanos() throws IOException { + // Test skip_list=true with date_nanos type + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date_nanos").field("skip_list", true))); + DateFieldMapper dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertTrue("skip_list should be true for date_nanos", dateFieldMapper.skiplist()); + + // Test skip_list=false with date_nanos type + mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date_nanos").field("skip_list", false))); + dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertFalse("skip_list should be false for date_nanos", dateFieldMapper.skiplist()); + + // Test default behavior for date_nanos + mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date_nanos"))); + dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertFalse("skip_list should default to false for date_nanos", dateFieldMapper.skiplist()); + } + + // Integration tests for end-to-end skip_list functionality + + public void testSkipListIntegrationDateFieldWithIndexedDocValues() throws IOException { + // Test creating index with skip_list=true for date field + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", true))); + + // Parse a document with date field + ParsedDocument doc = mapper.parse(source(b -> b.field("field", "2016-03-11T10:30:00Z"))); + + // Verify the field structure + IndexableField[] fields = doc.rootDoc().getFields("field"); + assertEquals("Should have 2 fields (point and doc values)", 2, fields.length); + + // Verify point field + IndexableField pointField = fields[0]; + assertEquals(1, pointField.fieldType().pointIndexDimensionCount()); + assertEquals(8, pointField.fieldType().pointNumBytes()); + assertFalse(pointField.fieldType().stored()); + assertEquals(1457692200000L, pointField.numericValue().longValue()); + + // Verify doc values field - when skip_list=true, should use indexed doc values + IndexableField dvField = fields[1]; + assertEquals(DocValuesType.SORTED_NUMERIC, dvField.fieldType().docValuesType()); + assertEquals(1457692200000L, dvField.numericValue().longValue()); + assertFalse(dvField.fieldType().stored()); + + // Verify the mapper has skip_list enabled + DateFieldMapper dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertTrue("skip_list should be enabled", dateFieldMapper.skiplist()); + } + + public void testSkipListIntegrationDateNanosFieldWithIndexedDocValues() throws IOException { + // Test creating index with skip_list=true for date_nanos field + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date_nanos").field("skip_list", true))); + + // Parse a document with date_nanos field + ParsedDocument doc = mapper.parse(source(b -> b.field("field", "2016-03-11T10:30:00.123456789Z"))); + + // Verify the field structure + IndexableField[] fields = doc.rootDoc().getFields("field"); + assertEquals("Should have 2 fields (point and doc values)", 2, fields.length); + + // Verify point field + IndexableField pointField = fields[0]; + assertEquals(1, pointField.fieldType().pointIndexDimensionCount()); + assertEquals(8, pointField.fieldType().pointNumBytes()); + assertFalse(pointField.fieldType().stored()); + + // Verify doc values field - when skip_list=true, should use indexed doc values + IndexableField dvField = fields[1]; + assertEquals(DocValuesType.SORTED_NUMERIC, dvField.fieldType().docValuesType()); + assertFalse(dvField.fieldType().stored()); + + // Verify the mapper has skip_list enabled + DateFieldMapper dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertTrue("skip_list should be enabled", dateFieldMapper.skiplist()); + } + + public void testSkipListIntegrationRegularDocValuesWhenDisabled() throws IOException { + // Test creating index with skip_list=false for date field + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", false))); + + // Parse a document with date field + ParsedDocument doc = mapper.parse(source(b -> b.field("field", "2016-03-11T10:30:00Z"))); + + // Verify the field structure + IndexableField[] fields = doc.rootDoc().getFields("field"); + assertEquals("Should have 2 fields (point and doc values)", 2, fields.length); + + // Verify point field + IndexableField pointField = fields[0]; + assertEquals(1, pointField.fieldType().pointIndexDimensionCount()); + assertEquals(8, pointField.fieldType().pointNumBytes()); + assertFalse(pointField.fieldType().stored()); + assertEquals(1457692200000L, pointField.numericValue().longValue()); + + // Verify doc values field - when skip_list=false, should use regular doc values + IndexableField dvField = fields[1]; + assertEquals(DocValuesType.SORTED_NUMERIC, dvField.fieldType().docValuesType()); + assertEquals(1457692200000L, dvField.numericValue().longValue()); + assertFalse(dvField.fieldType().stored()); + + // Verify the mapper has skip_list disabled + DateFieldMapper dateFieldMapper = (DateFieldMapper) mapper.mappers().getMapper("field"); + assertFalse("skip_list should be disabled", dateFieldMapper.skiplist()); + } + + public void testSkipListIntegrationWithMultipleResolutions() throws IOException { + // Test both MILLISECONDS and NANOSECONDS resolution with skip_list enabled + + // Test MILLISECONDS resolution (date type) + DocumentMapper mapperMillis = createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", true))); + ParsedDocument docMillis = mapperMillis.parse(source(b -> b.field("field", "2016-03-11T10:30:00Z"))); + + IndexableField[] fieldsMillis = docMillis.rootDoc().getFields("field"); + assertEquals("Date field should have 2 fields", 2, fieldsMillis.length); + assertEquals(1457692200000L, fieldsMillis[0].numericValue().longValue()); + assertEquals(1457692200000L, fieldsMillis[1].numericValue().longValue()); + + DateFieldMapper dateMapperMillis = (DateFieldMapper) mapperMillis.mappers().getMapper("field"); + assertTrue("Date field skip_list should be enabled", dateMapperMillis.skiplist()); + + // Test NANOSECONDS resolution (date_nanos type) + DocumentMapper mapperNanos = createDocumentMapper(fieldMapping(b -> b.field("type", "date_nanos").field("skip_list", true))); + ParsedDocument docNanos = mapperNanos.parse(source(b -> b.field("field", "2016-03-11T10:30:00.123456789Z"))); + + IndexableField[] fieldsNanos = docNanos.rootDoc().getFields("field"); + assertEquals("Date_nanos field should have 2 fields", 2, fieldsNanos.length); + + DateFieldMapper dateMapperNanos = (DateFieldMapper) mapperNanos.mappers().getMapper("field"); + assertTrue("Date_nanos field skip_list should be enabled", dateMapperNanos.skiplist()); + } + + public void testSkipListIntegrationMappingDefinitionSerialization() throws IOException { + // Test that skip_list parameter appears correctly in mapping definitions + + // Create mapper with skip_list=true + MapperService mapperService = createMapperService(fieldMapping(b -> b.field("type", "date").field("skip_list", true))); + + // Get the field type and verify skip_list is set + DateFieldMapper dateFieldMapper = (DateFieldMapper) mapperService.documentMapper().mappers().getMapper("field"); + assertTrue("skip_list should be true in mapper", dateFieldMapper.skiplist()); + + // Test with date_nanos as well + MapperService mapperServiceNanos = createMapperService(fieldMapping(b -> b.field("type", "date_nanos").field("skip_list", true))); + DateFieldMapper dateFieldMapperNanos = (DateFieldMapper) mapperServiceNanos.documentMapper().mappers().getMapper("field"); + assertTrue("skip_list should be true in date_nanos mapper", dateFieldMapperNanos.skiplist()); + } + + public void testSkipListIntegrationFieldBehaviorConsistency() throws IOException { + // Test that field behavior is consistent between skip_list enabled and disabled + + String dateValue = "2016-03-11T10:30:00Z"; + long expectedTimestamp = 1457692200000L; + + // Test with skip_list=true + DocumentMapper mapperEnabled = createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", true))); + ParsedDocument docEnabled = mapperEnabled.parse(source(b -> b.field("field", dateValue))); + + // Test with skip_list=false + DocumentMapper mapperDisabled = createDocumentMapper(fieldMapping(b -> b.field("type", "date").field("skip_list", false))); + ParsedDocument docDisabled = mapperDisabled.parse(source(b -> b.field("field", dateValue))); + + // Both should have the same field structure and values + IndexableField[] fieldsEnabled = docEnabled.rootDoc().getFields("field"); + IndexableField[] fieldsDisabled = docDisabled.rootDoc().getFields("field"); + + assertEquals("Both should have same number of fields", fieldsEnabled.length, fieldsDisabled.length); + assertEquals( + "Point field values should match", + fieldsEnabled[0].numericValue().longValue(), + fieldsDisabled[0].numericValue().longValue() + ); + assertEquals( + "Doc values field values should match", + fieldsEnabled[1].numericValue().longValue(), + fieldsDisabled[1].numericValue().longValue() + ); + assertEquals("Expected timestamp should match", expectedTimestamp, fieldsEnabled[0].numericValue().longValue()); + } } diff --git a/server/src/test/java/org/opensearch/index/mapper/NumberFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/NumberFieldTypeTests.java index 8d794220ea6c5..5f2ba1f083cc5 100644 --- a/server/src/test/java/org/opensearch/index/mapper/NumberFieldTypeTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/NumberFieldTypeTests.java @@ -175,7 +175,7 @@ public void testLongTermQueryWithDecimalPart() { } private static MappedFieldType unsearchable() { - return new NumberFieldType("field", NumberType.LONG, false, false, false, true, null, Collections.emptyMap()); + return new NumberFieldType("field", NumberType.LONG, false, false, false, false, true, null, Collections.emptyMap()); } public void testTermQuery() { @@ -1004,10 +1004,10 @@ public void testBitmapQuery() throws IOException { ft.bitmapQuery(bitmap) ); - ft = new NumberFieldType("field", NumberType.INTEGER, false, false, true, true, null, Collections.emptyMap()); + ft = new NumberFieldType("field", NumberType.INTEGER, false, false, true, true, true, null, Collections.emptyMap()); assertEquals(new BitmapDocValuesQuery("field", r), ft.bitmapQuery(bitmap)); - ft = new NumberFieldType("field", NumberType.INTEGER, true, false, false, true, null, Collections.emptyMap()); + ft = new NumberFieldType("field", NumberType.INTEGER, true, false, false, false, true, null, Collections.emptyMap()); assertEquals(new BitmapIndexQuery("field", r), ft.bitmapQuery(bitmap)); Directory dir = newDirectory(); diff --git a/server/src/test/java/org/opensearch/search/aggregations/AggregatorBaseTests.java b/server/src/test/java/org/opensearch/search/aggregations/AggregatorBaseTests.java index 34c1f5ab7f218..e9df26bc0c6f3 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/AggregatorBaseTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/AggregatorBaseTests.java @@ -117,6 +117,7 @@ private ValuesSourceConfig getVSConfig( indexed, false, true, + true, false, null, Collections.emptyMap() diff --git a/server/src/test/java/org/opensearch/search/collapse/CollapseBuilderTests.java b/server/src/test/java/org/opensearch/search/collapse/CollapseBuilderTests.java index fff27ecf34509..375eb64803360 100644 --- a/server/src/test/java/org/opensearch/search/collapse/CollapseBuilderTests.java +++ b/server/src/test/java/org/opensearch/search/collapse/CollapseBuilderTests.java @@ -174,6 +174,7 @@ public void testBuild() throws IOException { false, false, false, + false, null, Collections.emptyMap() ); @@ -187,6 +188,7 @@ public void testBuild() throws IOException { false, false, true, + true, false, null, Collections.emptyMap() diff --git a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java index f00111b6160a2..4eadc7d806815 100644 --- a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java +++ b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java @@ -1107,7 +1107,7 @@ public void testCollapseQuerySearchResults() throws Exception { IndexReader reader = DirectoryReader.open(dir); QueryShardContext queryShardContext = mock(QueryShardContext.class); when(queryShardContext.fieldMapper("user")).thenReturn( - new NumberFieldType("user", NumberType.INTEGER, true, false, true, false, null, Collections.emptyMap()) + new NumberFieldType("user", NumberType.INTEGER, true, false, true, true, false, null, Collections.emptyMap()) ); TestSearchContext context = new TestSearchContext(queryShardContext, indexShard, newContextSearcher(reader, executor)); diff --git a/server/src/test/java/org/opensearch/search/query/QueryProfilePhaseTests.java b/server/src/test/java/org/opensearch/search/query/QueryProfilePhaseTests.java index ea4db0630c7d4..e092bad7d130a 100644 --- a/server/src/test/java/org/opensearch/search/query/QueryProfilePhaseTests.java +++ b/server/src/test/java/org/opensearch/search/query/QueryProfilePhaseTests.java @@ -1423,7 +1423,7 @@ public void testCollapseQuerySearchResults() throws Exception { IndexReader reader = DirectoryReader.open(dir); QueryShardContext queryShardContext = mock(QueryShardContext.class); when(queryShardContext.fieldMapper("user")).thenReturn( - new NumberFieldType("user", NumberType.INTEGER, true, false, true, false, null, Collections.emptyMap()) + new NumberFieldType("user", NumberType.INTEGER, true, false, true, true, false, null, Collections.emptyMap()) ); TestSearchContext context = new TestSearchContext(queryShardContext, indexShard, newContextSearcher(reader, executor));