diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a7fd77591249..7a35898a3702f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Rename WorkloadGroupTestUtil to WorkloadManagementTestUtil ([#18709](https://github.com/opensearch-project/OpenSearch/pull/18709)) - Disallow resize for Warm Index, add Parameterized ITs for close in remote store ([#18686](https://github.com/opensearch-project/OpenSearch/pull/18686)) - Ability to run Code Coverage with Gradle and produce the jacoco reports locally ([#18509](https://github.com/opensearch-project/OpenSearch/issues/18509)) +- Extend BooleanQuery must_not rewrite to numeric must, term, and terms queries ([#18498](https://github.com/opensearch-project/OpenSearch/pull/18498)) - [Workload Management] Update logging and Javadoc, rename QueryGroup to WorkloadGroup ([#18711](https://github.com/opensearch-project/OpenSearch/issues/18711)) - Add NodeResourceUsageStats to ClusterInfo ([#18480](https://github.com/opensearch-project/OpenSearch/issues/18472)) - Introduce SecureHttpTransportParameters experimental API (to complement SecureTransportParameters counterpart) ([#18572](https://github.com/opensearch-project/OpenSearch/issues/18572)) diff --git a/server/src/internalClusterTest/java/org/opensearch/search/query/BooleanQueryIT.java b/server/src/internalClusterTest/java/org/opensearch/search/query/BooleanQueryIT.java index 0a1255004499f..b185b8b0131d1 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/query/BooleanQueryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/query/BooleanQueryIT.java @@ -13,13 +13,18 @@ import org.opensearch.common.settings.Settings; import org.opensearch.test.ParameterizedStaticSettingsOpenSearchIntegTestCase; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.List; +import java.util.Map; import static org.opensearch.index.query.QueryBuilders.boolQuery; import static org.opensearch.index.query.QueryBuilders.matchAllQuery; +import static org.opensearch.index.query.QueryBuilders.matchQuery; import static org.opensearch.index.query.QueryBuilders.rangeQuery; import static org.opensearch.index.query.QueryBuilders.termQuery; +import static org.opensearch.index.query.QueryBuilders.termsQuery; import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount; @@ -229,6 +234,53 @@ public void testMustNotRangeRewriteWithMoreThanOneValue() throws Exception { assertHitCount(client().prepareSearch().setQuery(matchAllQuery()).get(), numDocs); } + public void testMustNotNumericMatchOrTermQueryRewrite() throws Exception { + Map statusToDocCountMap = Map.of(200, 1000, 404, 30, 500, 1, 400, 1293); + String statusField = "status"; + createIndex("test"); + int totalDocs = 0; + for (Map.Entry entry : statusToDocCountMap.entrySet()) { + for (int i = 0; i < entry.getValue(); i++) { + client().prepareIndex("test").setSource(statusField, entry.getKey()).get(); + } + totalDocs += entry.getValue(); + } + ensureGreen(); + waitForRelocation(); + forceMerge(); + refresh(); + + int excludedValue = randomFrom(statusToDocCountMap.keySet()); + int expectedHitCount = totalDocs - statusToDocCountMap.get(excludedValue); + + // Check the rewritten match query behaves as expected + assertHitCount( + client().prepareSearch("test").setQuery(boolQuery().mustNot(matchQuery(statusField, excludedValue))).get(), + expectedHitCount + ); + + // Check the rewritten term query behaves as expected + assertHitCount( + client().prepareSearch("test").setQuery(boolQuery().mustNot(termQuery(statusField, excludedValue))).get(), + expectedHitCount + ); + + // Check a rewritten terms query behaves as expected + List excludedValues = new ArrayList<>(); + excludedValues.add(excludedValue); + int secondExcludedValue = randomFrom(statusToDocCountMap.keySet()); + expectedHitCount = totalDocs - statusToDocCountMap.get(excludedValue); + if (secondExcludedValue != excludedValue) { + excludedValues.add(secondExcludedValue); + expectedHitCount -= statusToDocCountMap.get(secondExcludedValue); + } + + assertHitCount( + client().prepareSearch("test").setQuery(boolQuery().mustNot(termsQuery(statusField, excludedValues))).get(), + expectedHitCount + ); + } + public void testMustToFilterRewrite() throws Exception { // Check we still get expected behavior after rewriting must clauses --> filter clauses. String intField = "int_field"; 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 6512007c9683e..c7c8e76614f79 100644 --- a/server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java @@ -1855,6 +1855,10 @@ public byte[] encodePoint(Number value) { public double toDoubleValue(long value) { return type.toDoubleValue(value); } + + public Number parse(Object value) { + return type.parse(value, coerce); + } } private final NumberType type; diff --git a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java index 82f24b6288cde..46c5a40457ce7 100644 --- a/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/BoolQueryBuilder.java @@ -485,31 +485,33 @@ private boolean rewriteMustNotRangeClausesToShould(BoolQueryBuilder newBuilder, return false; } + QueryShardContext shardContext = getQueryShardContext(queryRewriteContext); + boolean changed = false; - // For now, only handle the case where there's exactly 1 range query for this field. + // For now, only handle the case where there's exactly 1 complement-aware query for this field. Map fieldCounts = new HashMap<>(); - Set rangeQueries = new HashSet<>(); + Set complementAwareQueries = new HashSet<>(); for (QueryBuilder clause : mustNotClauses) { - if (clause instanceof RangeQueryBuilder rq) { - fieldCounts.merge(rq.fieldName(), 1, Integer::sum); - rangeQueries.add(rq); + if (clause instanceof ComplementAwareQueryBuilder && clause instanceof WithFieldName wfn) { + fieldCounts.merge(wfn.fieldName(), 1, Integer::sum); + complementAwareQueries.add((ComplementAwareQueryBuilder) wfn); } } - for (RangeQueryBuilder rq : rangeQueries) { - String fieldName = rq.fieldName(); + for (ComplementAwareQueryBuilder caq : complementAwareQueries) { + String fieldName = ((WithFieldName) caq).fieldName(); if (fieldCounts.getOrDefault(fieldName, 0) == 1) { // Check that all docs on this field have exactly 1 value, otherwise we can't perform this rewrite if (checkAllDocsHaveOneValue(leafReaderContexts, fieldName)) { - List complement = rq.getComplement(); + List complement = caq.getComplement(shardContext); if (complement != null) { BoolQueryBuilder nestedBoolQuery = new BoolQueryBuilder(); nestedBoolQuery.minimumShouldMatch(1); - for (RangeQueryBuilder complementComponent : complement) { + for (QueryBuilder complementComponent : complement) { nestedBoolQuery.should(complementComponent); } newBuilder.must(nestedBoolQuery); - newBuilder.mustNotClauses.remove(rq); + newBuilder.mustNotClauses.remove(caq); changed = true; } } @@ -537,6 +539,10 @@ private List getLeafReaderContexts(QueryRewriteContext queryR return indexSearcher.getIndexReader().leaves(); } + private QueryShardContext getQueryShardContext(QueryRewriteContext queryRewriteContext) { + return queryRewriteContext == null ? null : queryRewriteContext.convertToShardContext(); // Note this can still be null + } + private boolean checkAllDocsHaveOneValue(List contexts, String fieldName) { for (LeafReaderContext lrc : contexts) { PointValues values; diff --git a/server/src/main/java/org/opensearch/index/query/ComplementAwareQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/ComplementAwareQueryBuilder.java new file mode 100644 index 0000000000000..29d29a246c5bc --- /dev/null +++ b/server/src/main/java/org/opensearch/index/query/ComplementAwareQueryBuilder.java @@ -0,0 +1,23 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.query; + +import java.util.List; + +/** + * A QueryBuilder which can provide QueryBuilders that make up the complement of the original query. + */ +public interface ComplementAwareQueryBuilder { + /** + * Returns a list of RangeQueryBuilder whose elements, when combined, form the complement of this range query. + * May be null, in which case the complement couldn't be determined. + * @return the complement + */ + List getComplement(QueryShardContext context); +} diff --git a/server/src/main/java/org/opensearch/index/query/ComplementHelperUtils.java b/server/src/main/java/org/opensearch/index/query/ComplementHelperUtils.java new file mode 100644 index 0000000000000..0dfc439f32f65 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/query/ComplementHelperUtils.java @@ -0,0 +1,81 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.query; + +import org.opensearch.index.mapper.MappedFieldType; +import org.opensearch.index.mapper.NumberFieldMapper; + +import java.util.ArrayList; +import java.util.List; + +/** + * A class with helper functions to construct complements for some queries. + */ +public class ComplementHelperUtils { + /** + * Get the NumberFieldType for this fieldName from the context, or null if it isn't a NumberFieldType. + */ + public static NumberFieldMapper.NumberFieldType getNumberFieldType(QueryShardContext context, String fieldName) { + if (context == null) return null; + MappedFieldType fieldType = context.fieldMapper(fieldName); + if (!(fieldType instanceof NumberFieldMapper.NumberFieldType nft)) return null; + return nft; + } + + /** + * Returns a list of 2 RangeQueryBuilders matching everything but value. + */ + public static List numberValueToComplement(String fieldName, Number value) { + List complement = new ArrayList<>(); + RangeQueryBuilder belowRange = new RangeQueryBuilder(fieldName); + belowRange.to(value); + belowRange.includeUpper(false); + complement.add(belowRange); + + RangeQueryBuilder aboveRange = new RangeQueryBuilder(fieldName); + aboveRange.from(value); + aboveRange.includeLower(false); + complement.add(aboveRange); + return complement; + } + + /** + * Returns a list of RangeQueryBuilders matching everything except the provided sorted values. + * if isWholeNumber == true, and two sorted values are off by 1, the range between the two of them won't appear in + * the complement since no value could match it. + */ + public static List numberValuesToComplement(String fieldName, List sortedValues, boolean isWholeNumber) { + if (sortedValues.isEmpty()) return null; + List complement = new ArrayList<>(); + Number lastValue = null; + for (Number value : sortedValues) { + RangeQueryBuilder range = new RangeQueryBuilder(fieldName); + range.includeUpper(false); + range.to(value); + if (lastValue != null) { + // If this is a whole number field and the last value is 1 less than the current value, we can skip this part of the + // complement + if (isWholeNumber && value.longValue() - lastValue.longValue() == 1) { + continue; + } + range.includeLower(false); + range.from(lastValue); + } + complement.add(range); + lastValue = value; + } + // Finally add the last range query + RangeQueryBuilder lastRange = new RangeQueryBuilder(fieldName); + lastRange.from(sortedValues.get(sortedValues.size() - 1)); + lastRange.includeLower(false); + lastRange.includeUpper(true); + complement.add(lastRange); + return complement; + } +} diff --git a/server/src/main/java/org/opensearch/index/query/MatchQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/MatchQueryBuilder.java index 593077d18951e..e2e493be55977 100644 --- a/server/src/main/java/org/opensearch/index/query/MatchQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/MatchQueryBuilder.java @@ -43,11 +43,13 @@ import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.index.query.support.QueryParsers; import org.opensearch.index.search.MatchQuery; import org.opensearch.index.search.MatchQuery.ZeroTermsQuery; import java.io.IOException; +import java.util.List; import java.util.Objects; /** @@ -56,7 +58,7 @@ * * @opensearch.internal */ -public class MatchQueryBuilder extends AbstractQueryBuilder implements WithFieldName { +public class MatchQueryBuilder extends AbstractQueryBuilder implements ComplementAwareQueryBuilder, WithFieldName { private static final String CUTOFF_FREQUENCY_DEPRECATION_MSG = "you can omit this option, " + "the [match] query can skip block of documents efficiently if the total number of hits is not tracked"; @@ -589,4 +591,12 @@ public static MatchQueryBuilder fromXContent(XContentParser parser) throws IOExc return matchQuery; } + @Override + public List getComplement(QueryShardContext context) { + // If this is a match query on a numeric field, we can provide the complement using RangeQueryBuilder. + NumberFieldMapper.NumberFieldType nft = ComplementHelperUtils.getNumberFieldType(context, fieldName); + if (nft == null) return null; + Number numberValue = nft.parse(value); + return ComplementHelperUtils.numberValueToComplement(fieldName, numberValue); + } } diff --git a/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java index 79d6c54765243..cb4e1ad45efc8 100644 --- a/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java @@ -60,7 +60,10 @@ * * @opensearch.internal */ -public class RangeQueryBuilder extends AbstractQueryBuilder implements MultiTermQueryBuilder { +public class RangeQueryBuilder extends AbstractQueryBuilder + implements + MultiTermQueryBuilder, + ComplementAwareQueryBuilder { public static final String NAME = "range"; public static final boolean DEFAULT_INCLUDE_UPPER = true; @@ -545,12 +548,9 @@ protected boolean doEquals(RangeQueryBuilder other) { && Objects.equals(format, other.format); } - /** - * Returns a list of RangeQueryBuilder whose elements, when combined, form the complement of this range query. - * May be null. - * @return the complement - */ - public List getComplement() { + @Override + public List getComplement(QueryShardContext context) { + // This implementation doesn't need info from QueryShardContext if (relation != null && relation != ShapeRelation.INTERSECTS) { return null; } diff --git a/server/src/main/java/org/opensearch/index/query/TermQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/TermQueryBuilder.java index fe43fd00eae7a..ea8b92fd97b28 100644 --- a/server/src/main/java/org/opensearch/index/query/TermQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/TermQueryBuilder.java @@ -43,8 +43,10 @@ import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.mapper.ConstantFieldType; import org.opensearch.index.mapper.MappedFieldType; +import org.opensearch.index.mapper.NumberFieldMapper; import java.io.IOException; +import java.util.List; import java.util.Objects; /** @@ -52,7 +54,7 @@ * * @opensearch.internal */ -public class TermQueryBuilder extends BaseTermQueryBuilder { +public class TermQueryBuilder extends BaseTermQueryBuilder implements ComplementAwareQueryBuilder { public static final String NAME = "term"; public static final boolean DEFAULT_CASE_INSENSITIVITY = false; private static final ParseField CASE_INSENSITIVE_FIELD = new ParseField("case_insensitive"); @@ -238,4 +240,12 @@ protected final boolean doEquals(TermQueryBuilder other) { return super.doEquals(other) && Objects.equals(caseInsensitive, other.caseInsensitive); } + @Override + public List getComplement(QueryShardContext context) { + // If this is a term query on a numeric field, we can provide the complement using RangeQueryBuilder. + NumberFieldMapper.NumberFieldType nft = ComplementHelperUtils.getNumberFieldType(context, fieldName); + if (nft == null) return null; + Number numberValue = nft.parse(value); + return ComplementHelperUtils.numberValueToComplement(fieldName, numberValue); + } } diff --git a/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java index 70473c90b0603..805fd8b525482 100644 --- a/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java @@ -66,6 +66,7 @@ import java.util.Arrays; import java.util.Base64; import java.util.Collections; +import java.util.Comparator; import java.util.HashSet; import java.util.List; import java.util.Objects; @@ -79,7 +80,7 @@ * * @opensearch.internal */ -public class TermsQueryBuilder extends AbstractQueryBuilder implements WithFieldName { +public class TermsQueryBuilder extends AbstractQueryBuilder implements ComplementAwareQueryBuilder, WithFieldName { public static final String NAME = "terms"; private final String fieldName; @@ -639,4 +640,26 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) { return this; } + + @Override + public List getComplement(QueryShardContext context) { + // If this uses BITMAP value type, or if we're using termsLookup, we can't provide the complement. + if (valueType.equals(ValueType.BITMAP)) return null; + if (values == null || termsLookup != null) return null; + // If this is a terms query on a numeric field, we can provide the complement using RangeQueryBuilder. + NumberFieldMapper.NumberFieldType nft = ComplementHelperUtils.getNumberFieldType(context, fieldName); + if (nft == null) return null; + List numberValues = new ArrayList<>(); + for (Object value : values) { + numberValues.add(nft.parse(value)); + } + numberValues.sort(Comparator.comparingDouble(Number::doubleValue)); // For sorting purposes, use double value. + NumberFieldMapper.NumberType numberType = nft.numberType(); + // If there is some other field type that's a whole number, this will still be correct, the complement may just have some + // unnecessary components like "x < value < x + 1" + boolean isWholeNumber = numberType.equals(NumberFieldMapper.NumberType.INTEGER) + || numberType.equals(NumberFieldMapper.NumberType.LONG) + || numberType.equals(NumberFieldMapper.NumberType.SHORT); + return ComplementHelperUtils.numberValuesToComplement(fieldName, numberValues, isWholeNumber); + } } diff --git a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java index c9e988117ee9f..cb83b8e1986b9 100644 --- a/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/BoolQueryBuilderTests.java @@ -574,7 +574,7 @@ public void testOneSingleEndedMustNotRangeRewritten() throws Exception { IOUtils.close(w, reader, dir); } - public void testMultipleMustNotRangesNotRewritten() throws Exception { + public void testMultipleComplementAwareOnSameFieldNotRewritten() throws Exception { Directory dir = newDirectory(); IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); addDocument(w, INT_FIELD_NAME, 1); @@ -593,6 +593,16 @@ public void testMultipleMustNotRangesNotRewritten() throws Exception { assertTrue(rewritten.mustNot().contains(rq2of2)); assertEquals(0, rewritten.should().size()); + // Similarly 1 range query and 1 match query on the same field shouldn't be rewritten + qb = new BoolQueryBuilder(); + qb.mustNot(rq1of2); + QueryBuilder matchQuery = new MatchQueryBuilder(INT_FIELD_NAME, 200); + qb.mustNot(matchQuery); + rewritten = (BoolQueryBuilder) Rewriteable.rewrite(qb, createShardContext(searcher)); + assertTrue(rewritten.mustNot().contains(rq1of2)); + assertTrue(rewritten.mustNot().contains(matchQuery)); + assertEquals(0, rewritten.should().size()); + IOUtils.close(w, reader, dir); } @@ -631,6 +641,41 @@ public void testMustNotRewriteDisabledWithoutExactlyOneValuePerDoc() throws Exce IOUtils.close(w, reader, dir); } + public void testOneMustNotNumericMatchQueryRewritten() throws Exception { + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + addDocument(w, INT_FIELD_NAME, 1); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + BoolQueryBuilder qb = new BoolQueryBuilder(); + int excludedValue = 200; + QueryBuilder matchQuery = new MatchQueryBuilder(INT_FIELD_NAME, excludedValue); + qb.mustNot(matchQuery); + + BoolQueryBuilder rewritten = (BoolQueryBuilder) Rewriteable.rewrite(qb, createShardContext(searcher)); + assertFalse(rewritten.mustNot().contains(matchQuery)); + + QueryBuilder expectedLowerQuery = getRangeQueryBuilder(INT_FIELD_NAME, null, excludedValue, true, false); + QueryBuilder expectedUpperQuery = getRangeQueryBuilder(INT_FIELD_NAME, excludedValue, null, false, true); + assertEquals(1, rewritten.must().size()); + + BoolQueryBuilder nestedBoolQuery = (BoolQueryBuilder) rewritten.must().get(0); + assertEquals(2, nestedBoolQuery.should().size()); + assertEquals("1", nestedBoolQuery.minimumShouldMatch()); + assertTrue(nestedBoolQuery.should().contains(expectedLowerQuery)); + assertTrue(nestedBoolQuery.should().contains(expectedUpperQuery)); + + // When the QueryShardContext is null, we should not rewrite any match queries as we can't confirm if they're on numeric fields. + QueryRewriteContext nullContext = mock(QueryRewriteContext.class); + when(nullContext.convertToShardContext()).thenReturn(null); + BoolQueryBuilder rewrittenNoContext = (BoolQueryBuilder) Rewriteable.rewrite(qb, nullContext); + assertTrue(rewrittenNoContext.mustNot().contains(matchQuery)); + assertTrue(rewrittenNoContext.should().isEmpty()); + + IOUtils.close(w, reader, dir); + } + public void testMustClausesRewritten() throws Exception { BoolQueryBuilder qb = new BoolQueryBuilder(); @@ -718,7 +763,7 @@ private void addDocument(IndexWriter w, String fieldName, int... values) throws w.commit(); } - private IndexSearcher getIndexSearcher(DirectoryReader reader) throws Exception { + static IndexSearcher getIndexSearcher(DirectoryReader reader) throws Exception { SearchContext searchContext = mock(SearchContext.class); return new ContextIndexSearcher( reader, diff --git a/server/src/test/java/org/opensearch/index/query/MatchQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/MatchQueryBuilderTests.java index 81192fa0a18b2..e64311de88ace 100644 --- a/server/src/test/java/org/opensearch/index/query/MatchQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/MatchQueryBuilderTests.java @@ -34,6 +34,9 @@ import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.analysis.TokenStream; +import org.apache.lucene.analysis.standard.StandardAnalyzer; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.Term; import org.apache.lucene.queries.spans.SpanNearQuery; import org.apache.lucene.queries.spans.SpanOrQuery; @@ -50,6 +53,7 @@ import org.apache.lucene.search.Query; import org.apache.lucene.search.SynonymQuery; import org.apache.lucene.search.TermQuery; +import org.apache.lucene.store.Directory; import org.apache.lucene.tests.analysis.CannedBinaryTokenStream; import org.apache.lucene.tests.analysis.MockSynonymAnalyzer; import org.apache.lucene.util.BytesRef; @@ -58,6 +62,7 @@ import org.opensearch.common.compress.CompressedXContent; import org.opensearch.common.lucene.search.MultiPhrasePrefixQuery; import org.opensearch.common.lucene.search.Queries; +import org.opensearch.common.util.io.IOUtils; import org.opensearch.core.common.ParsingException; import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.mapper.MapperService; @@ -76,6 +81,7 @@ import java.util.Locale; import java.util.Map; +import static org.opensearch.index.query.BoolQueryBuilderTests.getIndexSearcher; import static org.hamcrest.CoreMatchers.either; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.Matchers.containsString; @@ -634,4 +640,40 @@ public void testCachingStrategiesWithNow() throws IOException { assertNotNull(rewritten.toQuery(context)); assertFalse("query should not be cacheable: " + queryBuilder.toString(), context.isCacheable()); } + + public void testGetComplement() throws Exception { + // getComplement() should return null if QueryShardContext is null + int value = 200; + MatchQueryBuilder queryBuilder = new MatchQueryBuilder(INT_FIELD_NAME, value); + assertNull(queryBuilder.getComplement(null)); + + // getComplement() should return 2 range queries if this is a numeric field + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + testGetComplementNumericField(queryBuilder, value, INT_FIELD_NAME, searcher); + + // should return null if this isn't a numeric field + queryBuilder = new MatchQueryBuilder(TEXT_FIELD_NAME, "some_text"); + assertNull(queryBuilder.getComplement(createShardContext(searcher))); + + IOUtils.close(w, reader, dir); + } + + // pkg-private so it can be reused in TermQueryBuilderTests + static void testGetComplementNumericField( + ComplementAwareQueryBuilder queryBuilder, + int value, + String fieldName, + IndexSearcher searcher + ) { + List complement = queryBuilder.getComplement(createShardContext(searcher)); + RangeQueryBuilder expectedLower = new RangeQueryBuilder(fieldName).to(value).includeLower(true).includeUpper(false); + RangeQueryBuilder expectedUpper = new RangeQueryBuilder(fieldName).from(value).includeLower(false).includeUpper(true); + assertEquals(2, complement.size()); + assertTrue(complement.contains(expectedLower)); + assertTrue(complement.contains(expectedUpper)); + } } diff --git a/server/src/test/java/org/opensearch/index/query/TermQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/TermQueryBuilderTests.java index c5bdf9b586df1..105a16a590d4f 100644 --- a/server/src/test/java/org/opensearch/index/query/TermQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/TermQueryBuilderTests.java @@ -34,18 +34,26 @@ import com.fasterxml.jackson.core.io.JsonStringEncoder; +import org.apache.lucene.analysis.standard.StandardAnalyzer; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.Term; import org.apache.lucene.search.AutomatonQuery; import org.apache.lucene.search.IndexOrDocValuesQuery; +import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.PointRangeQuery; import org.apache.lucene.search.Query; import org.apache.lucene.search.TermQuery; +import org.apache.lucene.store.Directory; +import org.opensearch.common.util.io.IOUtils; import org.opensearch.core.common.ParsingException; import org.opensearch.index.mapper.MappedFieldType; import java.io.IOException; +import static org.opensearch.index.query.BoolQueryBuilderTests.getIndexSearcher; +import static org.opensearch.index.query.MatchQueryBuilderTests.testGetComplementNumericField; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.Matchers.either; @@ -222,4 +230,25 @@ public void testMustRewrite() throws IOException { IllegalStateException e = expectThrows(IllegalStateException.class, () -> queryBuilder.toQuery(context)); assertEquals("Rewrite first", e.getMessage()); } + + public void testGetComplement() throws Exception { + // getComplement() should return null if QueryShardContext is null + int value = 200; + TermQueryBuilder queryBuilder = new TermQueryBuilder(INT_FIELD_NAME, value); + assertNull(queryBuilder.getComplement(null)); + + // getComplement() should return 2 range queries if this is a numeric field + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + testGetComplementNumericField(queryBuilder, value, INT_FIELD_NAME, searcher); + + // should return null if this isn't a numeric field + queryBuilder = new TermQueryBuilder(TEXT_FIELD_NAME, "some_text"); + assertNull(queryBuilder.getComplement(createShardContext(searcher))); + + IOUtils.close(w, reader, dir); + } } diff --git a/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java index cfe9fed3bc97c..e835a3b74133e 100644 --- a/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java @@ -32,22 +32,28 @@ package org.opensearch.index.query; +import org.apache.lucene.analysis.standard.StandardAnalyzer; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexWriter; import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.ConstantScoreQuery; import org.apache.lucene.search.FieldExistsQuery; import org.apache.lucene.search.IndexOrDocValuesQuery; +import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.PointInSetQuery; import org.apache.lucene.search.Query; import org.apache.lucene.search.TermInSetQuery; import org.apache.lucene.search.TermQuery; +import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; import org.opensearch.OpenSearchException; import org.opensearch.action.get.GetRequest; import org.opensearch.action.get.GetResponse; import org.opensearch.common.document.DocumentField; import org.opensearch.common.io.stream.BytesStreamOutput; +import org.opensearch.common.util.io.IOUtils; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.core.common.ParsingException; import org.opensearch.core.common.bytes.BytesArray; @@ -74,6 +80,7 @@ import org.roaringbitmap.RoaringBitmap; +import static org.opensearch.index.query.BoolQueryBuilderTests.getIndexSearcher; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.instanceOf; @@ -449,4 +456,101 @@ public void testTermsLookupBitmap() throws IOException { Query luceneQuery = rewritten.toQuery(context); assertTrue(luceneQuery instanceof IndexOrDocValuesQuery); } + + public void testGetComplementWholeNumber() throws Exception { + List values = List.of("200", "500", "304", "501"); + TermsQueryBuilder queryBuilder = new TermsQueryBuilder(INT_FIELD_NAME, values); + assertNull(queryBuilder.getComplement(null)); + + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + // Test multiple values + List complement = queryBuilder.getComplement(createShardContext(searcher)); + List expectedComplement = List.of( + new RangeQueryBuilder(INT_FIELD_NAME).to(200).includeLower(true).includeUpper(false), + new RangeQueryBuilder(INT_FIELD_NAME).from(200).to(304).includeLower(false).includeUpper(false), + new RangeQueryBuilder(INT_FIELD_NAME).from(304).to(500).includeLower(false).includeUpper(false), + // We don't expect a RangeQuery for 500 < value < 501, since nothing could match it on an int field + new RangeQueryBuilder(INT_FIELD_NAME).from(501).includeLower(false).includeUpper(true) + ); + assertEquals(complement, expectedComplement); + + // Test one value + String singleValue = "201"; + queryBuilder = new TermsQueryBuilder(INT_FIELD_NAME, singleValue); + expectedComplement = List.of( + new RangeQueryBuilder(INT_FIELD_NAME).to(201).includeLower(true).includeUpper(false), + new RangeQueryBuilder(INT_FIELD_NAME).from(201).includeLower(false).includeUpper(true) + ); + complement = queryBuilder.getComplement(createShardContext(searcher)); + assertEquals(complement, expectedComplement); + + // If zero values, we should get null + queryBuilder = new TermsQueryBuilder(INT_FIELD_NAME, List.of()); + complement = queryBuilder.getComplement(createShardContext(searcher)); + assertNull(complement); + IOUtils.close(w, reader, dir); + } + + public void testGetComplementDouble() throws Exception { + List values = List.of("200.0", "500.0", "304.12", "501.0"); + TermsQueryBuilder queryBuilder = new TermsQueryBuilder(DOUBLE_FIELD_NAME, values); + assertNull(queryBuilder.getComplement(null)); + + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + List complement = queryBuilder.getComplement(createShardContext(searcher)); + List expectedComplement = List.of( + new RangeQueryBuilder(DOUBLE_FIELD_NAME).to(200.0).includeLower(true).includeUpper(false), + new RangeQueryBuilder(DOUBLE_FIELD_NAME).from(200.0).to(304.12).includeLower(false).includeUpper(false), + new RangeQueryBuilder(DOUBLE_FIELD_NAME).from(304.12).to(500.0).includeLower(false).includeUpper(false), + new RangeQueryBuilder(DOUBLE_FIELD_NAME).from(500.0).to(501.0).includeLower(false).includeUpper(false), + new RangeQueryBuilder(DOUBLE_FIELD_NAME).from(501.0).includeLower(false).includeUpper(true) + ); + assertEquals(complement, expectedComplement); + IOUtils.close(w, reader, dir); + } + + public void testGetComplementNonNumericField() throws Exception { + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + TermsQueryBuilder queryBuilder = new TermsQueryBuilder(TEXT_FIELD_NAME, "some_text"); + assertNull(queryBuilder.getComplement(createShardContext(searcher))); + IOUtils.close(w, reader, dir); + } + + public void testGetComplementBitmap() throws Exception { + // Complement should return null if using bitmap value type. + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + TermsQueryBuilder queryBuilder = new TermsQueryBuilder(INT_FIELD_NAME, randomTermsLookup().store(true)).valueType( + TermsQueryBuilder.ValueType.BITMAP + ); + assertNull(queryBuilder.getComplement(createShardContext(searcher))); + IOUtils.close(w, reader, dir); + } + + public void testGetComplementValuesLookup() throws Exception { + // Complement should return null if the builder has termsLookup instead of values specified + Directory dir = newDirectory(); + IndexWriter w = new IndexWriter(dir, newIndexWriterConfig(new StandardAnalyzer())); + DirectoryReader reader = DirectoryReader.open(w); + IndexSearcher searcher = getIndexSearcher(reader); + + TermsQueryBuilder queryBuilder = new TermsQueryBuilder(INT_FIELD_NAME, randomTermsLookup().store(true)); + assertNull(queryBuilder.getComplement(createShardContext(searcher))); + IOUtils.close(w, reader, dir); + } }