From 641d61714b4ef9cf737e5fc481bc19fdb4497907 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Mon, 7 Apr 2025 23:52:54 +0530 Subject: [PATCH 01/36] Add bool filter provider Signed-off-by: Rishab Nahata --- .../provider/StarTreeFilterProvider.java | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java index 61f44ba3f163e..c024811a799d2 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java @@ -12,6 +12,7 @@ import org.opensearch.index.compositeindex.datacube.Dimension; import org.opensearch.index.mapper.CompositeDataCubeFieldType; import org.opensearch.index.mapper.MappedFieldType; +import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.MatchAllQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.RangeQueryBuilder; @@ -19,10 +20,13 @@ import org.opensearch.index.query.TermsQueryBuilder; import org.opensearch.search.internal.SearchContext; import org.opensearch.search.startree.StarTreeQueryHelper; +import org.opensearch.search.startree.filter.DimensionFilter; import org.opensearch.search.startree.filter.StarTreeFilter; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -59,7 +63,9 @@ class SingletonFactory { TermsQueryBuilder.NAME, new TermsStarTreeFilterProvider(), RangeQueryBuilder.NAME, - new RangeStarTreeFilterProvider() + new RangeStarTreeFilterProvider(), + BoolQueryBuilder.NAME, + new BoolStarTreeFilterProvider() ); public static StarTreeFilterProvider getProvider(QueryBuilder query) { @@ -156,4 +162,40 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C } + /** + * + */ + class BoolStarTreeFilterProvider implements StarTreeFilterProvider { + @Override + public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) + throws IOException { + BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) rawFilter; + if (boolQueryBuilder.hasClauses()) { + return null; // no clause present; fallback to default implementation + } + List mustClauses = boolQueryBuilder.must(); + if (mustClauses.isEmpty()) { + return null; // must clause not present; fallback to default implementation + } + Map> dimensionFilterMap = new HashMap<>(); + for (QueryBuilder clause : mustClauses) { + StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); + if (provider == null) { + return null; // Unsupported query type; fallback + } + + StarTreeFilter filter = provider.getFilter(context, clause, compositeFieldType); + if (filter == null) { + return null; // If any clause can't be converted, fallback + } + + // Merge filters into dimensionFilterMap + for (String dimension : filter.getDimensions()) { + List existingFilters = dimensionFilterMap.computeIfAbsent(dimension, k -> new ArrayList<>()); + existingFilters.addAll(filter.getFiltersForDimension(dimension)); + } + } + return new StarTreeFilter(dimensionFilterMap); + } + } } From 525e2d2a4392cceb6151b7aebd2bce36c481ee4c Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Tue, 15 Apr 2025 04:09:03 +0530 Subject: [PATCH 02/36] should clause support Signed-off-by: Rishab Nahata --- .../provider/StarTreeFilterProvider.java | 83 ++++++++-- .../DimensionFilterAndMapperTests.java | 153 ++++++++++++++++++ 2 files changed, 220 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java index c024811a799d2..700b795af2f64 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java @@ -170,32 +170,83 @@ class BoolStarTreeFilterProvider implements StarTreeFilterProvider { public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) throws IOException { BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) rawFilter; - if (boolQueryBuilder.hasClauses()) { + if (boolQueryBuilder.hasClauses() == false) { return null; // no clause present; fallback to default implementation } - List mustClauses = boolQueryBuilder.must(); - if (mustClauses.isEmpty()) { - return null; // must clause not present; fallback to default implementation - } Map> dimensionFilterMap = new HashMap<>(); - for (QueryBuilder clause : mustClauses) { - StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); - if (provider == null) { - return null; // Unsupported query type; fallback + + List mustClauses = boolQueryBuilder.must(); + if (mustClauses.isEmpty() == false) { + for (QueryBuilder clause : mustClauses) { + StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); + if (provider == null) { + return null; // Unsupported query type; fallback + } + + StarTreeFilter filter = provider.getFilter(context, clause, compositeFieldType); + if (filter == null) { + return null; // If any clause can't be converted, fallback + } + + // Merge filters into dimensionFilterMap + for (String dim : filter.getDimensions()) { + dimensionFilterMap.computeIfAbsent(dim, k -> new ArrayList<>()).addAll(filter.getFiltersForDimension(dim)); + } } + } - StarTreeFilter filter = provider.getFilter(context, clause, compositeFieldType); - if (filter == null) { - return null; // If any clause can't be converted, fallback + List shouldClauses = boolQueryBuilder.should(); + if (shouldClauses.isEmpty() == false) { + // Group 'should' clauses by dimension + Map> shouldClausesByDimension = new HashMap<>(); + + for (QueryBuilder clause : shouldClauses) { + String dimension = getDimensionFromQuery(clause); + if (dimension == null) { + return null; // Unsupported query type + } + shouldClausesByDimension.computeIfAbsent(dimension, k -> new ArrayList<>()).add(clause); + } + // If SHOULD clauses span multiple dimensions, we don't support it + if (shouldClausesByDimension.size() > 1) { + return null; } - // Merge filters into dimensionFilterMap - for (String dimension : filter.getDimensions()) { - List existingFilters = dimensionFilterMap.computeIfAbsent(dimension, k -> new ArrayList<>()); - existingFilters.addAll(filter.getFiltersForDimension(dimension)); + // Process 'should' clauses for the single dimension + for (Map.Entry> entry : shouldClausesByDimension.entrySet()) { + String dimension = entry.getKey(); + List clausesForDimension = entry.getValue(); + + List orFilters = new ArrayList<>(); + for (QueryBuilder clause : clausesForDimension) { + StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); + if (provider == null) { + return null; + } + + // TODO - nested boolean queries + StarTreeFilter filter = provider.getFilter(context, clause, compositeFieldType); + if (filter == null) { + return null; + } + + orFilters.addAll(filter.getFiltersForDimension(dimension)); + } + dimensionFilterMap.computeIfAbsent(dimension, k -> new ArrayList<>()).addAll(orFilters); } } return new StarTreeFilter(dimensionFilterMap); } + + private String getDimensionFromQuery(QueryBuilder query) { + if (query instanceof TermQueryBuilder) { + return ((TermQueryBuilder) query).fieldName(); + } else if (query instanceof TermsQueryBuilder) { + return ((TermsQueryBuilder) query).fieldName(); + } else if (query instanceof RangeQueryBuilder) { + return ((RangeQueryBuilder) query).fieldName(); + } + return null; + } } } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java index e89bc8e60e9da..26d860bc92848 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java @@ -20,8 +20,10 @@ import org.opensearch.index.mapper.CompositeDataCubeFieldType; import org.opensearch.index.mapper.KeywordFieldMapper; import org.opensearch.index.mapper.MapperService; +import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.index.mapper.StarTreeMapper; import org.opensearch.index.mapper.WildcardFieldMapper; +import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.RangeQueryBuilder; import org.opensearch.index.query.TermQueryBuilder; @@ -30,6 +32,7 @@ import org.opensearch.search.startree.filter.DimensionFilter; import org.opensearch.search.startree.filter.DimensionFilter.MatchType; import org.opensearch.search.startree.filter.MatchNoneFilter; +import org.opensearch.search.startree.filter.StarTreeFilter; import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import org.opensearch.search.startree.filter.provider.StarTreeFilterProvider; import org.opensearch.test.OpenSearchTestCase; @@ -190,4 +193,154 @@ public void testStarTreeFilterProviders() throws IOException { assertEquals(0, collector.collectedNodeCount()); } + public void testBoolStarTreeFilterProvider_MustClause() throws IOException { + CompositeDataCubeFieldType compositeDataCubeFieldType = new StarTreeMapper.StarTreeFieldType( + "star_tree", + new StarTreeField( + "star_tree", + List.of( + new OrdinalDimension("keyword"), + new OrdinalDimension("status"), + new OrdinalDimension("method") + ), + List.of(new Metric("field", List.of(MetricStat.MAX))), + new StarTreeFieldConfiguration( + randomIntBetween(1, 10_000), + Collections.emptySet(), + StarTreeFieldConfiguration.StarTreeBuildMode.ON_HEAP + ) + ) + ); + MapperService mapperService = mock(MapperService.class); + SearchContext searchContext = mock(SearchContext.class); + when(searchContext.mapperService()).thenReturn(mapperService); + + // Test empty bool query + BoolQueryBuilder emptyBoolQuery = new BoolQueryBuilder(); + StarTreeFilterProvider boolProvider = StarTreeFilterProvider.SingletonFactory.getProvider(emptyBoolQuery); + assertNull(boolProvider.getFilter(searchContext, emptyBoolQuery, compositeDataCubeFieldType)); + + // Test bool query with single must clause + KeywordFieldMapper.KeywordFieldType keywordFieldType = new KeywordFieldMapper.KeywordFieldType("keyword"); + when(mapperService.fieldType("keyword")).thenReturn(keywordFieldType); + + BoolQueryBuilder singleClauseBoolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("keyword", "value")); + StarTreeFilter singleClauseFilter = boolProvider.getFilter(searchContext, singleClauseBoolQuery, compositeDataCubeFieldType); + assertNotNull(singleClauseFilter); + assertFalse(singleClauseFilter.getFiltersForDimension("keyword").isEmpty()); + + // Test bool query with multiple must clauses + NumberFieldMapper.NumberFieldType statusFieldType = new NumberFieldMapper.NumberFieldType( + "status", + NumberFieldMapper.NumberType.INTEGER + ); + when(mapperService.fieldType("status")).thenReturn(statusFieldType); + + BoolQueryBuilder multiClauseBoolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("keyword", "value")) + .must(new RangeQueryBuilder("status").gte(200).lt(500)); + StarTreeFilter multiClauseFilter = boolProvider.getFilter(searchContext, multiClauseBoolQuery, compositeDataCubeFieldType); + assertNotNull(multiClauseFilter); + assertFalse(multiClauseFilter.getFiltersForDimension("keyword").isEmpty()); + assertFalse(multiClauseFilter.getFiltersForDimension("status").isEmpty()); + + // Test bool query with unsupported dimension + when(mapperService.fieldType("unsupported")).thenReturn(new WildcardFieldMapper.WildcardFieldType("unsupported")); + BoolQueryBuilder unsupportedBoolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("unsupported", "value")); + assertNull(boolProvider.getFilter(searchContext, unsupportedBoolQuery, compositeDataCubeFieldType)); + + // Test bool query with mixed valid and invalid clauses + BoolQueryBuilder mixedBoolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("keyword", "value")) + .must(new TermQueryBuilder("unsupported", "value")); + assertNull(boolProvider.getFilter(searchContext, mixedBoolQuery, compositeDataCubeFieldType)); + + // Test bool query with terms query + BoolQueryBuilder termsBoolQuery = new BoolQueryBuilder() + .must(new TermsQueryBuilder("keyword", List.of("value1", "value2"))); + StarTreeFilter termsFilter = boolProvider.getFilter(searchContext, termsBoolQuery, compositeDataCubeFieldType); + assertNotNull(termsFilter); + assertFalse(termsFilter.getFiltersForDimension("keyword").isEmpty()); + } + + public void testBoolStarTreeFilterProvider_ShouldClause() throws IOException { + CompositeDataCubeFieldType compositeDataCubeFieldType = new StarTreeMapper.StarTreeFieldType( + "star_tree", + new StarTreeField( + "star_tree", + List.of( + new OrdinalDimension("status"), + new OrdinalDimension("method") + ), + List.of(new Metric("field", List.of(MetricStat.MAX))), + new StarTreeFieldConfiguration( + randomIntBetween(1, 10_000), + Collections.emptySet(), + StarTreeFieldConfiguration.StarTreeBuildMode.ON_HEAP + ) + ) + ); + MapperService mapperService = mock(MapperService.class); + SearchContext searchContext = mock(SearchContext.class); + when(searchContext.mapperService()).thenReturn(mapperService); + + // Setup field types + NumberFieldMapper.NumberFieldType statusFieldType = new NumberFieldMapper.NumberFieldType( + "status", + NumberFieldMapper.NumberType.INTEGER + ); + KeywordFieldMapper.KeywordFieldType methodFieldType = new KeywordFieldMapper.KeywordFieldType("method"); + when(mapperService.fieldType("status")).thenReturn(statusFieldType); + when(mapperService.fieldType("method")).thenReturn(methodFieldType); + + StarTreeFilterProvider boolProvider = StarTreeFilterProvider.SingletonFactory.getProvider(new BoolQueryBuilder()); + + // Test valid SHOULD clause (same dimension) + BoolQueryBuilder validShouldQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("status", 404)); + StarTreeFilter validShouldFilter = boolProvider.getFilter(searchContext, validShouldQuery, compositeDataCubeFieldType); + assertNotNull("Should support OR within same dimension", validShouldFilter); + assertFalse(validShouldFilter.getFiltersForDimension("status").isEmpty()); + + // Test valid SHOULD clause with range + BoolQueryBuilder validRangeShouldQuery = new BoolQueryBuilder() + .should(new RangeQueryBuilder("status").gte(200).lt(300)) + .should(new RangeQueryBuilder("status").gte(400).lt(500)); + StarTreeFilter validRangeShouldFilter = boolProvider.getFilter( + searchContext, + validRangeShouldQuery, + compositeDataCubeFieldType + ); + assertNotNull("Should support OR of ranges within same dimension", validRangeShouldFilter); + assertFalse(validRangeShouldFilter.getFiltersForDimension("status").isEmpty()); + + // Test invalid SHOULD clause (different dimensions) + BoolQueryBuilder invalidShouldQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("method", "GET")); + StarTreeFilter invalidShouldFilter = boolProvider.getFilter(searchContext, invalidShouldQuery, compositeDataCubeFieldType); + assertNull("Should not support OR across different dimensions", invalidShouldFilter); + + // Test mixed MUST and SHOULD (same dimension) + BoolQueryBuilder mixedValidQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("method", "GET")) + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("status", 404)); + StarTreeFilter mixedValidFilter = boolProvider.getFilter(searchContext, mixedValidQuery, compositeDataCubeFieldType); + assertNotNull("Should support mixed MUST and SHOULD when SHOULD clauses are on same dimension", mixedValidFilter); + assertFalse(mixedValidFilter.getFiltersForDimension("method").isEmpty()); + assertFalse(mixedValidFilter.getFiltersForDimension("status").isEmpty()); + + // Test mixed MUST and SHOULD (different dimensions) + BoolQueryBuilder mixedInvalidQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("method", "GET")) + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("method", "POST")); + StarTreeFilter mixedInvalidFilter = boolProvider.getFilter(searchContext, mixedInvalidQuery, compositeDataCubeFieldType); + assertNull("Should not support mixed MUST and SHOULD when SHOULD clauses are on different dimensions", mixedInvalidFilter); + } + } From 9dbe1bf9fd2bc7f0a085f28128390ea6505abe53 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 17 Apr 2025 20:19:56 +0530 Subject: [PATCH 03/36] Handle nested bool queries with merging Signed-off-by: Rishab Nahata --- .../opensearch/common/util/FeatureFlags.java | 2 +- .../startree/filter/DimensionFilter.java | 7 + .../filter/DimensionFilterMerger.java | 223 ++++++++++++++++++ .../startree/filter/ExactMatchDimFilter.java | 9 + .../startree/filter/MatchNoneFilter.java | 11 + .../startree/filter/RangeMatchDimFilter.java | 22 ++ .../provider/DimensionFilterMapper.java | 4 +- .../provider/StarTreeFilterProvider.java | 117 ++++----- .../DimensionFilterAndMapperTests.java | 2 +- 9 files changed, 328 insertions(+), 69 deletions(-) create mode 100644 server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 4ff81cf0c1c96..9840e989e5378 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -96,7 +96,7 @@ public class FeatureFlags { * aggregations. */ public static final String STAR_TREE_INDEX = "opensearch.experimental.feature.composite_index.star_tree.enabled"; - public static final Setting STAR_TREE_INDEX_SETTING = Setting.boolSetting(STAR_TREE_INDEX, false, Property.NodeScope); + public static final Setting STAR_TREE_INDEX_SETTING = Setting.boolSetting(STAR_TREE_INDEX, true, Property.NodeScope); /** * Gates the functionality of application based configuration templates. diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java index 64f971a58f216..b7e59ad46139b 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java @@ -47,6 +47,11 @@ public void matchStarTreeNodes(StarTreeNode parentNode, StarTreeValues starTreeV public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return true; } + + @Override + public String getDimensionName() { + return ""; + } }; /** @@ -74,6 +79,8 @@ public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { */ boolean matchDimValue(long ordinal, StarTreeValues starTreeValues); + String getDimensionName(); + /** * Represents how to match a value when comparing during StarTreeTraversal */ diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java new file mode 100644 index 0000000000000..6ae51eed3138e --- /dev/null +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java @@ -0,0 +1,223 @@ +/* + * 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.search.startree.filter; + +import org.apache.lucene.util.BytesRef; + +import java.util.ArrayList; +import java.util.List; + +public class DimensionFilterMerger { + + /** + * Gets intersection of two DimensionFilters + * Returns null if intersection results in no possible matches. + */ + public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter filter2) { + if (filter1 == null || filter2 == null) { + return null; + } + + // Verify filters are for same dimension + if (!filter1.getDimensionName().equals(filter2.getDimensionName())) { + throw new IllegalArgumentException("Cannot intersect filters for different dimensions: " + + filter1.getDimensionName() + " and " + filter2.getDimensionName()); + } + + if (filter1 instanceof RangeMatchDimFilter && filter2 instanceof RangeMatchDimFilter) { + return intersectRangeFilters((RangeMatchDimFilter) filter1, (RangeMatchDimFilter) filter2); + } + + if (filter1 instanceof ExactMatchDimFilter && filter2 instanceof ExactMatchDimFilter) { + return intersectExactMatchFilters((ExactMatchDimFilter) filter1, (ExactMatchDimFilter) filter2); + } + + // Handle Range + ExactMatch combinations + if (filter1 instanceof RangeMatchDimFilter && filter2 instanceof ExactMatchDimFilter) { + return intersectRangeWithExactMatch((RangeMatchDimFilter) filter1, (ExactMatchDimFilter) filter2); + } + + if (filter1 instanceof ExactMatchDimFilter && filter2 instanceof RangeMatchDimFilter) { + return intersectRangeWithExactMatch((RangeMatchDimFilter) filter2, (ExactMatchDimFilter) filter1); + } + + throw new IllegalArgumentException("Unsupported filter combination: " + + filter1.getClass().getSimpleName() + " and " + filter2.getClass().getSimpleName()); + } + + /** + * Intersects two range filters. + * Returns null if ranges don't overlap. + */ + private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, RangeMatchDimFilter range2) { + Object low1 = range1.getLow(); + Object high1 = range1.getHigh(); + Object low2 = range2.getLow(); + Object high2 = range2.getHigh(); + + // Find the more restrictive bounds + Object newLow; + boolean includeLow; + if (low1 == null) { + newLow = low2; + includeLow = range2.isIncludeLow(); + } else if (low2 == null) { + newLow = low1; + includeLow = range1.isIncludeLow(); + } else { + int comparison = compareValues(low1, low2); + if (comparison > 0) { + newLow = low1; + includeLow = range1.isIncludeLow(); + } else if (comparison < 0) { + newLow = low2; + includeLow = range2.isIncludeLow(); + } else { + newLow = low1; + includeLow = range1.isIncludeLow() && range2.isIncludeLow(); + } + } + + Object newHigh; + boolean includeHigh; + if (high1 == null) { + newHigh = high2; + includeHigh = range2.isIncludeHigh(); + } else if (high2 == null) { + newHigh = high1; + includeHigh = range1.isIncludeHigh(); + } else { + int comparison = compareValues(high1, high2); + if (comparison < 0) { + newHigh = high1; + includeHigh = range1.isIncludeHigh(); + } else if (comparison > 0) { + newHigh = high2; + includeHigh = range2.isIncludeHigh(); + } else { + newHigh = high1; + includeHigh = range1.isIncludeHigh() && range2.isIncludeHigh(); + } + } + + // Check if range is valid + if (newLow != null && newHigh != null) { + int comparison = compareValues(newLow, newHigh); + if (comparison > 0 || (comparison == 0 && (!includeLow || !includeHigh))) { + return null; // No overlap + } + } + + return new RangeMatchDimFilter( + range1.getDimensionName(), + newLow, + newHigh, + includeLow, + includeHigh + ); + } + + /** + * Intersects two exact match filters. + * Returns null if no common values. + */ + private static DimensionFilter intersectExactMatchFilters(ExactMatchDimFilter exact1, ExactMatchDimFilter exact2) { + List values1 = exact1.getRawValues(); + List values2 = exact2.getRawValues(); + + List intersection = new ArrayList<>(); + for (Object value : values1) { + if (values2.contains(value)) { + intersection.add(value); + } + } + + if (intersection.isEmpty()) { + return null; + } + + return new ExactMatchDimFilter(exact1.getDimensionName(), intersection); + } + + /** + * Intersects a range filter with an exact match filter. + * Returns null if no values from exact match are within range. + */ + private static DimensionFilter intersectRangeWithExactMatch(RangeMatchDimFilter range, ExactMatchDimFilter exact) { + List validValues = new ArrayList<>(); + + for (Object value : exact.getRawValues()) { + if (isValueInRange(value, range)) { + validValues.add(value); + } + } + + if (validValues.isEmpty()) { + return null; + } + + return new ExactMatchDimFilter(exact.getDimensionName(), validValues); + } + + /** + * Checks if a value falls within a range. + */ + private static boolean isValueInRange(Object value, RangeMatchDimFilter range) { + if (range.getLow() != null) { + int comparison = compareValues(value, range.getLow()); + if (comparison < 0 || (comparison == 0 && !range.isIncludeLow())) { + return false; + } + } + + if (range.getHigh() != null) { + int comparison = compareValues(value, range.getHigh()); + if (comparison > 0 || (comparison == 0 && !range.isIncludeHigh())) { + return false; + } + } + + return true; + } + + /** + * Compares two values, handling different types appropriately. + */ + @SuppressWarnings("unchecked") + private static int compareValues(Object v1, Object v2) { + if (v1 instanceof Number && v2 instanceof Number) { + double d1 = ((Number) v1).doubleValue(); + double d2 = ((Number) v2).doubleValue(); + return Double.compare(d1, d2); + } + + if (v1 instanceof String && v2 instanceof String) { + return ((String) v1).compareTo((String) v2); + } + + if (v1 instanceof BytesRef && v2 instanceof BytesRef) { + return ((BytesRef) v1).compareTo((BytesRef) v2); + } + + // Convert BytesRef to String if needed + if (v1 instanceof BytesRef) { + v1 = ((BytesRef) v1).utf8ToString(); + } + if (v2 instanceof BytesRef) { + v2 = ((BytesRef) v2).utf8ToString(); + } + + if (v1 instanceof Comparable && v1.getClass().equals(v2.getClass())) { + return ((Comparable) v1).compareTo(v2); + } + + throw new IllegalArgumentException("Cannot compare values of types: " + + v1.getClass().getName() + " and " + v2.getClass().getName()); + } +} diff --git a/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java index 28ea261ca1e56..659b790ad8103 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java @@ -30,6 +30,10 @@ public class ExactMatchDimFilter implements DimensionFilter { private final String dimensionName; + public List getRawValues() { + return rawValues; + } + private final List rawValues; // Order is essential for successive binary search @@ -81,4 +85,9 @@ public void matchStarTreeNodes(StarTreeNode parentNode, StarTreeValues starTreeV public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return convertedOrdinals.contains(ordinal); } + + @Override + public String getDimensionName() { + return dimensionName; + } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java index 3066b4d7a8a3f..6682a61c2aa47 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java @@ -19,6 +19,12 @@ */ @ExperimentalApi public class MatchNoneFilter implements DimensionFilter { + + private final String dimensionName; + + public MatchNoneFilter(String dimensionName) { + this.dimensionName = dimensionName; + } @Override public void initialiseForSegment(StarTreeValues starTreeValues, SearchContext searchContext) { // Nothing to do as we won't match anything. @@ -33,4 +39,9 @@ public void matchStarTreeNodes(StarTreeNode parentNode, StarTreeValues starTreeV public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return false; } + + @Override + public String getDimensionName() { + return dimensionName; + } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java index fecf1a9ebf76b..a823fc6d88a53 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java @@ -27,6 +27,27 @@ public class RangeMatchDimFilter implements DimensionFilter { private final String dimensionName; + @Override + public String getDimensionName() { + return dimensionName; + } + + public Object getLow() { + return low; + } + + public Object getHigh() { + return high; + } + + public boolean isIncludeLow() { + return includeLow; + } + + public boolean isIncludeHigh() { + return includeHigh; + } + private final Object low; private final Object high; private final boolean includeLow; @@ -35,6 +56,7 @@ public class RangeMatchDimFilter implements DimensionFilter { private Long lowOrdinal; private Long highOrdinal; + // TODO - see if we need this while intersecting private boolean skipRangeCollection = false; public RangeMatchDimFilter(String dimensionName, Object low, Object high, boolean includeLow, boolean includeHigh) { diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java index 8afdb00864b22..4d3d6a8d2c193 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java @@ -164,14 +164,14 @@ public DimensionFilter getRangeMatchFilter( boolean lowerTermHasDecimalPart = hasDecimalPart(parsedLow); if ((lowerTermHasDecimalPart == false && includeLow == false) || (lowerTermHasDecimalPart && signum(parsedLow) > 0)) { if (parsedLow.equals(defaultMaximum())) { - return new MatchNoneFilter(); + return new MatchNoneFilter(mappedFieldType.name()); } ++parsedLow; } boolean upperTermHasDecimalPart = hasDecimalPart(parsedHigh); if ((upperTermHasDecimalPart == false && includeHigh == false) || (upperTermHasDecimalPart && signum(parsedHigh) < 0)) { if (parsedHigh.equals(defaultMinimum())) { - return new MatchNoneFilter(); + return new MatchNoneFilter(mappedFieldType.name()); } --parsedHigh; } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java index 700b795af2f64..3b97953f38359 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java @@ -8,6 +8,7 @@ package org.opensearch.search.startree.filter.provider; +import org.apache.lucene.util.BytesRef; import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.index.compositeindex.datacube.Dimension; import org.opensearch.index.mapper.CompositeDataCubeFieldType; @@ -21,14 +22,17 @@ import org.opensearch.search.internal.SearchContext; import org.opensearch.search.startree.StarTreeQueryHelper; import org.opensearch.search.startree.filter.DimensionFilter; +import org.opensearch.search.startree.filter.DimensionFilterMerger; import org.opensearch.search.startree.filter.StarTreeFilter; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * Converts a {@link QueryBuilder} into a {@link StarTreeFilter} by generating the appropriate @{@link org.opensearch.search.startree.filter.DimensionFilter} @@ -162,91 +166,74 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C } - /** - * - */ class BoolStarTreeFilterProvider implements StarTreeFilterProvider { @Override - public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) - throws IOException { + public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) throws IOException { BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) rawFilter; - if (boolQueryBuilder.hasClauses() == false) { - return null; // no clause present; fallback to default implementation + if (!boolQueryBuilder.hasClauses()) { + return null; } - Map> dimensionFilterMap = new HashMap<>(); - - List mustClauses = boolQueryBuilder.must(); - if (mustClauses.isEmpty() == false) { - for (QueryBuilder clause : mustClauses) { - StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); - if (provider == null) { - return null; // Unsupported query type; fallback - } - StarTreeFilter filter = provider.getFilter(context, clause, compositeFieldType); - if (filter == null) { - return null; // If any clause can't be converted, fallback - } + return processMustClauses(boolQueryBuilder.must(), context, compositeFieldType); + } - // Merge filters into dimensionFilterMap - for (String dim : filter.getDimensions()) { - dimensionFilterMap.computeIfAbsent(dim, k -> new ArrayList<>()).addAll(filter.getFiltersForDimension(dim)); - } - } + private StarTreeFilter processMustClauses(List mustClauses, SearchContext context, + CompositeDataCubeFieldType compositeFieldType) throws IOException { + if (mustClauses.isEmpty()) { + return null; } + Map> dimensionToFilters = new HashMap<>(); - List shouldClauses = boolQueryBuilder.should(); - if (shouldClauses.isEmpty() == false) { - // Group 'should' clauses by dimension - Map> shouldClausesByDimension = new HashMap<>(); + for (QueryBuilder clause : mustClauses) { + StarTreeFilter clauseFilter; - for (QueryBuilder clause : shouldClauses) { - String dimension = getDimensionFromQuery(clause); - if (dimension == null) { - return null; // Unsupported query type + if (clause instanceof BoolQueryBuilder) { + // Recursive processing for nested bool + clauseFilter = processMustClauses(((BoolQueryBuilder) clause).must(), context, compositeFieldType); + } else { + // Process individual clause + StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); + if (provider == null) { + return null; } - shouldClausesByDimension.computeIfAbsent(dimension, k -> new ArrayList<>()).add(clause); + clauseFilter = provider.getFilter(context, clause, compositeFieldType); } - // If SHOULD clauses span multiple dimensions, we don't support it - if (shouldClausesByDimension.size() > 1) { + + if (clauseFilter == null) { return null; } - // Process 'should' clauses for the single dimension - for (Map.Entry> entry : shouldClausesByDimension.entrySet()) { - String dimension = entry.getKey(); - List clausesForDimension = entry.getValue(); - - List orFilters = new ArrayList<>(); - for (QueryBuilder clause : clausesForDimension) { - StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); - if (provider == null) { - return null; + // Merge filters for each dimension + for (String dimension : clauseFilter.getDimensions()) { + List existingFilters = dimensionToFilters.get(dimension); + List newFilters = clauseFilter.getFiltersForDimension(dimension); + + if (existingFilters == null) { + dimensionToFilters.put(dimension, new ArrayList<>(newFilters)); + } else { + // Here's where we need the DimensionFilter merging logic + // For example: merging range with term, or range with range + DimensionFilter mergedFilter = mergeDimensionFilters(existingFilters, newFilters); + if (mergedFilter == null) { + return null; // No possible matches after merging } - - // TODO - nested boolean queries - StarTreeFilter filter = provider.getFilter(context, clause, compositeFieldType); - if (filter == null) { - return null; - } - - orFilters.addAll(filter.getFiltersForDimension(dimension)); + dimensionToFilters.put(dimension, Collections.singletonList(mergedFilter)); } - dimensionFilterMap.computeIfAbsent(dimension, k -> new ArrayList<>()).addAll(orFilters); } } - return new StarTreeFilter(dimensionFilterMap); + + return new StarTreeFilter(dimensionToFilters); } - private String getDimensionFromQuery(QueryBuilder query) { - if (query instanceof TermQueryBuilder) { - return ((TermQueryBuilder) query).fieldName(); - } else if (query instanceof TermsQueryBuilder) { - return ((TermsQueryBuilder) query).fieldName(); - } else if (query instanceof RangeQueryBuilder) { - return ((RangeQueryBuilder) query).fieldName(); - } - return null; + /** + * Merges multiple DimensionFilters for the same dimension. + * This is where we need to implement the actual merging logic. + */ + private DimensionFilter mergeDimensionFilters(List filters1, List filters2) { + DimensionFilter filter1 = filters1.getFirst(); + DimensionFilter filter2 = filters2.getFirst(); + + return DimensionFilterMerger.intersect(filter1, filter2); } } } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java index 26d860bc92848..187d099253f9f 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java @@ -185,7 +185,7 @@ public void testStarTreeFilterProviders() throws IOException { } // Testing MatchNoneFilter - DimensionFilter dimensionFilter = new MatchNoneFilter(); + DimensionFilter dimensionFilter = new MatchNoneFilter("random"); dimensionFilter.initialiseForSegment(null, null); ArrayBasedCollector collector = new ArrayBasedCollector(); assertFalse(dimensionFilter.matchDimValue(1, null)); From 1adb472f89d5de08b50c420f950af0a0ccf19e5e Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 17 Apr 2025 22:40:36 +0530 Subject: [PATCH 04/36] Remove test cases Signed-off-by: Rishab Nahata --- .../DimensionFilterAndMapperTests.java | 151 ------------------ 1 file changed, 151 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java index 187d099253f9f..ec684af5f9aec 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java @@ -192,155 +192,4 @@ public void testStarTreeFilterProviders() throws IOException { dimensionFilter.matchStarTreeNodes(null, null, collector); assertEquals(0, collector.collectedNodeCount()); } - - public void testBoolStarTreeFilterProvider_MustClause() throws IOException { - CompositeDataCubeFieldType compositeDataCubeFieldType = new StarTreeMapper.StarTreeFieldType( - "star_tree", - new StarTreeField( - "star_tree", - List.of( - new OrdinalDimension("keyword"), - new OrdinalDimension("status"), - new OrdinalDimension("method") - ), - List.of(new Metric("field", List.of(MetricStat.MAX))), - new StarTreeFieldConfiguration( - randomIntBetween(1, 10_000), - Collections.emptySet(), - StarTreeFieldConfiguration.StarTreeBuildMode.ON_HEAP - ) - ) - ); - MapperService mapperService = mock(MapperService.class); - SearchContext searchContext = mock(SearchContext.class); - when(searchContext.mapperService()).thenReturn(mapperService); - - // Test empty bool query - BoolQueryBuilder emptyBoolQuery = new BoolQueryBuilder(); - StarTreeFilterProvider boolProvider = StarTreeFilterProvider.SingletonFactory.getProvider(emptyBoolQuery); - assertNull(boolProvider.getFilter(searchContext, emptyBoolQuery, compositeDataCubeFieldType)); - - // Test bool query with single must clause - KeywordFieldMapper.KeywordFieldType keywordFieldType = new KeywordFieldMapper.KeywordFieldType("keyword"); - when(mapperService.fieldType("keyword")).thenReturn(keywordFieldType); - - BoolQueryBuilder singleClauseBoolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("keyword", "value")); - StarTreeFilter singleClauseFilter = boolProvider.getFilter(searchContext, singleClauseBoolQuery, compositeDataCubeFieldType); - assertNotNull(singleClauseFilter); - assertFalse(singleClauseFilter.getFiltersForDimension("keyword").isEmpty()); - - // Test bool query with multiple must clauses - NumberFieldMapper.NumberFieldType statusFieldType = new NumberFieldMapper.NumberFieldType( - "status", - NumberFieldMapper.NumberType.INTEGER - ); - when(mapperService.fieldType("status")).thenReturn(statusFieldType); - - BoolQueryBuilder multiClauseBoolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("keyword", "value")) - .must(new RangeQueryBuilder("status").gte(200).lt(500)); - StarTreeFilter multiClauseFilter = boolProvider.getFilter(searchContext, multiClauseBoolQuery, compositeDataCubeFieldType); - assertNotNull(multiClauseFilter); - assertFalse(multiClauseFilter.getFiltersForDimension("keyword").isEmpty()); - assertFalse(multiClauseFilter.getFiltersForDimension("status").isEmpty()); - - // Test bool query with unsupported dimension - when(mapperService.fieldType("unsupported")).thenReturn(new WildcardFieldMapper.WildcardFieldType("unsupported")); - BoolQueryBuilder unsupportedBoolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("unsupported", "value")); - assertNull(boolProvider.getFilter(searchContext, unsupportedBoolQuery, compositeDataCubeFieldType)); - - // Test bool query with mixed valid and invalid clauses - BoolQueryBuilder mixedBoolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("keyword", "value")) - .must(new TermQueryBuilder("unsupported", "value")); - assertNull(boolProvider.getFilter(searchContext, mixedBoolQuery, compositeDataCubeFieldType)); - - // Test bool query with terms query - BoolQueryBuilder termsBoolQuery = new BoolQueryBuilder() - .must(new TermsQueryBuilder("keyword", List.of("value1", "value2"))); - StarTreeFilter termsFilter = boolProvider.getFilter(searchContext, termsBoolQuery, compositeDataCubeFieldType); - assertNotNull(termsFilter); - assertFalse(termsFilter.getFiltersForDimension("keyword").isEmpty()); - } - - public void testBoolStarTreeFilterProvider_ShouldClause() throws IOException { - CompositeDataCubeFieldType compositeDataCubeFieldType = new StarTreeMapper.StarTreeFieldType( - "star_tree", - new StarTreeField( - "star_tree", - List.of( - new OrdinalDimension("status"), - new OrdinalDimension("method") - ), - List.of(new Metric("field", List.of(MetricStat.MAX))), - new StarTreeFieldConfiguration( - randomIntBetween(1, 10_000), - Collections.emptySet(), - StarTreeFieldConfiguration.StarTreeBuildMode.ON_HEAP - ) - ) - ); - MapperService mapperService = mock(MapperService.class); - SearchContext searchContext = mock(SearchContext.class); - when(searchContext.mapperService()).thenReturn(mapperService); - - // Setup field types - NumberFieldMapper.NumberFieldType statusFieldType = new NumberFieldMapper.NumberFieldType( - "status", - NumberFieldMapper.NumberType.INTEGER - ); - KeywordFieldMapper.KeywordFieldType methodFieldType = new KeywordFieldMapper.KeywordFieldType("method"); - when(mapperService.fieldType("status")).thenReturn(statusFieldType); - when(mapperService.fieldType("method")).thenReturn(methodFieldType); - - StarTreeFilterProvider boolProvider = StarTreeFilterProvider.SingletonFactory.getProvider(new BoolQueryBuilder()); - - // Test valid SHOULD clause (same dimension) - BoolQueryBuilder validShouldQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) - .should(new TermQueryBuilder("status", 404)); - StarTreeFilter validShouldFilter = boolProvider.getFilter(searchContext, validShouldQuery, compositeDataCubeFieldType); - assertNotNull("Should support OR within same dimension", validShouldFilter); - assertFalse(validShouldFilter.getFiltersForDimension("status").isEmpty()); - - // Test valid SHOULD clause with range - BoolQueryBuilder validRangeShouldQuery = new BoolQueryBuilder() - .should(new RangeQueryBuilder("status").gte(200).lt(300)) - .should(new RangeQueryBuilder("status").gte(400).lt(500)); - StarTreeFilter validRangeShouldFilter = boolProvider.getFilter( - searchContext, - validRangeShouldQuery, - compositeDataCubeFieldType - ); - assertNotNull("Should support OR of ranges within same dimension", validRangeShouldFilter); - assertFalse(validRangeShouldFilter.getFiltersForDimension("status").isEmpty()); - - // Test invalid SHOULD clause (different dimensions) - BoolQueryBuilder invalidShouldQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) - .should(new TermQueryBuilder("method", "GET")); - StarTreeFilter invalidShouldFilter = boolProvider.getFilter(searchContext, invalidShouldQuery, compositeDataCubeFieldType); - assertNull("Should not support OR across different dimensions", invalidShouldFilter); - - // Test mixed MUST and SHOULD (same dimension) - BoolQueryBuilder mixedValidQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("method", "GET")) - .should(new TermQueryBuilder("status", 200)) - .should(new TermQueryBuilder("status", 404)); - StarTreeFilter mixedValidFilter = boolProvider.getFilter(searchContext, mixedValidQuery, compositeDataCubeFieldType); - assertNotNull("Should support mixed MUST and SHOULD when SHOULD clauses are on same dimension", mixedValidFilter); - assertFalse(mixedValidFilter.getFiltersForDimension("method").isEmpty()); - assertFalse(mixedValidFilter.getFiltersForDimension("status").isEmpty()); - - // Test mixed MUST and SHOULD (different dimensions) - BoolQueryBuilder mixedInvalidQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("method", "GET")) - .should(new TermQueryBuilder("status", 200)) - .should(new TermQueryBuilder("method", "POST")); - StarTreeFilter mixedInvalidFilter = boolProvider.getFilter(searchContext, mixedInvalidQuery, compositeDataCubeFieldType); - assertNull("Should not support mixed MUST and SHOULD when SHOULD clauses are on different dimensions", mixedInvalidFilter); - } - } From 0d2253748d7824bb78cd5bc2611e6e49d73a9762 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Fri, 18 Apr 2025 06:04:17 +0530 Subject: [PATCH 05/36] Add should clause support Signed-off-by: Rishab Nahata --- .../provider/StarTreeFilterProvider.java | 126 +++++++++++++++--- 1 file changed, 109 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java index 3b97953f38359..a91d86759296d 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java @@ -174,7 +174,15 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C return null; } - return processMustClauses(boolQueryBuilder.must(), context, compositeFieldType); + if (boolQueryBuilder.must().isEmpty() == false) { + return processMustClauses(boolQueryBuilder.must(), context, compositeFieldType); + } + + if (boolQueryBuilder.should().isEmpty() == false) { + return processShouldClauses(boolQueryBuilder.should(), context, compositeFieldType); + } + + return null; } private StarTreeFilter processMustClauses(List mustClauses, SearchContext context, @@ -188,9 +196,24 @@ private StarTreeFilter processMustClauses(List mustClauses, Search StarTreeFilter clauseFilter; if (clause instanceof BoolQueryBuilder) { - // Recursive processing for nested bool - clauseFilter = processMustClauses(((BoolQueryBuilder) clause).must(), context, compositeFieldType); + BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; + if (boolClause.must().isEmpty() == false) { + // Process nested MUST + clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); + } else if (boolClause.should().isEmpty() == false) { + // Process SHOULD inside MUST + clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); + // Note: clauseFilter now contains all SHOULD conditions as separate filters + } else { + return null; + } } else { + // Only allow term, terms, and range queries +// if (!(clause instanceof TermQueryBuilder) && +// !(clause instanceof TermsQueryBuilder) && +// !(clause instanceof RangeQueryBuilder)) { +// return null; +// } // Process individual clause StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); if (provider == null) { @@ -209,15 +232,36 @@ private StarTreeFilter processMustClauses(List mustClauses, Search List newFilters = clauseFilter.getFiltersForDimension(dimension); if (existingFilters == null) { + // No existing filters for this dimension dimensionToFilters.put(dimension, new ArrayList<>(newFilters)); } else { - // Here's where we need the DimensionFilter merging logic - // For example: merging range with term, or range with range - DimensionFilter mergedFilter = mergeDimensionFilters(existingFilters, newFilters); - if (mergedFilter == null) { - return null; // No possible matches after merging + // We have existing filters for this dimension + if (newFilters.size() > 1) { + // New filters are from SHOULD clause (multiple filters = OR condition) + // Need to intersect each SHOULD filter with existing filters + List intersectedFilters = new ArrayList<>(); + for (DimensionFilter shouldFilter : newFilters) { + for (DimensionFilter existingFilter : existingFilters) { + DimensionFilter intersected = DimensionFilterMerger.intersect(existingFilter, shouldFilter); + if (intersected != null) { + intersectedFilters.add(intersected); + } + } + } + if (intersectedFilters.isEmpty()) { + return null; // No valid intersections + } + dimensionToFilters.put(dimension, intersectedFilters); + } else { + // Here's where we need the DimensionFilter merging logic + // For example: merging range with term, or range with range + // And a single dimension filter coming from should clause is as good as must clause + DimensionFilter mergedFilter = DimensionFilterMerger.intersect(existingFilters.getFirst(), newFilters.getFirst()); + if (mergedFilter == null) { + return null; // No possible matches after merging + } + dimensionToFilters.put(dimension, Collections.singletonList(mergedFilter)); } - dimensionToFilters.put(dimension, Collections.singletonList(mergedFilter)); } } } @@ -225,15 +269,63 @@ private StarTreeFilter processMustClauses(List mustClauses, Search return new StarTreeFilter(dimensionToFilters); } - /** - * Merges multiple DimensionFilters for the same dimension. - * This is where we need to implement the actual merging logic. - */ - private DimensionFilter mergeDimensionFilters(List filters1, List filters2) { - DimensionFilter filter1 = filters1.getFirst(); - DimensionFilter filter2 = filters2.getFirst(); + private StarTreeFilter processShouldClauses(List shouldClauses, SearchContext context, + CompositeDataCubeFieldType compositeFieldType) throws IOException { + if (shouldClauses.isEmpty()) { + return null; + } - return DimensionFilterMerger.intersect(filter1, filter2); + // First, validate all SHOULD clauses are for same dimension + String commonDimension = null; + Map> dimensionToFilters = new HashMap<>(); + for (QueryBuilder clause : shouldClauses) { + StarTreeFilter clauseFilter; + + if (clause instanceof BoolQueryBuilder) { + BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; + + // Reject if contains MUST clauses + if (boolClause.must().isEmpty() == false) { + return null; // Cannot have MUST inside SHOULD + } + + // Process nested SHOULD + clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); + } else { + // Only allow term, terms, and range queries + if (!(clause instanceof TermQueryBuilder) && + !(clause instanceof TermsQueryBuilder) && + !(clause instanceof RangeQueryBuilder)) { + return null; + } + + StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); + if (provider == null) { + return null; + } + clauseFilter = provider.getFilter(context, clause, compositeFieldType); + } + + if (clauseFilter == null) { + return null; + } + + // Validate single dimension + if (clauseFilter.getDimensions().size() != 1) { + return null; // SHOULD clause must operate on single dimension + } + + String dimension = clauseFilter.getDimensions().iterator().next(); + if (commonDimension == null) { + commonDimension = dimension; + } else if (!commonDimension.equals(dimension)) { + return null; // All SHOULD clauses must operate on same dimension + } + + // Simply collect all filters - StarTreeTraversal will handle OR operation + dimensionToFilters.computeIfAbsent(dimension, k -> new ArrayList<>()).addAll(clauseFilter.getFiltersForDimension(dimension)); + } + return new StarTreeFilter(dimensionToFilters); } } } From 308c3c9b7d61a1689e9f1d3529580638d74d04f0 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sat, 19 Apr 2025 02:39:15 +0530 Subject: [PATCH 06/36] Add tests Signed-off-by: Rishab Nahata --- .../filter/DimensionFilterMerger.java | 23 +- .../startree/filter/MatchNoneFilter.java | 1 + .../provider/StarTreeFilterProvider.java | 44 ++-- .../DimensionFilterAndMapperTests.java | 3 - .../startree/DimensionFilterMergerTests.java | 229 ++++++++++++++++++ 5 files changed, 266 insertions(+), 34 deletions(-) create mode 100644 server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java index 6ae51eed3138e..56c827a030af4 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java @@ -26,8 +26,9 @@ public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter // Verify filters are for same dimension if (!filter1.getDimensionName().equals(filter2.getDimensionName())) { - throw new IllegalArgumentException("Cannot intersect filters for different dimensions: " + - filter1.getDimensionName() + " and " + filter2.getDimensionName()); + throw new IllegalArgumentException( + "Cannot intersect filters for different dimensions: " + filter1.getDimensionName() + " and " + filter2.getDimensionName() + ); } if (filter1 instanceof RangeMatchDimFilter && filter2 instanceof RangeMatchDimFilter) { @@ -47,8 +48,9 @@ public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter return intersectRangeWithExactMatch((RangeMatchDimFilter) filter2, (ExactMatchDimFilter) filter1); } - throw new IllegalArgumentException("Unsupported filter combination: " + - filter1.getClass().getSimpleName() + " and " + filter2.getClass().getSimpleName()); + throw new IllegalArgumentException( + "Unsupported filter combination: " + filter1.getClass().getSimpleName() + " and " + filter2.getClass().getSimpleName() + ); } /** @@ -114,13 +116,7 @@ private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, } } - return new RangeMatchDimFilter( - range1.getDimensionName(), - newLow, - newHigh, - includeLow, - includeHigh - ); + return new RangeMatchDimFilter(range1.getDimensionName(), newLow, newHigh, includeLow, includeHigh); } /** @@ -217,7 +213,8 @@ private static int compareValues(Object v1, Object v2) { return ((Comparable) v1).compareTo(v2); } - throw new IllegalArgumentException("Cannot compare values of types: " + - v1.getClass().getName() + " and " + v2.getClass().getName()); + throw new IllegalArgumentException( + "Cannot compare values of types: " + v1.getClass().getName() + " and " + v2.getClass().getName() + ); } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java index 6682a61c2aa47..df1312f317012 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java @@ -25,6 +25,7 @@ public class MatchNoneFilter implements DimensionFilter { public MatchNoneFilter(String dimensionName) { this.dimensionName = dimensionName; } + @Override public void initialiseForSegment(StarTreeValues starTreeValues, SearchContext searchContext) { // Nothing to do as we won't match anything. diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java index a91d86759296d..fe2f748275a01 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java @@ -8,7 +8,6 @@ package org.opensearch.search.startree.filter.provider; -import org.apache.lucene.util.BytesRef; import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.index.compositeindex.datacube.Dimension; import org.opensearch.index.mapper.CompositeDataCubeFieldType; @@ -29,10 +28,8 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; /** * Converts a {@link QueryBuilder} into a {@link StarTreeFilter} by generating the appropriate @{@link org.opensearch.search.startree.filter.DimensionFilter} @@ -168,7 +165,8 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C class BoolStarTreeFilterProvider implements StarTreeFilterProvider { @Override - public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) throws IOException { + public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) + throws IOException { BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) rawFilter; if (!boolQueryBuilder.hasClauses()) { return null; @@ -185,8 +183,11 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C return null; } - private StarTreeFilter processMustClauses(List mustClauses, SearchContext context, - CompositeDataCubeFieldType compositeFieldType) throws IOException { + private StarTreeFilter processMustClauses( + List mustClauses, + SearchContext context, + CompositeDataCubeFieldType compositeFieldType + ) throws IOException { if (mustClauses.isEmpty()) { return null; } @@ -209,11 +210,11 @@ private StarTreeFilter processMustClauses(List mustClauses, Search } } else { // Only allow term, terms, and range queries -// if (!(clause instanceof TermQueryBuilder) && -// !(clause instanceof TermsQueryBuilder) && -// !(clause instanceof RangeQueryBuilder)) { -// return null; -// } + // if (!(clause instanceof TermQueryBuilder) && + // !(clause instanceof TermsQueryBuilder) && + // !(clause instanceof RangeQueryBuilder)) { + // return null; + // } // Process individual clause StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); if (provider == null) { @@ -256,7 +257,10 @@ private StarTreeFilter processMustClauses(List mustClauses, Search // Here's where we need the DimensionFilter merging logic // For example: merging range with term, or range with range // And a single dimension filter coming from should clause is as good as must clause - DimensionFilter mergedFilter = DimensionFilterMerger.intersect(existingFilters.getFirst(), newFilters.getFirst()); + DimensionFilter mergedFilter = DimensionFilterMerger.intersect( + existingFilters.getFirst(), + newFilters.getFirst() + ); if (mergedFilter == null) { return null; // No possible matches after merging } @@ -269,8 +273,11 @@ private StarTreeFilter processMustClauses(List mustClauses, Search return new StarTreeFilter(dimensionToFilters); } - private StarTreeFilter processShouldClauses(List shouldClauses, SearchContext context, - CompositeDataCubeFieldType compositeFieldType) throws IOException { + private StarTreeFilter processShouldClauses( + List shouldClauses, + SearchContext context, + CompositeDataCubeFieldType compositeFieldType + ) throws IOException { if (shouldClauses.isEmpty()) { return null; } @@ -293,9 +300,9 @@ private StarTreeFilter processShouldClauses(List shouldClauses, Se clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); } else { // Only allow term, terms, and range queries - if (!(clause instanceof TermQueryBuilder) && - !(clause instanceof TermsQueryBuilder) && - !(clause instanceof RangeQueryBuilder)) { + if (!(clause instanceof TermQueryBuilder) + && !(clause instanceof TermsQueryBuilder) + && !(clause instanceof RangeQueryBuilder)) { return null; } @@ -323,7 +330,8 @@ private StarTreeFilter processShouldClauses(List shouldClauses, Se } // Simply collect all filters - StarTreeTraversal will handle OR operation - dimensionToFilters.computeIfAbsent(dimension, k -> new ArrayList<>()).addAll(clauseFilter.getFiltersForDimension(dimension)); + dimensionToFilters.computeIfAbsent(dimension, k -> new ArrayList<>()) + .addAll(clauseFilter.getFiltersForDimension(dimension)); } return new StarTreeFilter(dimensionToFilters); } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java index ec684af5f9aec..2198433a7a066 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java @@ -20,10 +20,8 @@ import org.opensearch.index.mapper.CompositeDataCubeFieldType; import org.opensearch.index.mapper.KeywordFieldMapper; import org.opensearch.index.mapper.MapperService; -import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.index.mapper.StarTreeMapper; import org.opensearch.index.mapper.WildcardFieldMapper; -import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.RangeQueryBuilder; import org.opensearch.index.query.TermQueryBuilder; @@ -32,7 +30,6 @@ import org.opensearch.search.startree.filter.DimensionFilter; import org.opensearch.search.startree.filter.DimensionFilter.MatchType; import org.opensearch.search.startree.filter.MatchNoneFilter; -import org.opensearch.search.startree.filter.StarTreeFilter; import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import org.opensearch.search.startree.filter.provider.StarTreeFilterProvider; import org.opensearch.test.OpenSearchTestCase; diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java new file mode 100644 index 0000000000000..27a2838772b05 --- /dev/null +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java @@ -0,0 +1,229 @@ +/* + * 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.search.aggregations.startree; + +import org.opensearch.search.startree.filter.DimensionFilter; +import org.opensearch.search.startree.filter.DimensionFilterMerger; +import org.opensearch.search.startree.filter.ExactMatchDimFilter; +import org.opensearch.search.startree.filter.RangeMatchDimFilter; +import org.opensearch.test.OpenSearchTestCase; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; + +public class DimensionFilterMergerTests extends OpenSearchTestCase { + + public void testRangeIntersection() { + // Basic range intersection + assertRangeIntersection( + range("status", 200, 500, true, true), + range("status", 300, 400, true, true), + range("status", 300, 400, true, true) + ); + + // Boundary conditions + assertRangeIntersection( + range("status", 200, 200, true, true), + range("status", 200, 200, true, true), + range("status", 200, 200, true, true) + ); + + // Inclusive/Exclusive boundaries + assertRangeIntersection( + range("status", 200, 300, true, false), + range("status", 200, 300, false, true), + range("status", 200, 300, false, false) + ); + + // Non-overlapping ranges + assertNoIntersection(range("status", 200, 300, true, true), range("status", 301, 400, true, true)); + + // Exactly touching ranges (no overlap) + assertNoIntersection(range("status", 200, 300, true, false), range("status", 300, 400, true, true)); + + // Null bounds (unbounded ranges) + assertRangeIntersection( + range("status", null, 500, true, true), + range("status", 200, null, true, true), + range("status", 200, 500, true, true) + ); + + // Single point overlap + assertRangeIntersection( + range("status", 200, 300, true, true), + range("status", 300, 400, true, true), + range("status", 300, 300, true, true) + ); + + // Very large ranges + assertRangeIntersection( + range("status", Long.MIN_VALUE, Long.MAX_VALUE, true, true), + range("status", 200, 300, true, true), + range("status", 200, 300, true, true) + ); + + // Zero-width ranges + assertNoIntersection(range("status", 200, 200, true, true), range("status", 200, 200, false, false)); + + // Floating point precision + assertRangeIntersection( + range("latency", 1.0, 2.0, true, true), + range("latency", 2.0, 3.0, true, true), + range("latency", 2.0, 2.0, true, true) + ); + + // Different numeric types + assertRangeIntersection( + range("size", 1.0f, 2.0f, true, true), + range("size", 1L, 2L, true, true), + range("size", 1.0f, 2.0f, true, true) + ); + + // String to numeric conversion + assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMerger.intersect(range("status", "200", "300", true, true), range("status", 200, 300, true, true)) + ); + } + + public void testExactMatchIntersection() { + // Single value intersection + assertExactMatchIntersection( + exactMatch("status", List.of(200)), + exactMatch("status", List.of(200)), + exactMatch("status", List.of(200)) + ); + + // Multiple values intersection + assertExactMatchIntersection( + exactMatch("status", Arrays.asList(200, 300, 400)), + exactMatch("status", Arrays.asList(300, 400, 500)), + exactMatch("status", Arrays.asList(300, 400)) + ); + + // No intersection + assertNoIntersection(exactMatch("status", List.of(200)), exactMatch("status", List.of(300))); + + // Empty list + assertNoIntersection(exactMatch("status", Collections.emptyList()), exactMatch("status", List.of(200))); + + // Duplicate values + assertExactMatchIntersection( + exactMatch("status", Arrays.asList(200, 200, 300)), + exactMatch("status", Arrays.asList(200, 300, 300)), + exactMatch("status", Arrays.asList(200, 300)) + ); + + // Special characters in string values + assertExactMatchIntersection( + exactMatch("method", Arrays.asList("GET", "GET*")), + exactMatch("method", Arrays.asList("GET", "GET/")), + exactMatch("method", List.of("GET")) + ); + + // Case sensitivity + assertNoIntersection(exactMatch("method", Arrays.asList("GET", "Post")), exactMatch("method", Arrays.asList("get", "POST"))); + } + + public void testRangeExactMatchIntersection() { + // Value in range + assertRangeExactMatchIntersection( + range("status", 200, 300, true, true), + exactMatch("status", List.of(250)), + exactMatch("status", List.of(250)) + ); + + // Value at range boundaries + assertRangeExactMatchIntersection( + range("status", 200, 300, true, true), + exactMatch("status", Arrays.asList(200, 300)), + exactMatch("status", Arrays.asList(200, 300)) + ); + + // Value at exclusive boundaries + assertRangeExactMatchIntersection( + range("status", 200, 300, false, false), + exactMatch("status", Arrays.asList(201, 299)), + exactMatch("status", Arrays.asList(201, 299)) + ); + + // No values in range + assertNoIntersection(range("status", 200, 300, true, true), exactMatch("status", Arrays.asList(199, 301))); + + // Multiple values, some in range + assertRangeExactMatchIntersection( + range("status", 200, 300, true, true), + exactMatch("status", Arrays.asList(199, 200, 250, 300, 301)), + exactMatch("status", Arrays.asList(200, 250, 300)) + ); + } + + public void testDifferentDimensions() { + // Cannot intersect different dimensions + assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMerger.intersect(range("status", 200, 300, true, true), range("port", 80, 443, true, true)) + ); + } + + public void testTypeConversion() { + // Integer and Long + assertRangeIntersection( + range("port", 80, 443, true, true), + range("port", 80L, 443L, true, true), + range("port", 80, 443, true, true) + ); + + // Double and Integer + assertRangeIntersection( + range("latency", 1.0, 2.0, true, true), + range("latency", 1, 2, true, true), + range("latency", 1.0, 2.0, true, true) + ); + } + + // Helper methods + private RangeMatchDimFilter range(String dimension, Object low, Object high, boolean includeLow, boolean includeHigh) { + return new RangeMatchDimFilter(dimension, low, high, includeLow, includeHigh); + } + + private ExactMatchDimFilter exactMatch(String dimension, List values) { + return new ExactMatchDimFilter(dimension, values); + } + + private void assertRangeIntersection(RangeMatchDimFilter filter1, RangeMatchDimFilter filter2, RangeMatchDimFilter expected) { + DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2); + assertTrue(result instanceof RangeMatchDimFilter); + RangeMatchDimFilter rangeResult = (RangeMatchDimFilter) result; + assertEquals(expected.getLow(), rangeResult.getLow()); + assertEquals(expected.getHigh(), rangeResult.getHigh()); + assertEquals(expected.isIncludeLow(), rangeResult.isIncludeLow()); + assertEquals(expected.isIncludeHigh(), rangeResult.isIncludeHigh()); + } + + private void assertExactMatchIntersection(ExactMatchDimFilter filter1, ExactMatchDimFilter filter2, ExactMatchDimFilter expected) { + DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2); + assertTrue(result instanceof ExactMatchDimFilter); + ExactMatchDimFilter exactResult = (ExactMatchDimFilter) result; + assertEquals(new HashSet<>(expected.getRawValues()), new HashSet<>(exactResult.getRawValues())); + } + + private void assertRangeExactMatchIntersection(RangeMatchDimFilter range, ExactMatchDimFilter exact, ExactMatchDimFilter expected) { + DimensionFilter result = DimensionFilterMerger.intersect(range, exact); + assertTrue(result instanceof ExactMatchDimFilter); + ExactMatchDimFilter exactResult = (ExactMatchDimFilter) result; + assertEquals(new HashSet<>(expected.getRawValues()), new HashSet<>(exactResult.getRawValues())); + } + + private void assertNoIntersection(DimensionFilter filter1, DimensionFilter filter2) { + assertNull(DimensionFilterMerger.intersect(filter1, filter2)); + } +} From d766111d1a7a74d672d21ee71409cd48ae3cad30 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sat, 19 Apr 2025 03:34:08 +0530 Subject: [PATCH 07/36] Add tests Signed-off-by: Rishab Nahata --- .../DimensionFilterAndMapperTests.java | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java index 2198433a7a066..812254baf4c1a 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java @@ -20,8 +20,10 @@ import org.opensearch.index.mapper.CompositeDataCubeFieldType; import org.opensearch.index.mapper.KeywordFieldMapper; import org.opensearch.index.mapper.MapperService; +import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.index.mapper.StarTreeMapper; import org.opensearch.index.mapper.WildcardFieldMapper; +import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.RangeQueryBuilder; import org.opensearch.index.query.TermQueryBuilder; @@ -30,6 +32,7 @@ import org.opensearch.search.startree.filter.DimensionFilter; import org.opensearch.search.startree.filter.DimensionFilter.MatchType; import org.opensearch.search.startree.filter.MatchNoneFilter; +import org.opensearch.search.startree.filter.StarTreeFilter; import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import org.opensearch.search.startree.filter.provider.StarTreeFilterProvider; import org.opensearch.test.OpenSearchTestCase; @@ -132,7 +135,7 @@ public void testStarTreeFilterProviders() throws IOException { "star_tree", new StarTreeField( "star_tree", - List.of(new OrdinalDimension("keyword")), + List.of(new OrdinalDimension("keyword"), new OrdinalDimension("status"), new OrdinalDimension("method")), List.of(new Metric("field", List.of(MetricStat.MAX))), new StarTreeFieldConfiguration( randomIntBetween(1, 10_000), @@ -188,5 +191,48 @@ public void testStarTreeFilterProviders() throws IOException { assertFalse(dimensionFilter.matchDimValue(1, null)); dimensionFilter.matchStarTreeNodes(null, null, collector); assertEquals(0, collector.collectedNodeCount()); + + // Setup common field types + KeywordFieldMapper.KeywordFieldType methodType = new KeywordFieldMapper.KeywordFieldType("method"); + NumberFieldMapper.NumberFieldType statusType = new NumberFieldMapper.NumberFieldType( + "status", + NumberFieldMapper.NumberType.INTEGER + ); + when(mapperService.fieldType("method")).thenReturn(methodType); + when(mapperService.fieldType("status")).thenReturn(statusType); + + // Test simple MUST clause + BoolQueryBuilder simpleMustQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new TermQueryBuilder("status", 200)); + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(simpleMustQuery); + StarTreeFilter filter = provider.getFilter(searchContext, simpleMustQuery, compositeDataCubeFieldType); + assertNotNull(filter); + assertEquals(2, filter.getDimensions().size()); + assertTrue(filter.getDimensions().contains("method")); + assertTrue(filter.getDimensions().contains("status")); + + // Test MUST with nested SHOULD on different dimension + BoolQueryBuilder mustWithShould = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)).should(new TermQueryBuilder("status", 404))); + filter = provider.getFilter(searchContext, mustWithShould, compositeDataCubeFieldType); + assertNotNull(filter); + assertEquals(2, filter.getDimensions().size()); + assertEquals(1, filter.getFiltersForDimension("method").size()); + assertEquals(2, filter.getFiltersForDimension("status").size()); + + // Test invalid SHOULD across dimensions + BoolQueryBuilder invalidShould = new BoolQueryBuilder().should(new TermQueryBuilder("method", "GET")) + .should(new TermQueryBuilder("status", 200)); + assertNull(provider.getFilter(searchContext, invalidShould, compositeDataCubeFieldType)); + + // Test MUST with invalid nested SHOULD + BoolQueryBuilder invalidNestedShould = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new BoolQueryBuilder().should(new TermQueryBuilder("method", "POST")).should(new TermQueryBuilder("status", 200))); + assertNull(provider.getFilter(searchContext, invalidNestedShould, compositeDataCubeFieldType)); + + // Test MUST with SHOULD on same dimension + BoolQueryBuilder mustWithSameDimShould = new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)) + .must(new BoolQueryBuilder().should(new TermQueryBuilder("status", 404)).should(new TermQueryBuilder("status", 500))); + assertNull(provider.getFilter(searchContext, mustWithSameDimShould, compositeDataCubeFieldType)); } } From ac34a730ff583beeb883cc29851d5204c73299fe Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sat, 19 Apr 2025 04:03:41 +0530 Subject: [PATCH 08/36] Add test Signed-off-by: Rishab Nahata --- .../startree/StarTreeFilterTests.java | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java index 7282b0fafb8aa..38c5bc1e1c466 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java @@ -50,6 +50,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; @@ -480,4 +481,45 @@ public static XContentBuilder getExpandedMapping( b.endObject(); }); } + + public void testStarTreeFilterWithBoolQueries() throws IOException { + List docs = new ArrayList<>(); + Directory directory = createStarTreeIndex(5, true, docs); + DirectoryReader ir = DirectoryReader.open(directory); + initValuesSourceRegistry(); + LeafReaderContext context = ir.leaves().get(0); + SegmentReader reader = Lucene.segmentReader(context.reader()); + CompositeIndexReader starTreeDocValuesReader = (CompositeIndexReader) reader.getDocValuesReader(); + + MapperService mapperService = Mockito.mock(MapperService.class); + SearchContext searchContext = Mockito.mock(SearchContext.class); + Mockito.when(searchContext.mapperService()).thenReturn(mapperService); + Mockito.when(mapperService.fieldType(SNDV)) + .thenReturn(new NumberFieldMapper.NumberFieldType(SNDV, NumberFieldMapper.NumberType.INTEGER)); + Mockito.when(mapperService.fieldType(DV)) + .thenReturn(new NumberFieldMapper.NumberFieldType(DV, NumberFieldMapper.NumberType.INTEGER)); + + // Test 'MUST' clause + StarTreeFilter mustFilter = new StarTreeFilter(Map.of( + SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(0L))), + DV, List.of(new ExactMatchDimFilter(DV, List.of(0L))) + )); + long starTreeDocCount = getDocCountFromStarTree(starTreeDocValuesReader, mustFilter, context, searchContext); + long docCount = getDocCount(docs, Map.of(SNDV, 0L, DV, 0L)); + assertEquals(docCount, starTreeDocCount); + + // Test 'SHOULD' clause (same dimension) + StarTreeFilter shouldFilter = new StarTreeFilter(Map.of( + SNDV, Arrays.asList( + new ExactMatchDimFilter(SNDV, List.of(0L)), + new ExactMatchDimFilter(SNDV, List.of(1L)) + ) + )); + starTreeDocCount = getDocCountFromStarTree(starTreeDocValuesReader, shouldFilter, context, searchContext); + docCount = getDocCount(docs, Map.of(SNDV, 0L)) + getDocCount(docs, Map.of(SNDV, 1L)); + assertEquals(docCount, starTreeDocCount); + + ir.close(); + directory.close(); + } } From 5a0ecf148c29a1eb92ccabddab14b3d7d5424b02 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sat, 19 Apr 2025 04:16:27 +0530 Subject: [PATCH 09/36] Add tests Signed-off-by: Rishab Nahata --- .../startree/MetricAggregatorTests.java | 23 ++++++++++++++++++- .../startree/StarTreeFilterTests.java | 16 +++++-------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java index 6e10562c3a846..237dd62497537 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/MetricAggregatorTests.java @@ -52,6 +52,7 @@ import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.mapper.MapperService; import org.opensearch.index.mapper.NumberFieldMapper; +import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryShardContext; import org.opensearch.index.query.RangeQueryBuilder; @@ -245,7 +246,9 @@ private void testStarTreeDocValuesInternal(Codec codec, List for (int cases = 0; cases < 15; cases++) { // Get all types of queries (Term/Terms/Range) for all the given dimensions. List allFieldQueries = dimensionFieldData.stream() - .flatMap(x -> Stream.of(x.getTermQueryBuilder(), x.getTermsQueryBuilder(), x.getRangeQueryBuilder())) + .flatMap( + x -> Stream.of(x.getTermQueryBuilder(), x.getTermsQueryBuilder(), x.getRangeQueryBuilder(), x.getBoolQueryBuilder()) + ) .toList(); for (QueryBuilder qb : allFieldQueries) { @@ -547,6 +550,24 @@ public QueryBuilder getRangeQueryBuilder() { .includeUpper(randomBoolean()); } + public QueryBuilder getBoolQueryBuilder() { + // MUST only + BoolQueryBuilder mustOnly = new BoolQueryBuilder().must(getTermQueryBuilder()).must(getRangeQueryBuilder()); + + // MUST with nested SHOULD on same dimension + BoolQueryBuilder mustWithShould = new BoolQueryBuilder().must(getTermQueryBuilder()) + .must( + new BoolQueryBuilder().should(new TermQueryBuilder(fieldName, valueSupplier.get())) + .should(new TermQueryBuilder(fieldName, valueSupplier.get())) + ); + + // SHOULD only on same dimension + BoolQueryBuilder shouldOnly = new BoolQueryBuilder().should(new TermQueryBuilder(fieldName, valueSupplier.get())) + .should(new RangeQueryBuilder(fieldName).from(valueSupplier.get()).to(valueSupplier.get())); + + return randomFrom(mustOnly, mustWithShould, shouldOnly); + } + public String getFieldType() { return fieldType; } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java index 38c5bc1e1c466..050dbe7152774 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeFilterTests.java @@ -500,21 +500,17 @@ public void testStarTreeFilterWithBoolQueries() throws IOException { .thenReturn(new NumberFieldMapper.NumberFieldType(DV, NumberFieldMapper.NumberType.INTEGER)); // Test 'MUST' clause - StarTreeFilter mustFilter = new StarTreeFilter(Map.of( - SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(0L))), - DV, List.of(new ExactMatchDimFilter(DV, List.of(0L))) - )); + StarTreeFilter mustFilter = new StarTreeFilter( + Map.of(SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(0L))), DV, List.of(new ExactMatchDimFilter(DV, List.of(0L)))) + ); long starTreeDocCount = getDocCountFromStarTree(starTreeDocValuesReader, mustFilter, context, searchContext); long docCount = getDocCount(docs, Map.of(SNDV, 0L, DV, 0L)); assertEquals(docCount, starTreeDocCount); // Test 'SHOULD' clause (same dimension) - StarTreeFilter shouldFilter = new StarTreeFilter(Map.of( - SNDV, Arrays.asList( - new ExactMatchDimFilter(SNDV, List.of(0L)), - new ExactMatchDimFilter(SNDV, List.of(1L)) - ) - )); + StarTreeFilter shouldFilter = new StarTreeFilter( + Map.of(SNDV, Arrays.asList(new ExactMatchDimFilter(SNDV, List.of(0L)), new ExactMatchDimFilter(SNDV, List.of(1L)))) + ); starTreeDocCount = getDocCountFromStarTree(starTreeDocValuesReader, shouldFilter, context, searchContext); docCount = getDocCount(docs, Map.of(SNDV, 0L)) + getDocCount(docs, Map.of(SNDV, 1L)); assertEquals(docCount, starTreeDocCount); From ae760351758210df073047bb554d3eb749ec6060 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sun, 20 Apr 2025 19:29:01 +0530 Subject: [PATCH 10/36] Tests for basic must clause Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 152 ++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java new file mode 100644 index 0000000000000..9a38db75a4b50 --- /dev/null +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -0,0 +1,152 @@ +/* + * 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.search.aggregations.startree; + +import org.opensearch.index.compositeindex.datacube.Metric; +import org.opensearch.index.compositeindex.datacube.MetricStat; +import org.opensearch.index.compositeindex.datacube.OrdinalDimension; +import org.opensearch.index.compositeindex.datacube.startree.StarTreeField; +import org.opensearch.index.compositeindex.datacube.startree.StarTreeFieldConfiguration; +import org.opensearch.index.mapper.CompositeDataCubeFieldType; +import org.opensearch.index.mapper.KeywordFieldMapper; +import org.opensearch.index.mapper.MapperService; +import org.opensearch.index.mapper.NumberFieldMapper; +import org.opensearch.index.mapper.StarTreeMapper; +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.RangeQueryBuilder; +import org.opensearch.index.query.TermQueryBuilder; +import org.opensearch.index.query.TermsQueryBuilder; +import org.opensearch.search.internal.SearchContext; +import org.opensearch.search.startree.filter.DimensionFilter; +import org.opensearch.search.startree.filter.ExactMatchDimFilter; +import org.opensearch.search.startree.filter.RangeMatchDimFilter; +import org.opensearch.search.startree.filter.StarTreeFilter; +import org.opensearch.search.startree.filter.provider.StarTreeFilterProvider; +import org.opensearch.test.OpenSearchTestCase; +import org.junit.Before; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Set; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class BoolStarTreeFilterProviderTests extends OpenSearchTestCase { + private SearchContext searchContext; + private MapperService mapperService; + private CompositeDataCubeFieldType compositeFieldType; + + @Before + public void setup() { + // Setup common test dependencies + searchContext = mock(SearchContext.class); + mapperService = mock(MapperService.class); + when(searchContext.mapperService()).thenReturn(mapperService); + + // Setup field types + KeywordFieldMapper.KeywordFieldType methodType = new KeywordFieldMapper.KeywordFieldType("method"); + NumberFieldMapper.NumberFieldType statusType = new NumberFieldMapper.NumberFieldType( + "status", + NumberFieldMapper.NumberType.INTEGER + ); + NumberFieldMapper.NumberFieldType portType = new NumberFieldMapper.NumberFieldType("port", NumberFieldMapper.NumberType.INTEGER); + when(mapperService.fieldType("method")).thenReturn(methodType); + when(mapperService.fieldType("status")).thenReturn(statusType); + when(mapperService.fieldType("port")).thenReturn(portType); + + // Create composite field type with dimensions + compositeFieldType = new StarTreeMapper.StarTreeFieldType( + "star_tree", + new StarTreeField( + "star_tree", + List.of(new OrdinalDimension("method"), new OrdinalDimension("status"), new OrdinalDimension("port")), + List.of(new Metric("size", List.of(MetricStat.SUM))), + new StarTreeFieldConfiguration( + randomIntBetween(1, 10_000), + Collections.emptySet(), + StarTreeFieldConfiguration.StarTreeBuildMode.ON_HEAP + ) + ) + ); + } + + public void testSimpleMustWithMultipleDimensions() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have two dimensions", 2, filter.getDimensions().size()); + assertTrue("Should contain method dimension", filter.getDimensions().contains("method")); + assertTrue("Should contain status dimension", filter.getDimensions().contains("status")); + + List methodFilters = filter.getFiltersForDimension("method"); + assertEquals("Should have one filter for method", 1, methodFilters.size()); + assertTrue("Should be ExactMatchDimFilter", methodFilters.getFirst() instanceof ExactMatchDimFilter); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have one filter for status", 1, statusFilters.size()); + assertTrue("Should be ExactMatchDimFilter", statusFilters.getFirst() instanceof ExactMatchDimFilter); + } + + public void testNestedMustClauses() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)).must(new TermQueryBuilder("port", 443))); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); + assertTrue("Should contain all dimensions", filter.getDimensions().containsAll(Set.of("method", "status", "port"))); + } + + public void testMustWithDifferentQueryTypes() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new TermsQueryBuilder("status", Arrays.asList(200, 201))) + .must(new RangeQueryBuilder("port").gte(80).lte(443)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); + + assertTrue( + "Status should have ExactMatchDimFilter", + filter.getFiltersForDimension("status").getFirst() instanceof ExactMatchDimFilter + ); + assertTrue("Port should have RangeMatchDimFilter", filter.getFiltersForDimension("port").getFirst() instanceof RangeMatchDimFilter); + } + + public void testMustWithSameDimension() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)) + .must(new TermQueryBuilder("status", 404)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + // this should return null as same dimension in MUST is logically impossible + assertNull("Filter should be null for same dimension in MUST", filter); + } + + public void testEmptyMustClause() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder(); + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for empty bool query", filter); + } + +} From 4cb1f6674736bb940074c14383fc32713788c4fd Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sun, 20 Apr 2025 19:51:59 +0530 Subject: [PATCH 11/36] Add tests for basic should clause Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 9a38db75a4b50..e72cc2eae5e6f 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -149,4 +149,102 @@ public void testEmptyMustClause() throws IOException { assertNull("Filter should be null for empty bool query", filter); } + public void testShouldWithSameDimension() throws IOException { + // SHOULD with same dimension (Term queries) + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("status", 404)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + assertTrue("Should contain status dimension", filter.getDimensions().contains("status")); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have two filters for status", 2, statusFilters.size()); + assertTrue("Both should be ExactMatchDimFilter", + statusFilters.stream().allMatch(f -> f instanceof ExactMatchDimFilter)); + } + + public void testShouldWithSameDimensionRange() throws IOException { + // SHOULD with same dimension (Range queries) + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new RangeQueryBuilder("status").gte(200).lt(300)) + .should(new RangeQueryBuilder("status").gte(400).lt(500)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have two filters for status", 2, statusFilters.size()); + assertTrue("Both should be RangeMatchDimFilter", + statusFilters.stream().allMatch(f -> f instanceof RangeMatchDimFilter)); + } + + public void testShouldWithSameDimensionMixed() throws IOException { + // SHOULD with same dimension (Mixed Term and Range) + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new RangeQueryBuilder("status").gte(400).lt(500)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have two filters for status", 2, statusFilters.size()); + assertTrue("Should have both ExactMatch and RangeMatch filters", + statusFilters.stream().anyMatch(f -> f instanceof ExactMatchDimFilter) && + statusFilters.stream().anyMatch(f -> f instanceof RangeMatchDimFilter)); + } + + public void testShouldWithDifferentDimensions() throws IOException { + // SHOULD with different dimensions (should be rejected) + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("method", "GET")); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for SHOULD across different dimensions", filter); + } + + public void testNestedShouldSameDimension() throws IOException { + // Nested SHOULD with same dimension + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 404)) + .should(new TermQueryBuilder("status", 500))); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have three filters for status", 3, statusFilters.size()); + assertTrue("All should be ExactMatchDimFilter", + statusFilters.stream().allMatch(f -> f instanceof ExactMatchDimFilter)); + } + + public void testEmptyShouldClause() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new BoolQueryBuilder()); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for empty SHOULD clause", filter); + } + } From c93754198165dc68f50fd9f26b6a9f44e619ce0d Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sun, 20 Apr 2025 20:54:32 +0530 Subject: [PATCH 12/36] Add tests Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 279 +++++++++++++++--- 1 file changed, 242 insertions(+), 37 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index e72cc2eae5e6f..73c72944f591b 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -8,6 +8,7 @@ package org.opensearch.search.aggregations.startree; +import org.apache.lucene.util.BytesRef; import org.opensearch.index.compositeindex.datacube.Metric; import org.opensearch.index.compositeindex.datacube.MetricStat; import org.opensearch.index.compositeindex.datacube.OrdinalDimension; @@ -34,7 +35,9 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Optional; import java.util.Set; import static org.mockito.Mockito.mock; @@ -94,10 +97,12 @@ public void testSimpleMustWithMultipleDimensions() throws IOException { List methodFilters = filter.getFiltersForDimension("method"); assertEquals("Should have one filter for method", 1, methodFilters.size()); assertTrue("Should be ExactMatchDimFilter", methodFilters.getFirst() instanceof ExactMatchDimFilter); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); List statusFilters = filter.getFiltersForDimension("status"); assertEquals("Should have one filter for status", 1, statusFilters.size()); assertTrue("Should be ExactMatchDimFilter", statusFilters.getFirst() instanceof ExactMatchDimFilter); + assertExactMatchValue((ExactMatchDimFilter) statusFilters.getFirst(), 200L); } public void testNestedMustClauses() throws IOException { @@ -109,7 +114,18 @@ public void testNestedMustClauses() throws IOException { assertNotNull("Filter should not be null", filter); assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); - assertTrue("Should contain all dimensions", filter.getDimensions().containsAll(Set.of("method", "status", "port"))); + + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify status filter + List statusFilters = filter.getFiltersForDimension("status"); + assertExactMatchValue((ExactMatchDimFilter) statusFilters.getFirst(), 200L); + + // Verify port filter + List portFilters = filter.getFiltersForDimension("port"); + assertExactMatchValue((ExactMatchDimFilter) portFilters.getFirst(), 443L); } public void testMustWithDifferentQueryTypes() throws IOException { @@ -121,13 +137,26 @@ public void testMustWithDifferentQueryTypes() throws IOException { StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); assertNotNull("Filter should not be null", filter); - assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); - assertTrue( - "Status should have ExactMatchDimFilter", - filter.getFiltersForDimension("status").getFirst() instanceof ExactMatchDimFilter - ); - assertTrue("Port should have RangeMatchDimFilter", filter.getFiltersForDimension("port").getFirst() instanceof RangeMatchDimFilter); + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertTrue("Method should be ExactMatchDimFilter", methodFilters.getFirst() instanceof ExactMatchDimFilter); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify status filter + List statusFilters = filter.getFiltersForDimension("status"); + assertTrue("Status should be ExactMatchDimFilter", statusFilters.getFirst() instanceof ExactMatchDimFilter); + Set expectedStatusValues = Set.of(200L, 201L); + Set actualStatusValues = new HashSet<>(((ExactMatchDimFilter) statusFilters.getFirst()).getRawValues()); + assertEquals("Status should have expected values", expectedStatusValues, actualStatusValues); + + // Verify port filter + List portFilters = filter.getFiltersForDimension("port"); + assertTrue("Port should be RangeMatchDimFilter", portFilters.getFirst() instanceof RangeMatchDimFilter); + RangeMatchDimFilter portRange = (RangeMatchDimFilter) portFilters.getFirst(); + assertEquals("Port lower bound should be 80", 80L, portRange.getLow()); + assertEquals("Port upper bound should be 443", 443L, portRange.getHigh()); + assertTrue("Port bounds should be inclusive", portRange.isIncludeLow() && portRange.isIncludeHigh()); } public void testMustWithSameDimension() throws IOException { @@ -150,9 +179,7 @@ public void testEmptyMustClause() throws IOException { } public void testShouldWithSameDimension() throws IOException { - // SHOULD with same dimension (Term queries) - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) .should(new TermQueryBuilder("status", 404)); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -164,15 +191,19 @@ public void testShouldWithSameDimension() throws IOException { List statusFilters = filter.getFiltersForDimension("status"); assertEquals("Should have two filters for status", 2, statusFilters.size()); - assertTrue("Both should be ExactMatchDimFilter", - statusFilters.stream().allMatch(f -> f instanceof ExactMatchDimFilter)); + assertTrue("Both should be ExactMatchDimFilter", statusFilters.stream().allMatch(f -> f instanceof ExactMatchDimFilter)); + + Set expectedValues = Set.of(200L, 404L); + Set actualValues = new HashSet<>(); + for (DimensionFilter dimensionFilter : statusFilters) { + actualValues.addAll(((ExactMatchDimFilter) dimensionFilter).getRawValues()); + } + assertEquals("Should contain expected status values", expectedValues, actualValues); } public void testShouldWithSameDimensionRange() throws IOException { - // SHOULD with same dimension (Range queries) - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new RangeQueryBuilder("status").gte(200).lt(300)) - .should(new RangeQueryBuilder("status").gte(400).lt(500)); + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new RangeQueryBuilder("status").gte(200).lte(300)) + .should(new RangeQueryBuilder("status").gte(400).lte(500)); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); @@ -182,15 +213,26 @@ public void testShouldWithSameDimensionRange() throws IOException { List statusFilters = filter.getFiltersForDimension("status"); assertEquals("Should have two filters for status", 2, statusFilters.size()); - assertTrue("Both should be RangeMatchDimFilter", - statusFilters.stream().allMatch(f -> f instanceof RangeMatchDimFilter)); + assertTrue("Both should be RangeMatchDimFilter", statusFilters.stream().allMatch(f -> f instanceof RangeMatchDimFilter)); + + // Verify first range + RangeMatchDimFilter firstRange = (RangeMatchDimFilter) statusFilters.getFirst(); + assertEquals("First range lower bound should be 200", 200L, firstRange.getLow()); + assertEquals("First range upper bound should be 300", 300L, firstRange.getHigh()); + assertTrue("First range lower bound should be inclusive", firstRange.isIncludeLow()); + assertTrue("First range upper bound should be inclusive", firstRange.isIncludeHigh()); + + // Verify second range + RangeMatchDimFilter secondRange = (RangeMatchDimFilter) statusFilters.get(1); + assertEquals("Second range lower bound should be 400", 400L, secondRange.getLow()); + assertEquals("Second range upper bound should be 500", 500L, secondRange.getHigh()); + assertTrue("Second range lower bound should be inclusive", secondRange.isIncludeLow()); + assertTrue("Second range upper bound should be inclusive", secondRange.isIncludeHigh()); } public void testShouldWithSameDimensionMixed() throws IOException { - // SHOULD with same dimension (Mixed Term and Range) - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) - .should(new RangeQueryBuilder("status").gte(400).lt(500)); + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) + .should(new RangeQueryBuilder("status").gte(400).lte(500)); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); @@ -200,15 +242,30 @@ public void testShouldWithSameDimensionMixed() throws IOException { List statusFilters = filter.getFiltersForDimension("status"); assertEquals("Should have two filters for status", 2, statusFilters.size()); - assertTrue("Should have both ExactMatch and RangeMatch filters", - statusFilters.stream().anyMatch(f -> f instanceof ExactMatchDimFilter) && - statusFilters.stream().anyMatch(f -> f instanceof RangeMatchDimFilter)); + + // Find and verify exact match filter + Optional exactFilter = statusFilters.stream() + .filter(f -> f instanceof ExactMatchDimFilter) + .map(f -> (ExactMatchDimFilter) f) + .findFirst(); + assertTrue("Should have exact match filter", exactFilter.isPresent()); + assertEquals("Exact match should be 200", 200L, exactFilter.get().getRawValues().get(0)); + + // Find and verify range filter + Optional rangeFilter = statusFilters.stream() + .filter(f -> f instanceof RangeMatchDimFilter) + .map(f -> (RangeMatchDimFilter) f) + .findFirst(); + assertTrue("Should have range filter", rangeFilter.isPresent()); + assertEquals("Range lower bound should be 400", 400L, rangeFilter.get().getLow()); + assertEquals("Range upper bound should be 500", 500L, rangeFilter.get().getHigh()); + assertTrue("Range lower bound should be inclusive", rangeFilter.get().isIncludeLow()); + assertTrue("Range upper bound should be inclusive", rangeFilter.get().isIncludeHigh()); } public void testShouldWithDifferentDimensions() throws IOException { // SHOULD with different dimensions (should be rejected) - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) .should(new TermQueryBuilder("method", "GET")); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -218,12 +275,8 @@ public void testShouldWithDifferentDimensions() throws IOException { } public void testNestedShouldSameDimension() throws IOException { - // Nested SHOULD with same dimension - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) - .should(new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 404)) - .should(new TermQueryBuilder("status", 500))); + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) + .should(new BoolQueryBuilder().should(new TermQueryBuilder("status", 404)).should(new TermQueryBuilder("status", 500))); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); @@ -233,13 +286,18 @@ public void testNestedShouldSameDimension() throws IOException { List statusFilters = filter.getFiltersForDimension("status"); assertEquals("Should have three filters for status", 3, statusFilters.size()); - assertTrue("All should be ExactMatchDimFilter", - statusFilters.stream().allMatch(f -> f instanceof ExactMatchDimFilter)); + assertTrue("All should be ExactMatchDimFilter", statusFilters.stream().allMatch(f -> f instanceof ExactMatchDimFilter)); + + Set expectedValues = Set.of(200L, 404L, 500L); + Set actualValues = new HashSet<>(); + for (DimensionFilter dimensionFilter : statusFilters) { + actualValues.addAll(((ExactMatchDimFilter) dimensionFilter).getRawValues()); + } + assertEquals("Should contain all expected status values", expectedValues, actualValues); } public void testEmptyShouldClause() throws IOException { - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new BoolQueryBuilder()); + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new BoolQueryBuilder()); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); @@ -247,4 +305,151 @@ public void testEmptyShouldClause() throws IOException { assertNull("Filter should be null for empty SHOULD clause", filter); } + public void testMustContainingShouldSameDimension() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("status").gte(200).lt(500)) + .must(new BoolQueryBuilder().should(new TermQueryBuilder("status", 404)).should(new TermQueryBuilder("status", 403))); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have two filters after intersection", 2, statusFilters.size()); + + Set expectedValues = Set.of(403L, 404L); + Set actualValues = new HashSet<>(); + for (DimensionFilter dimFilter : statusFilters) { + assertTrue("Should be ExactMatchDimFilter", dimFilter instanceof ExactMatchDimFilter); + ExactMatchDimFilter exactFilter = (ExactMatchDimFilter) dimFilter; + actualValues.addAll(exactFilter.getRawValues()); + } + assertEquals("Should contain expected status values", expectedValues, actualValues); + } + + public void testMustContainingShouldDifferentDimension() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)).should(new TermQueryBuilder("status", 404))); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have two dimensions", 2, filter.getDimensions().size()); + + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertEquals("Method should have one filter", 1, methodFilters.size()); + assertTrue("Should be ExactMatchDimFilter", methodFilters.getFirst() instanceof ExactMatchDimFilter); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify status filters + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Status should have two filters", 2, statusFilters.size()); + Set expectedStatusValues = Set.of(200L, 404L); + Set actualStatusValues = new HashSet<>(); + for (DimensionFilter dimFilter : statusFilters) { + assertTrue("Should be ExactMatchDimFilter", dimFilter instanceof ExactMatchDimFilter); + ExactMatchDimFilter exactFilter = (ExactMatchDimFilter) dimFilter; + actualStatusValues.addAll(exactFilter.getRawValues()); + } + assertEquals("Should contain expected status values", expectedStatusValues, actualStatusValues); + } + + public void testMultipleLevelsMustNesting() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must( + new BoolQueryBuilder().must(new RangeQueryBuilder("status").gte(200).lte(300)) + .must(new BoolQueryBuilder().must(new TermQueryBuilder("port", 443))) + ); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertEquals("Method should have one filter", 1, methodFilters.size()); + assertTrue("Should be ExactMatchDimFilter", methodFilters.getFirst() instanceof ExactMatchDimFilter); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify status filter + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Status should have one filter", 1, statusFilters.size()); + assertTrue("Should be RangeMatchDimFilter", statusFilters.getFirst() instanceof RangeMatchDimFilter); + RangeMatchDimFilter rangeFilter = (RangeMatchDimFilter) statusFilters.getFirst(); + assertEquals("Lower bound should be 200", 200L, rangeFilter.getLow()); + assertEquals("Upper bound should be 300", 300L, rangeFilter.getHigh()); + assertTrue("Lower bound should be inclusive", rangeFilter.isIncludeLow()); + assertTrue("Upper bound should be inclusive", rangeFilter.isIncludeHigh()); + + // Verify port filter + List portFilters = filter.getFiltersForDimension("port"); + assertEquals("Port should have one filter", 1, portFilters.size()); + assertTrue("Should be ExactMatchDimFilter", portFilters.getFirst() instanceof ExactMatchDimFilter); + assertExactMatchValue((ExactMatchDimFilter) portFilters.getFirst(), 443L); + } + + public void testShouldInsideShouldSameDimension() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) + .should(new BoolQueryBuilder().should(new TermQueryBuilder("status", 404)).should(new TermQueryBuilder("status", 500))); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have three filters", 3, statusFilters.size()); + + Set expectedValues = Set.of(200L, 404L, 500L); + Set actualValues = new HashSet<>(); + for (DimensionFilter dimFilter : statusFilters) { + assertTrue("Should be ExactMatchDimFilter", dimFilter instanceof ExactMatchDimFilter); + ExactMatchDimFilter exactFilter = (ExactMatchDimFilter) dimFilter; + actualValues.addAll(exactFilter.getRawValues()); + } + assertEquals("Should contain all expected values", expectedValues, actualValues); + } + + public void testMustInsideShouldRejected() throws IOException { + // MUST inside SHOULD (should be rejected) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should( + new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)).must(new TermQueryBuilder("method", "GET")) + ); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for MUST inside SHOULD", filter); + } + + public void testComplexNestedStructure() throws IOException { + // Complex nested structure with both MUST and SHOULD + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must( + new BoolQueryBuilder().must(new RangeQueryBuilder("port").gte(80).lte(443)) + .must(new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)).should(new TermQueryBuilder("status", 404))) + ); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); + assertTrue("Should contain all dimensions", filter.getDimensions().containsAll(Set.of("method", "port", "status"))); + } + + // Helper methods for assertions + private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { + assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); + } + + private void assertExactMatchValue(ExactMatchDimFilter filter, Long expectedValue) { + assertEquals(expectedValue, filter.getRawValues().getFirst()); + } + } From b508ea6d3f6264c1f30ba4f8d150f6765fa2d186 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sun, 20 Apr 2025 22:31:21 +0530 Subject: [PATCH 13/36] Add tests Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 73c72944f591b..9809b633c2df8 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -443,6 +443,111 @@ public void testComplexNestedStructure() throws IOException { assertTrue("Should contain all dimensions", filter.getDimensions().containsAll(Set.of("method", "port", "status"))); } + public void testMaximumNestingDepth() throws IOException { + // Build a deeply nested bool query + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")); + + BoolQueryBuilder current = boolQuery; + for (int i = 0; i < 10; i++) { // Test with 10 levels of nesting + BoolQueryBuilder nested = new BoolQueryBuilder().must(new TermQueryBuilder("status", 200 + i)); + current.must(nested); + current = nested; + } + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should not be null", filter); + } + + public void testAllClauseTypesCombined() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must( + new BoolQueryBuilder().must(new RangeQueryBuilder("port").gte(80).lte(443)) + .must(new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)).should(new TermQueryBuilder("status", 201))) + ) + .must(new TermsQueryBuilder("method", Arrays.asList("GET", "POST"))); // This should intersect with first method term + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); + + // Verify method filter (should be intersection of term and terms) + List methodFilters = filter.getFiltersForDimension("method"); + assertEquals("Should have one filter for method after intersection", 1, methodFilters.size()); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify port filter + List portFilters = filter.getFiltersForDimension("port"); + assertTrue("Port should be RangeMatchDimFilter", portFilters.getFirst() instanceof RangeMatchDimFilter); + RangeMatchDimFilter portRange = (RangeMatchDimFilter) portFilters.getFirst(); + assertEquals("Port lower bound should be 80", 80L, portRange.getLow()); + assertEquals("Port upper bound should be 443", 443L, portRange.getHigh()); + + // Verify status filters + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have two filters for status", 2, statusFilters.size()); + Set expectedStatusValues = Set.of(200L, 201L); + Set actualStatusValues = new HashSet<>(); + for (DimensionFilter statusFilter : statusFilters) { + actualStatusValues.addAll(((ExactMatchDimFilter) statusFilter).getRawValues()); + } + assertEquals("Status should have expected values", expectedStatusValues, actualStatusValues); + } + + public void testEmptyNestedBools() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new BoolQueryBuilder()) + .must(new BoolQueryBuilder().must(new BoolQueryBuilder())); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for empty nested bool queries", filter); + } + + public void testSingleClauseBoolQueries() throws IOException { + // Test single MUST clause + BoolQueryBuilder mustOnly = new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(mustOnly); + StarTreeFilter filter = provider.getFilter(searchContext, mustOnly, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + assertExactMatchValue((ExactMatchDimFilter) filter.getFiltersForDimension("status").get(0), 200L); + + // Test single SHOULD clause + BoolQueryBuilder shouldOnly = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)); + + filter = provider.getFilter(searchContext, shouldOnly, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + assertExactMatchValue((ExactMatchDimFilter) filter.getFiltersForDimension("status").get(0), 200L); + } + + public void testDuplicateDimensionsAcrossNesting() throws IOException { + // Test duplicate dimensions that should be merged/intersected + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("status").gte(200).lte(500)) + .must(new BoolQueryBuilder().must(new RangeQueryBuilder("status").gte(300).lte(400))); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have one filter after intersection", 1, statusFilters.size()); + RangeMatchDimFilter rangeFilter = (RangeMatchDimFilter) statusFilters.getFirst(); + assertEquals("Lower bound should be 300", 300L, rangeFilter.getLow()); + assertEquals("Upper bound should be 400", 400L, rangeFilter.getHigh()); + assertTrue("Lower bound should be exclusive", rangeFilter.isIncludeLow()); + assertTrue("Upper bound should be exclusive", rangeFilter.isIncludeHigh()); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); From 25e611e0ddb2aa87c6a31900278b8c6002b3ab57 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sun, 20 Apr 2025 23:00:20 +0530 Subject: [PATCH 14/36] Add tests Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 160 ++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 9809b633c2df8..4e5ccf3fd1a8b 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -19,10 +19,12 @@ import org.opensearch.index.mapper.MapperService; import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.index.mapper.StarTreeMapper; +import org.opensearch.index.mapper.WildcardFieldMapper; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.RangeQueryBuilder; import org.opensearch.index.query.TermQueryBuilder; import org.opensearch.index.query.TermsQueryBuilder; +import org.opensearch.index.query.WildcardQueryBuilder; import org.opensearch.search.internal.SearchContext; import org.opensearch.search.startree.filter.DimensionFilter; import org.opensearch.search.startree.filter.ExactMatchDimFilter; @@ -548,6 +550,164 @@ public void testDuplicateDimensionsAcrossNesting() throws IOException { assertTrue("Upper bound should be exclusive", rangeFilter.isIncludeHigh()); } + public void testKeywordFieldTypeHandling() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new TermsQueryBuilder("method", Arrays.asList("GET", "POST"))) + .must(new TermQueryBuilder("status", 200)) + .must(new BoolQueryBuilder() + .should(new TermQueryBuilder("port", 80)) + .should(new TermQueryBuilder("port", 9200))); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + + // Verify method filter (keyword term query) + List statusFilters = filter.getFiltersForDimension("status"); + assertExactMatchValue((ExactMatchDimFilter) statusFilters.getFirst(), 200L); + + // Verify method filter (keyword terms query) + List methodFilters = filter.getFiltersForDimension("method"); + ExactMatchDimFilter methodFilter = (ExactMatchDimFilter) methodFilters.getFirst(); + Set expectedMethod = new HashSet<>(); + expectedMethod.add(new BytesRef("GET")); + expectedMethod.add(new BytesRef("POST")); + assertEquals(expectedMethod, new HashSet<>(methodFilter.getRawValues())); + + // Verify port filter (keyword SHOULD terms) + List portFilters = filter.getFiltersForDimension("port"); + assertEquals(2, portFilters.size()); + Set expectedPorts = new HashSet<>(); + expectedPorts.add(80L); + expectedPorts.add(9200L); + Set actualZones = new HashSet<>(); + for (DimensionFilter portFilter : portFilters) { + actualZones.addAll(((ExactMatchDimFilter) portFilter).getRawValues()); + } + assertEquals(expectedPorts, actualZones); + } + + public void testInvalidDimensionNames() throws IOException { + // Test dimension that doesn't exist in mapping + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("non_existent_field", "value")) + .must(new TermQueryBuilder("method", "GET")); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for non-existent dimension", filter); + + // Test dimension that exists in mapping but not in star tree dimensions + NumberFieldMapper.NumberFieldType nonStarTreeField = new NumberFieldMapper.NumberFieldType( + "non_star_tree_field", + NumberFieldMapper.NumberType.INTEGER + ); + when(mapperService.fieldType("non_star_tree_field")).thenReturn(nonStarTreeField); + + boolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("non_star_tree_field", 100)) + .must(new TermQueryBuilder("method", "GET")); + + filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + assertNull("Filter should be null for non-star-tree dimension", filter); + } + + public void testUnsupportedQueryTypes() throws IOException { + // Test unsupported query type in MUST + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new WildcardQueryBuilder("method", "GET*")) + .must(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for unsupported query type", filter); + + // Test unsupported query type in SHOULD + boolQuery = new BoolQueryBuilder() + .should(new WildcardQueryBuilder("status", "2*")) + .should(new TermQueryBuilder("status", 404)); + + filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + assertNull("Filter should be null for unsupported query type in SHOULD", filter); + } + + public void testInvalidFieldTypes() throws IOException { + // Test with unsupported field type + WildcardFieldMapper.WildcardFieldType wildcardType = new WildcardFieldMapper.WildcardFieldType("wildcard_field"); + when(mapperService.fieldType("wildcard_field")).thenReturn(wildcardType); + + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("wildcard_field", "value")) + .must(new TermQueryBuilder("method", "GET")); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for unsupported field type", filter); + } + + public void testInvalidShouldClauses() throws IOException { + // Test SHOULD clauses with different dimensions + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("method", "GET")); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for SHOULD with different dimensions", filter); + + // Test nested MUST inside SHOULD + boolQuery = new BoolQueryBuilder() + .should(new BoolQueryBuilder() + .must(new TermQueryBuilder("status", 200)) + .must(new TermQueryBuilder("method", "GET"))); + + filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + assertNull("Filter should be null for MUST inside SHOULD", filter); + } + + public void testInvalidMustClauses() throws IOException { + // Test MUST clauses with same dimension + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("status", 200)) + .must(new TermQueryBuilder("status", 404)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for multiple MUST on same dimension", filter); + + // Test incompatible range intersections + boolQuery = new BoolQueryBuilder() + .must(new RangeQueryBuilder("status").gte(200).lt(300)) + .must(new RangeQueryBuilder("status").gte(400).lt(500)); + + filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + assertNull("Filter should be null for non-overlapping ranges", filter); + } + + public void testMalformedQueries() throws IOException { + // Test empty bool query + BoolQueryBuilder boolQuery = new BoolQueryBuilder(); + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for empty bool query", filter); + + // Test deeply nested empty bool queries + boolQuery = new BoolQueryBuilder() + .must(new BoolQueryBuilder() + .must(new BoolQueryBuilder() + .must(new BoolQueryBuilder()))); + + filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + assertNull("Filter should be null for nested empty bool queries", filter); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); From f883f3259d561f03474e145185ce3efcf08d21ae Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sun, 20 Apr 2025 23:11:05 +0530 Subject: [PATCH 15/36] Add tests Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 182 +++++++++++++++--- 1 file changed, 151 insertions(+), 31 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 4e5ccf3fd1a8b..bb0edbf1d8b1c 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -64,16 +64,29 @@ public void setup() { NumberFieldMapper.NumberType.INTEGER ); NumberFieldMapper.NumberFieldType portType = new NumberFieldMapper.NumberFieldType("port", NumberFieldMapper.NumberType.INTEGER); + KeywordFieldMapper.KeywordFieldType zoneType = new KeywordFieldMapper.KeywordFieldType("zone"); + NumberFieldMapper.NumberFieldType responseTimeType = new NumberFieldMapper.NumberFieldType( + "response_time", + NumberFieldMapper.NumberType.INTEGER + ); when(mapperService.fieldType("method")).thenReturn(methodType); when(mapperService.fieldType("status")).thenReturn(statusType); when(mapperService.fieldType("port")).thenReturn(portType); + when(mapperService.fieldType("zone")).thenReturn(zoneType); + when(mapperService.fieldType("response_time")).thenReturn(responseTimeType); // Create composite field type with dimensions compositeFieldType = new StarTreeMapper.StarTreeFieldType( "star_tree", new StarTreeField( "star_tree", - List.of(new OrdinalDimension("method"), new OrdinalDimension("status"), new OrdinalDimension("port")), + List.of( + new OrdinalDimension("method"), + new OrdinalDimension("status"), + new OrdinalDimension("port"), + new OrdinalDimension("zone"), + new OrdinalDimension("response_time") + ), List.of(new Metric("size", List.of(MetricStat.SUM))), new StarTreeFieldConfiguration( randomIntBetween(1, 10_000), @@ -551,12 +564,9 @@ public void testDuplicateDimensionsAcrossNesting() throws IOException { } public void testKeywordFieldTypeHandling() throws IOException { - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new TermsQueryBuilder("method", Arrays.asList("GET", "POST"))) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermsQueryBuilder("method", Arrays.asList("GET", "POST"))) .must(new TermQueryBuilder("status", 200)) - .must(new BoolQueryBuilder() - .should(new TermQueryBuilder("port", 80)) - .should(new TermQueryBuilder("port", 9200))); + .must(new BoolQueryBuilder().should(new TermQueryBuilder("port", 80)).should(new TermQueryBuilder("port", 9200))); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); @@ -590,8 +600,7 @@ public void testKeywordFieldTypeHandling() throws IOException { public void testInvalidDimensionNames() throws IOException { // Test dimension that doesn't exist in mapping - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("non_existent_field", "value")) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("non_existent_field", "value")) .must(new TermQueryBuilder("method", "GET")); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -606,8 +615,7 @@ public void testInvalidDimensionNames() throws IOException { ); when(mapperService.fieldType("non_star_tree_field")).thenReturn(nonStarTreeField); - boolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("non_star_tree_field", 100)) + boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("non_star_tree_field", 100)) .must(new TermQueryBuilder("method", "GET")); filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); @@ -616,8 +624,7 @@ public void testInvalidDimensionNames() throws IOException { public void testUnsupportedQueryTypes() throws IOException { // Test unsupported query type in MUST - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new WildcardQueryBuilder("method", "GET*")) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new WildcardQueryBuilder("method", "GET*")) .must(new TermQueryBuilder("status", 200)); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -626,9 +633,7 @@ public void testUnsupportedQueryTypes() throws IOException { assertNull("Filter should be null for unsupported query type", filter); // Test unsupported query type in SHOULD - boolQuery = new BoolQueryBuilder() - .should(new WildcardQueryBuilder("status", "2*")) - .should(new TermQueryBuilder("status", 404)); + boolQuery = new BoolQueryBuilder().should(new WildcardQueryBuilder("status", "2*")).should(new TermQueryBuilder("status", 404)); filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); assertNull("Filter should be null for unsupported query type in SHOULD", filter); @@ -639,8 +644,7 @@ public void testInvalidFieldTypes() throws IOException { WildcardFieldMapper.WildcardFieldType wildcardType = new WildcardFieldMapper.WildcardFieldType("wildcard_field"); when(mapperService.fieldType("wildcard_field")).thenReturn(wildcardType); - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("wildcard_field", "value")) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("wildcard_field", "value")) .must(new TermQueryBuilder("method", "GET")); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -651,8 +655,7 @@ public void testInvalidFieldTypes() throws IOException { public void testInvalidShouldClauses() throws IOException { // Test SHOULD clauses with different dimensions - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) .should(new TermQueryBuilder("method", "GET")); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -661,10 +664,9 @@ public void testInvalidShouldClauses() throws IOException { assertNull("Filter should be null for SHOULD with different dimensions", filter); // Test nested MUST inside SHOULD - boolQuery = new BoolQueryBuilder() - .should(new BoolQueryBuilder() - .must(new TermQueryBuilder("status", 200)) - .must(new TermQueryBuilder("method", "GET"))); + boolQuery = new BoolQueryBuilder().should( + new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)).must(new TermQueryBuilder("method", "GET")) + ); filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); assertNull("Filter should be null for MUST inside SHOULD", filter); @@ -672,8 +674,7 @@ public void testInvalidShouldClauses() throws IOException { public void testInvalidMustClauses() throws IOException { // Test MUST clauses with same dimension - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("status", 200)) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)) .must(new TermQueryBuilder("status", 404)); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -682,8 +683,7 @@ public void testInvalidMustClauses() throws IOException { assertNull("Filter should be null for multiple MUST on same dimension", filter); // Test incompatible range intersections - boolQuery = new BoolQueryBuilder() - .must(new RangeQueryBuilder("status").gte(200).lt(300)) + boolQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("status").gte(200).lt(300)) .must(new RangeQueryBuilder("status").gte(400).lt(500)); filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); @@ -699,15 +699,135 @@ public void testMalformedQueries() throws IOException { assertNull("Filter should be null for empty bool query", filter); // Test deeply nested empty bool queries - boolQuery = new BoolQueryBuilder() - .must(new BoolQueryBuilder() - .must(new BoolQueryBuilder() - .must(new BoolQueryBuilder()))); + boolQuery = new BoolQueryBuilder().must(new BoolQueryBuilder().must(new BoolQueryBuilder().must(new BoolQueryBuilder()))); filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); assertNull("Filter should be null for nested empty bool queries", filter); } + public void testComplexMustWithNestedShould() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new RangeQueryBuilder("port").gte(80).lte(443)) + .must( + new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)).should(new RangeQueryBuilder("status").gte(500).lt(600)) + ) // Success or 5xx errors + .must( + new BoolQueryBuilder().must( + new BoolQueryBuilder().should(new TermQueryBuilder("zone", "us-east")).should(new TermQueryBuilder("zone", "us-west")) + ) + ); + + // Add field type for zone + KeywordFieldMapper.KeywordFieldType zoneType = new KeywordFieldMapper.KeywordFieldType("zone"); + when(mapperService.fieldType("zone")).thenReturn(zoneType); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have four dimensions", 4, filter.getDimensions().size()); + + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify port range + List portFilters = filter.getFiltersForDimension("port"); + RangeMatchDimFilter portRange = (RangeMatchDimFilter) portFilters.getFirst(); + assertEquals(80L, portRange.getLow()); + assertEquals(443L, portRange.getHigh()); + assertTrue(portRange.isIncludeLow() && portRange.isIncludeHigh()); + + // Verify status filters (term OR range) + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals(2, statusFilters.size()); + for (DimensionFilter statusFilter : statusFilters) { + if (statusFilter instanceof ExactMatchDimFilter) { + assertEquals(200L, ((ExactMatchDimFilter) statusFilter).getRawValues().getFirst()); + } else { + RangeMatchDimFilter statusRange = (RangeMatchDimFilter) statusFilter; + assertEquals(500L, statusRange.getLow()); + assertEquals(599L, statusRange.getHigh()); + assertTrue(statusRange.isIncludeLow()); + assertTrue(statusRange.isIncludeHigh()); + } + } + + // Verify zone filters + List zoneFilters = filter.getFiltersForDimension("zone"); + assertEquals(2, zoneFilters.size()); + Set expectedZones = new HashSet<>(); + expectedZones.add(new BytesRef("us-east")); + expectedZones.add(new BytesRef("us-west")); + Set actualZones = new HashSet<>(); + for (DimensionFilter zoneFilter : zoneFilters) { + actualZones.addAll(((ExactMatchDimFilter) zoneFilter).getRawValues()); + } + assertEquals(expectedZones, actualZones); + } + + public void testRangeAndTermCombinations() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("status").gte(200).lt(300)) // 2xx status codes + .must(new BoolQueryBuilder().should(new TermQueryBuilder("status", 201)).should(new TermQueryBuilder("status", 204))) // Specific + // success + // codes + .must(new TermQueryBuilder("method", "POST")); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "POST"); + + // Verify status filters (intersection of range and terms) + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals(2, statusFilters.size()); + Set expectedStatus = Set.of(201L, 204L); + Set actualStatus = new HashSet<>(); + for (DimensionFilter statusFilter : statusFilters) { + assertTrue(statusFilter instanceof ExactMatchDimFilter); + actualStatus.addAll(((ExactMatchDimFilter) statusFilter).getRawValues()); + } + assertEquals(expectedStatus, actualStatus); + } + + public void testDeepNestedShouldClauses() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must( + new BoolQueryBuilder().should( + new BoolQueryBuilder().should(new TermQueryBuilder("response_time", 100)) + .should(new TermQueryBuilder("response_time", 200)) + ) + .should( + new BoolQueryBuilder().should(new TermQueryBuilder("response_time", 300)) + .should(new TermQueryBuilder("response_time", 400)) + ) + ); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.get(0), "GET"); + + // Verify response_time filters (all SHOULD conditions) + List responseTimeFilters = filter.getFiltersForDimension("response_time"); + assertEquals(4, responseTimeFilters.size()); + Set expectedTimes = Set.of(100L, 200L, 300L, 400L); + Set actualTimes = new HashSet<>(); + for (DimensionFilter timeFilter : responseTimeFilters) { + assertTrue(timeFilter instanceof ExactMatchDimFilter); + actualTimes.addAll(((ExactMatchDimFilter) timeFilter).getRawValues()); + } + assertEquals(expectedTimes, actualTimes); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); From 04eb4c5d43f3ce2b25da2c78a205c6678128ef9a Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Sun, 20 Apr 2025 23:22:33 +0530 Subject: [PATCH 16/36] Add tests Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index bb0edbf1d8b1c..153226b0660b8 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -828,6 +828,30 @@ public void testDeepNestedShouldClauses() throws IOException { assertEquals(expectedTimes, actualTimes); } + public void testLargeNumberOfClauses() throws IOException { + // Create a bool query with large number of SHOULD clauses + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")); + + // Add 100 SHOULD clauses for status + BoolQueryBuilder statusShould = new BoolQueryBuilder(); + for (int i = 200; i < 300; i++) { + statusShould.should(new TermQueryBuilder("status", i)); + } + boolQuery.must(statusShould); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + + // Verify filters + List methodFilters = filter.getFiltersForDimension("method"); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals(100, statusFilters.size()); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); From 5d9fe8d5c1cff790d6124a7144e35cf826277577 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Mon, 21 Apr 2025 14:07:11 +0530 Subject: [PATCH 17/36] Clean up Signed-off-by: Rishab Nahata --- .../startree/filter/DimensionFilter.java | 9 +- .../filter/DimensionFilterMerger.java | 46 ++++----- .../startree/filter/ExactMatchDimFilter.java | 8 +- .../startree/filter/MatchNoneFilter.java | 11 --- .../startree/filter/RangeMatchDimFilter.java | 45 +++++---- .../provider/DimensionFilterMapper.java | 4 +- .../DimensionFilterAndMapperTests.java | 2 +- .../startree/DimensionFilterMergerTests.java | 98 +++++++------------ 8 files changed, 83 insertions(+), 140 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java index b7e59ad46139b..be4549fc66522 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilter.java @@ -47,11 +47,6 @@ public void matchStarTreeNodes(StarTreeNode parentNode, StarTreeValues starTreeV public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return true; } - - @Override - public String getDimensionName() { - return ""; - } }; /** @@ -79,7 +74,9 @@ public String getDimensionName() { */ boolean matchDimValue(long ordinal, StarTreeValues starTreeValues); - String getDimensionName(); + default String getDimensionName() { + return null; + } /** * Represents how to match a value when comparing during StarTreeTraversal diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java index 56c827a030af4..dbac29a367fdd 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java @@ -24,6 +24,10 @@ public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter return null; } + if (filter1.getDimensionName() == null || filter2.getDimensionName() == null) { + throw new IllegalArgumentException("Cannot intersect filters with null dimension name"); + } + // Verify filters are for same dimension if (!filter1.getDimensionName().equals(filter2.getDimensionName())) { throw new IllegalArgumentException( @@ -31,31 +35,35 @@ public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter ); } + // Handle Range + Range combination if (filter1 instanceof RangeMatchDimFilter && filter2 instanceof RangeMatchDimFilter) { return intersectRangeFilters((RangeMatchDimFilter) filter1, (RangeMatchDimFilter) filter2); } + // Handle ExactMatch + ExactMatch combination if (filter1 instanceof ExactMatchDimFilter && filter2 instanceof ExactMatchDimFilter) { return intersectExactMatchFilters((ExactMatchDimFilter) filter1, (ExactMatchDimFilter) filter2); } - // Handle Range + ExactMatch combinations + // Handle Range + ExactMatch combination if (filter1 instanceof RangeMatchDimFilter && filter2 instanceof ExactMatchDimFilter) { return intersectRangeWithExactMatch((RangeMatchDimFilter) filter1, (ExactMatchDimFilter) filter2); } + // Handle ExactMatch + Range combination if (filter1 instanceof ExactMatchDimFilter && filter2 instanceof RangeMatchDimFilter) { return intersectRangeWithExactMatch((RangeMatchDimFilter) filter2, (ExactMatchDimFilter) filter1); } + // throw exception for unsupported exception throw new IllegalArgumentException( "Unsupported filter combination: " + filter1.getClass().getSimpleName() + " and " + filter2.getClass().getSimpleName() ); } /** - * Intersects two range filters. - * Returns null if ranges don't overlap. + * Intersects two range filters + * Returns null if ranges don't overlap */ private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, RangeMatchDimFilter range2) { Object low1 = range1.getLow(); @@ -120,8 +128,8 @@ private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, } /** - * Intersects two exact match filters. - * Returns null if no common values. + * Intersects two exact match filters + * Returns null if no common values */ private static DimensionFilter intersectExactMatchFilters(ExactMatchDimFilter exact1, ExactMatchDimFilter exact2) { List values1 = exact1.getRawValues(); @@ -178,43 +186,23 @@ private static boolean isValueInRange(Object value, RangeMatchDimFilter range) { return false; } } - return true; } /** - * Compares two values, handling different types appropriately. + * Compares two values - Long for numeric fields and BytesRef for keywords field */ - @SuppressWarnings("unchecked") private static int compareValues(Object v1, Object v2) { - if (v1 instanceof Number && v2 instanceof Number) { - double d1 = ((Number) v1).doubleValue(); - double d2 = ((Number) v2).doubleValue(); - return Double.compare(d1, d2); - } - - if (v1 instanceof String && v2 instanceof String) { - return ((String) v1).compareTo((String) v2); + if (v1 instanceof Long && v2 instanceof Long) { + return Long.compare((Long) v1, (Long) v2); } if (v1 instanceof BytesRef && v2 instanceof BytesRef) { return ((BytesRef) v1).compareTo((BytesRef) v2); } - // Convert BytesRef to String if needed - if (v1 instanceof BytesRef) { - v1 = ((BytesRef) v1).utf8ToString(); - } - if (v2 instanceof BytesRef) { - v2 = ((BytesRef) v2).utf8ToString(); - } - - if (v1 instanceof Comparable && v1.getClass().equals(v2.getClass())) { - return ((Comparable) v1).compareTo(v2); - } - throw new IllegalArgumentException( - "Cannot compare values of types: " + v1.getClass().getName() + " and " + v2.getClass().getName() + "Can only compare Long or BytesRef values, got types: " + v1.getClass().getName() + " and " + v2.getClass().getName() ); } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java index 659b790ad8103..6a6fcc35b4569 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/ExactMatchDimFilter.java @@ -30,10 +30,6 @@ public class ExactMatchDimFilter implements DimensionFilter { private final String dimensionName; - public List getRawValues() { - return rawValues; - } - private final List rawValues; // Order is essential for successive binary search @@ -86,6 +82,10 @@ public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return convertedOrdinals.contains(ordinal); } + public List getRawValues() { + return rawValues; + } + @Override public String getDimensionName() { return dimensionName; diff --git a/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java index df1312f317012..12550b989a220 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/MatchNoneFilter.java @@ -20,12 +20,6 @@ @ExperimentalApi public class MatchNoneFilter implements DimensionFilter { - private final String dimensionName; - - public MatchNoneFilter(String dimensionName) { - this.dimensionName = dimensionName; - } - @Override public void initialiseForSegment(StarTreeValues starTreeValues, SearchContext searchContext) { // Nothing to do as we won't match anything. @@ -40,9 +34,4 @@ public void matchStarTreeNodes(StarTreeNode parentNode, StarTreeValues starTreeV public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return false; } - - @Override - public String getDimensionName() { - return dimensionName; - } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java index a823fc6d88a53..dba6e0a0eaebb 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/RangeMatchDimFilter.java @@ -26,28 +26,6 @@ public class RangeMatchDimFilter implements DimensionFilter { private final String dimensionName; - - @Override - public String getDimensionName() { - return dimensionName; - } - - public Object getLow() { - return low; - } - - public Object getHigh() { - return high; - } - - public boolean isIncludeLow() { - return includeLow; - } - - public boolean isIncludeHigh() { - return includeHigh; - } - private final Object low; private final Object high; private final boolean includeLow; @@ -56,7 +34,7 @@ public boolean isIncludeHigh() { private Long lowOrdinal; private Long highOrdinal; - // TODO - see if we need this while intersecting + // TODO - see if we need to handle this while intersecting private boolean skipRangeCollection = false; public RangeMatchDimFilter(String dimensionName, Object low, Object high, boolean includeLow, boolean includeHigh) { @@ -108,4 +86,25 @@ public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return lowOrdinal <= ordinal && ordinal <= highOrdinal; } + @Override + public String getDimensionName() { + return dimensionName; + } + + public Object getLow() { + return low; + } + + public Object getHigh() { + return high; + } + + public boolean isIncludeLow() { + return includeLow; + } + + public boolean isIncludeHigh() { + return includeHigh; + } + } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java index 4d3d6a8d2c193..8afdb00864b22 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java @@ -164,14 +164,14 @@ public DimensionFilter getRangeMatchFilter( boolean lowerTermHasDecimalPart = hasDecimalPart(parsedLow); if ((lowerTermHasDecimalPart == false && includeLow == false) || (lowerTermHasDecimalPart && signum(parsedLow) > 0)) { if (parsedLow.equals(defaultMaximum())) { - return new MatchNoneFilter(mappedFieldType.name()); + return new MatchNoneFilter(); } ++parsedLow; } boolean upperTermHasDecimalPart = hasDecimalPart(parsedHigh); if ((upperTermHasDecimalPart == false && includeHigh == false) || (upperTermHasDecimalPart && signum(parsedHigh) < 0)) { if (parsedHigh.equals(defaultMinimum())) { - return new MatchNoneFilter(mappedFieldType.name()); + return new MatchNoneFilter(); } --parsedHigh; } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java index 812254baf4c1a..d10ba190f86f1 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java @@ -185,7 +185,7 @@ public void testStarTreeFilterProviders() throws IOException { } // Testing MatchNoneFilter - DimensionFilter dimensionFilter = new MatchNoneFilter("random"); + DimensionFilter dimensionFilter = new MatchNoneFilter(); dimensionFilter.initialiseForSegment(null, null); ArrayBasedCollector collector = new ArrayBasedCollector(); assertFalse(dimensionFilter.matchDimValue(1, null)); diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java index 27a2838772b05..0722a8a302366 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java @@ -24,70 +24,56 @@ public class DimensionFilterMergerTests extends OpenSearchTestCase { public void testRangeIntersection() { // Basic range intersection assertRangeIntersection( - range("status", 200, 500, true, true), - range("status", 300, 400, true, true), - range("status", 300, 400, true, true) + range("status", 200L, 500L, true, true), + range("status", 300L, 400L, true, true), + range("status", 300L, 400L, true, true) ); // Boundary conditions assertRangeIntersection( - range("status", 200, 200, true, true), - range("status", 200, 200, true, true), - range("status", 200, 200, true, true) + range("status", 200L, 200L, true, true), + range("status", 200L, 200L, true, true), + range("status", 200L, 200L, true, true) ); // Inclusive/Exclusive boundaries assertRangeIntersection( - range("status", 200, 300, true, false), - range("status", 200, 300, false, true), - range("status", 200, 300, false, false) + range("status", 200L, 300L, true, false), + range("status", 200L, 300L, false, true), + range("status", 200L, 300L, false, false) ); // Non-overlapping ranges - assertNoIntersection(range("status", 200, 300, true, true), range("status", 301, 400, true, true)); + assertNoIntersection(range("status", 200L, 300L, true, true), range("status", 301L, 400L, true, true)); // Exactly touching ranges (no overlap) - assertNoIntersection(range("status", 200, 300, true, false), range("status", 300, 400, true, true)); + assertNoIntersection(range("status", 200L, 300L, true, false), range("status", 300L, 400L, true, true)); // Null bounds (unbounded ranges) assertRangeIntersection( - range("status", null, 500, true, true), - range("status", 200, null, true, true), - range("status", 200, 500, true, true) + range("status", null, 500L, true, true), + range("status", 200L, null, true, true), + range("status", 200L, 500L, true, true) ); // Single point overlap assertRangeIntersection( - range("status", 200, 300, true, true), - range("status", 300, 400, true, true), - range("status", 300, 300, true, true) + range("status", 200L, 300L, true, true), + range("status", 300L, 400L, true, true), + range("status", 300L, 300L, true, true) ); // Very large ranges assertRangeIntersection( range("status", Long.MIN_VALUE, Long.MAX_VALUE, true, true), - range("status", 200, 300, true, true), - range("status", 200, 300, true, true) + range("status", 200L, 300L, true, true), + range("status", 200L, 300L, true, true) ); // Zero-width ranges - assertNoIntersection(range("status", 200, 200, true, true), range("status", 200, 200, false, false)); + assertNoIntersection(range("status", 200L, 200L, true, true), range("status", 200L, 200L, false, false)); - // Floating point precision - assertRangeIntersection( - range("latency", 1.0, 2.0, true, true), - range("latency", 2.0, 3.0, true, true), - range("latency", 2.0, 2.0, true, true) - ); - - // Different numeric types - assertRangeIntersection( - range("size", 1.0f, 2.0f, true, true), - range("size", 1L, 2L, true, true), - range("size", 1.0f, 2.0f, true, true) - ); - - // String to numeric conversion + // incompatible types assertThrows( IllegalArgumentException.class, () -> DimensionFilterMerger.intersect(range("status", "200", "300", true, true), range("status", 200, 300, true, true)) @@ -136,33 +122,33 @@ public void testExactMatchIntersection() { public void testRangeExactMatchIntersection() { // Value in range assertRangeExactMatchIntersection( - range("status", 200, 300, true, true), - exactMatch("status", List.of(250)), - exactMatch("status", List.of(250)) + range("status", 200L, 300L, true, true), + exactMatch("status", List.of(250L)), + exactMatch("status", List.of(250L)) ); // Value at range boundaries assertRangeExactMatchIntersection( - range("status", 200, 300, true, true), - exactMatch("status", Arrays.asList(200, 300)), - exactMatch("status", Arrays.asList(200, 300)) + range("status", 200L, 300L, true, true), + exactMatch("status", Arrays.asList(200L, 300L)), + exactMatch("status", Arrays.asList(200L, 300L)) ); // Value at exclusive boundaries assertRangeExactMatchIntersection( - range("status", 200, 300, false, false), - exactMatch("status", Arrays.asList(201, 299)), - exactMatch("status", Arrays.asList(201, 299)) + range("status", 200L, 300L, false, false), + exactMatch("status", Arrays.asList(201L, 299L)), + exactMatch("status", Arrays.asList(201L, 299L)) ); // No values in range - assertNoIntersection(range("status", 200, 300, true, true), exactMatch("status", Arrays.asList(199, 301))); + assertNoIntersection(range("status", 200L, 300L, true, true), exactMatch("status", Arrays.asList(199L, 301L))); // Multiple values, some in range assertRangeExactMatchIntersection( - range("status", 200, 300, true, true), - exactMatch("status", Arrays.asList(199, 200, 250, 300, 301)), - exactMatch("status", Arrays.asList(200, 250, 300)) + range("status", 200L, 300L, true, true), + exactMatch("status", Arrays.asList(199L, 200L, 250L, 300L, 301L)), + exactMatch("status", Arrays.asList(200L, 250L, 300L)) ); } @@ -174,22 +160,6 @@ public void testDifferentDimensions() { ); } - public void testTypeConversion() { - // Integer and Long - assertRangeIntersection( - range("port", 80, 443, true, true), - range("port", 80L, 443L, true, true), - range("port", 80, 443, true, true) - ); - - // Double and Integer - assertRangeIntersection( - range("latency", 1.0, 2.0, true, true), - range("latency", 1, 2, true, true), - range("latency", 1.0, 2.0, true, true) - ); - } - // Helper methods private RangeMatchDimFilter range(String dimension, Object low, Object high, boolean includeLow, boolean includeHigh) { return new RangeMatchDimFilter(dimension, low, high, includeLow, includeHigh); From a17d242bd38edcf622217dea5363b01d40417d88 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Mon, 21 Apr 2025 15:15:52 +0530 Subject: [PATCH 18/36] Handle must inside should Signed-off-by: Rishab Nahata --- .../filter/provider/StarTreeFilterProvider.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java index fe2f748275a01..a2be174b75b26 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java @@ -290,14 +290,13 @@ private StarTreeFilter processShouldClauses( if (clause instanceof BoolQueryBuilder) { BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; - - // Reject if contains MUST clauses if (boolClause.must().isEmpty() == false) { - return null; // Cannot have MUST inside SHOULD + clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); + } else if (boolClause.should().isEmpty() == false) { + clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); + } else { + return null; } - - // Process nested SHOULD - clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); } else { // Only allow term, terms, and range queries if (!(clause instanceof TermQueryBuilder) @@ -325,7 +324,7 @@ private StarTreeFilter processShouldClauses( String dimension = clauseFilter.getDimensions().iterator().next(); if (commonDimension == null) { commonDimension = dimension; - } else if (!commonDimension.equals(dimension)) { + } else if (commonDimension.equals(dimension) == false) { return null; // All SHOULD clauses must operate on same dimension } From 6db89a5c9f2f0952535122c06b75a3f731a0f32c Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Mon, 21 Apr 2025 15:20:24 +0530 Subject: [PATCH 19/36] Add tests for must inside should Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 153226b0660b8..196447ce485ff 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -852,6 +852,36 @@ public void testLargeNumberOfClauses() throws IOException { assertEquals(100, statusFilters.size()); } + public void testMustInsideShould() throws IOException { + // Test valid case - all clauses on same dimension + BoolQueryBuilder validBoolQuery = new BoolQueryBuilder().should( + new BoolQueryBuilder().must(new RangeQueryBuilder("status").gte(200).lt(300)).must(new TermQueryBuilder("status", 201)) + ).should(new TermQueryBuilder("status", 404)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(validBoolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, validBoolQuery, compositeFieldType); + + assertNotNull("Filter should not be null for same dimension", filter); + assertEquals("Should have one dimension", 1, filter.getDimensions().size()); + List statusFilters = filter.getFiltersForDimension("status"); + assertEquals("Should have two filters", 2, statusFilters.size()); + Set expectedValues = Set.of(201L, 404L); + Set actualValues = new HashSet<>(); + for (DimensionFilter dimFilter : statusFilters) { + assertTrue("Should be ExactMatchDimFilter", dimFilter instanceof ExactMatchDimFilter); + actualValues.addAll(((ExactMatchDimFilter) dimFilter).getRawValues()); + } + assertEquals("Should contain expected values", expectedValues, actualValues); + + // Test invalid case - multiple dimensions in MUST inside SHOULD + BoolQueryBuilder invalidBoolQuery = new BoolQueryBuilder().should( + new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)).must(new TermQueryBuilder("method", "GET")) + ).should(new TermQueryBuilder("status", 404)); + + filter = provider.getFilter(searchContext, invalidBoolQuery, compositeFieldType); + assertNull("Filter should be null for multiple dimensions in MUST inside SHOULD", filter); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); From a5adef0e2dadfb187add098e2b3c683dbe410a61 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Tue, 22 Apr 2025 15:23:06 +0530 Subject: [PATCH 20/36] Move BoolStarTreeFilterProvider to different source file Signed-off-by: Rishab Nahata --- .../provider/BoolStarTreeFilterProvider.java | 200 ++++++++++++++++++ .../provider/StarTreeFilterProvider.java | 178 ---------------- 2 files changed, 200 insertions(+), 178 deletions(-) create mode 100644 server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java new file mode 100644 index 0000000000000..2ce88e14b2895 --- /dev/null +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -0,0 +1,200 @@ +/* + * 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.search.startree.filter.provider; + +import org.opensearch.index.mapper.CompositeDataCubeFieldType; +import org.opensearch.index.query.BoolQueryBuilder; +import org.opensearch.index.query.QueryBuilder; +import org.opensearch.index.query.RangeQueryBuilder; +import org.opensearch.index.query.TermQueryBuilder; +import org.opensearch.index.query.TermsQueryBuilder; +import org.opensearch.search.internal.SearchContext; +import org.opensearch.search.startree.filter.DimensionFilter; +import org.opensearch.search.startree.filter.DimensionFilterMerger; +import org.opensearch.search.startree.filter.StarTreeFilter; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class BoolStarTreeFilterProvider implements StarTreeFilterProvider { + @Override + public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) + throws IOException { + BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) rawFilter; + if (!boolQueryBuilder.hasClauses()) { + return null; + } + + if (boolQueryBuilder.must().isEmpty() == false) { + return processMustClauses(boolQueryBuilder.must(), context, compositeFieldType); + } + + if (boolQueryBuilder.should().isEmpty() == false) { + return processShouldClauses(boolQueryBuilder.should(), context, compositeFieldType); + } + + return null; + } + + private StarTreeFilter processMustClauses( + List mustClauses, + SearchContext context, + CompositeDataCubeFieldType compositeFieldType + ) throws IOException { + if (mustClauses.isEmpty()) { + return null; + } + Map> dimensionToFilters = new HashMap<>(); + + for (QueryBuilder clause : mustClauses) { + StarTreeFilter clauseFilter; + + if (clause instanceof BoolQueryBuilder) { + BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; + if (boolClause.must().isEmpty() == false) { + // Process nested MUST + clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); + } else if (boolClause.should().isEmpty() == false) { + // Process SHOULD inside MUST + clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); + // Note: clauseFilter now contains all SHOULD conditions as separate filters + } else { + return null; + } + } else { + // Only allow other supported QueryBuilders + if (!(clause instanceof TermQueryBuilder) + && !(clause instanceof TermsQueryBuilder) + && !(clause instanceof RangeQueryBuilder)) { + return null; + } + // Process individual clause + StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); + if (provider == null) { + return null; + } + clauseFilter = provider.getFilter(context, clause, compositeFieldType); + } + + if (clauseFilter == null) { + return null; + } + + // Merge filters for each dimension + for (String dimension : clauseFilter.getDimensions()) { + List existingFilters = dimensionToFilters.get(dimension); + List newFilters = clauseFilter.getFiltersForDimension(dimension); + + if (existingFilters == null) { + // No existing filters for this dimension + dimensionToFilters.put(dimension, new ArrayList<>(newFilters)); + } else { + // We have existing filters for this dimension + if (newFilters.size() > 1) { + // New filters are from SHOULD clause (multiple filters = OR condition) + // Need to intersect each SHOULD filter with existing filters + List intersectedFilters = new ArrayList<>(); + for (DimensionFilter shouldFilter : newFilters) { + for (DimensionFilter existingFilter : existingFilters) { + DimensionFilter intersected = DimensionFilterMerger.intersect(existingFilter, shouldFilter); + if (intersected != null) { + intersectedFilters.add(intersected); + } + } + } + if (intersectedFilters.isEmpty()) { + return null; // No valid intersections + } + dimensionToFilters.put(dimension, intersectedFilters); + } else { + // Here's where we need the DimensionFilter merging logic + // For example: merging range with term, or range with range + // And a single dimension filter coming from should clause is as good as must clause + DimensionFilter mergedFilter = DimensionFilterMerger.intersect( + existingFilters.getFirst(), + newFilters.getFirst() + ); + if (mergedFilter == null) { + return null; // No possible matches after merging + } + dimensionToFilters.put(dimension, Collections.singletonList(mergedFilter)); + } + } + } + } + + return new StarTreeFilter(dimensionToFilters); + } + + private StarTreeFilter processShouldClauses( + List shouldClauses, + SearchContext context, + CompositeDataCubeFieldType compositeFieldType + ) throws IOException { + if (shouldClauses.isEmpty()) { + return null; + } + + // First, validate all SHOULD clauses are for same dimension + String commonDimension = null; + Map> dimensionToFilters = new HashMap<>(); + for (QueryBuilder clause : shouldClauses) { + StarTreeFilter clauseFilter; + + if (clause instanceof BoolQueryBuilder) { + BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; + if (boolClause.must().isEmpty() == false) { + clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); + } else if (boolClause.should().isEmpty() == false) { + clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); + } else { + return null; + } + } else { + // Only allow other supported QueryBuilders + if (!(clause instanceof TermQueryBuilder) + && !(clause instanceof TermsQueryBuilder) + && !(clause instanceof RangeQueryBuilder)) { + return null; + } + + StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); + if (provider == null) { + return null; + } + clauseFilter = provider.getFilter(context, clause, compositeFieldType); + } + + if (clauseFilter == null) { + return null; + } + + // Validate single dimension + if (clauseFilter.getDimensions().size() != 1) { + return null; // SHOULD clause must operate on single dimension + } + + String dimension = clauseFilter.getDimensions().iterator().next(); + if (commonDimension == null) { + commonDimension = dimension; + } else if (commonDimension.equals(dimension) == false) { + return null; // All SHOULD clauses must operate on same dimension + } + + // Simply collect all filters - StarTreeTraversal will handle OR operation + dimensionToFilters.computeIfAbsent(dimension, k -> new ArrayList<>()) + .addAll(clauseFilter.getFiltersForDimension(dimension)); + } + return new StarTreeFilter(dimensionToFilters); + } +} diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java index a2be174b75b26..b58e284323fdf 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java @@ -20,14 +20,10 @@ import org.opensearch.index.query.TermsQueryBuilder; import org.opensearch.search.internal.SearchContext; import org.opensearch.search.startree.StarTreeQueryHelper; -import org.opensearch.search.startree.filter.DimensionFilter; -import org.opensearch.search.startree.filter.DimensionFilterMerger; import org.opensearch.search.startree.filter.StarTreeFilter; import java.io.IOException; -import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; @@ -160,179 +156,5 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C ); } } - - } - - class BoolStarTreeFilterProvider implements StarTreeFilterProvider { - @Override - public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) - throws IOException { - BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) rawFilter; - if (!boolQueryBuilder.hasClauses()) { - return null; - } - - if (boolQueryBuilder.must().isEmpty() == false) { - return processMustClauses(boolQueryBuilder.must(), context, compositeFieldType); - } - - if (boolQueryBuilder.should().isEmpty() == false) { - return processShouldClauses(boolQueryBuilder.should(), context, compositeFieldType); - } - - return null; - } - - private StarTreeFilter processMustClauses( - List mustClauses, - SearchContext context, - CompositeDataCubeFieldType compositeFieldType - ) throws IOException { - if (mustClauses.isEmpty()) { - return null; - } - Map> dimensionToFilters = new HashMap<>(); - - for (QueryBuilder clause : mustClauses) { - StarTreeFilter clauseFilter; - - if (clause instanceof BoolQueryBuilder) { - BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; - if (boolClause.must().isEmpty() == false) { - // Process nested MUST - clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); - } else if (boolClause.should().isEmpty() == false) { - // Process SHOULD inside MUST - clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); - // Note: clauseFilter now contains all SHOULD conditions as separate filters - } else { - return null; - } - } else { - // Only allow term, terms, and range queries - // if (!(clause instanceof TermQueryBuilder) && - // !(clause instanceof TermsQueryBuilder) && - // !(clause instanceof RangeQueryBuilder)) { - // return null; - // } - // Process individual clause - StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); - if (provider == null) { - return null; - } - clauseFilter = provider.getFilter(context, clause, compositeFieldType); - } - - if (clauseFilter == null) { - return null; - } - - // Merge filters for each dimension - for (String dimension : clauseFilter.getDimensions()) { - List existingFilters = dimensionToFilters.get(dimension); - List newFilters = clauseFilter.getFiltersForDimension(dimension); - - if (existingFilters == null) { - // No existing filters for this dimension - dimensionToFilters.put(dimension, new ArrayList<>(newFilters)); - } else { - // We have existing filters for this dimension - if (newFilters.size() > 1) { - // New filters are from SHOULD clause (multiple filters = OR condition) - // Need to intersect each SHOULD filter with existing filters - List intersectedFilters = new ArrayList<>(); - for (DimensionFilter shouldFilter : newFilters) { - for (DimensionFilter existingFilter : existingFilters) { - DimensionFilter intersected = DimensionFilterMerger.intersect(existingFilter, shouldFilter); - if (intersected != null) { - intersectedFilters.add(intersected); - } - } - } - if (intersectedFilters.isEmpty()) { - return null; // No valid intersections - } - dimensionToFilters.put(dimension, intersectedFilters); - } else { - // Here's where we need the DimensionFilter merging logic - // For example: merging range with term, or range with range - // And a single dimension filter coming from should clause is as good as must clause - DimensionFilter mergedFilter = DimensionFilterMerger.intersect( - existingFilters.getFirst(), - newFilters.getFirst() - ); - if (mergedFilter == null) { - return null; // No possible matches after merging - } - dimensionToFilters.put(dimension, Collections.singletonList(mergedFilter)); - } - } - } - } - - return new StarTreeFilter(dimensionToFilters); - } - - private StarTreeFilter processShouldClauses( - List shouldClauses, - SearchContext context, - CompositeDataCubeFieldType compositeFieldType - ) throws IOException { - if (shouldClauses.isEmpty()) { - return null; - } - - // First, validate all SHOULD clauses are for same dimension - String commonDimension = null; - Map> dimensionToFilters = new HashMap<>(); - for (QueryBuilder clause : shouldClauses) { - StarTreeFilter clauseFilter; - - if (clause instanceof BoolQueryBuilder) { - BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; - if (boolClause.must().isEmpty() == false) { - clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); - } else if (boolClause.should().isEmpty() == false) { - clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); - } else { - return null; - } - } else { - // Only allow term, terms, and range queries - if (!(clause instanceof TermQueryBuilder) - && !(clause instanceof TermsQueryBuilder) - && !(clause instanceof RangeQueryBuilder)) { - return null; - } - - StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); - if (provider == null) { - return null; - } - clauseFilter = provider.getFilter(context, clause, compositeFieldType); - } - - if (clauseFilter == null) { - return null; - } - - // Validate single dimension - if (clauseFilter.getDimensions().size() != 1) { - return null; // SHOULD clause must operate on single dimension - } - - String dimension = clauseFilter.getDimensions().iterator().next(); - if (commonDimension == null) { - commonDimension = dimension; - } else if (commonDimension.equals(dimension) == false) { - return null; // All SHOULD clauses must operate on same dimension - } - - // Simply collect all filters - StarTreeTraversal will handle OR operation - dimensionToFilters.computeIfAbsent(dimension, k -> new ArrayList<>()) - .addAll(clauseFilter.getFiltersForDimension(dimension)); - } - return new StarTreeFilter(dimensionToFilters); - } } } From e4d43f92c4c4ea3cdf8835cec5b5c80fc275763a Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Tue, 22 Apr 2025 15:34:23 +0530 Subject: [PATCH 21/36] Handle filter clause Signed-off-by: Rishab Nahata --- .../provider/BoolStarTreeFilterProvider.java | 13 +++- .../BoolStarTreeFilterProviderTests.java | 69 +++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java index 2ce88e14b2895..2d57372783b21 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -36,7 +36,7 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C } if (boolQueryBuilder.must().isEmpty() == false) { - return processMustClauses(boolQueryBuilder.must(), context, compositeFieldType); + return processMustClauses(getCombinedMustAndFilterClauses(boolQueryBuilder), context, compositeFieldType); } if (boolQueryBuilder.should().isEmpty() == false) { @@ -63,7 +63,7 @@ private StarTreeFilter processMustClauses( BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; if (boolClause.must().isEmpty() == false) { // Process nested MUST - clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); + clauseFilter = processMustClauses(getCombinedMustAndFilterClauses(boolClause), context, compositeFieldType); } else if (boolClause.should().isEmpty() == false) { // Process SHOULD inside MUST clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); @@ -154,7 +154,7 @@ private StarTreeFilter processShouldClauses( if (clause instanceof BoolQueryBuilder) { BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; if (boolClause.must().isEmpty() == false) { - clauseFilter = processMustClauses(boolClause.must(), context, compositeFieldType); + clauseFilter = processMustClauses(getCombinedMustAndFilterClauses(boolClause), context, compositeFieldType); } else if (boolClause.should().isEmpty() == false) { clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); } else { @@ -197,4 +197,11 @@ private StarTreeFilter processShouldClauses( } return new StarTreeFilter(dimensionToFilters); } + + private List getCombinedMustAndFilterClauses(BoolQueryBuilder boolQuery) { + List mustAndFilterClauses = new ArrayList<>(); + mustAndFilterClauses.addAll(boolQuery.must()); + mustAndFilterClauses.addAll(boolQuery.filter()); + return mustAndFilterClauses; + } } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 196447ce485ff..589149a21c75c 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -882,6 +882,75 @@ public void testMustInsideShould() throws IOException { assertNull("Filter should be null for multiple dimensions in MUST inside SHOULD", filter); } + public void testCombinedMustAndFilterClauses() throws IOException { + // Test combination of MUST and FILTER clauses + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("method", "GET")) + .filter(new TermQueryBuilder("status", 200)) + .filter(new RangeQueryBuilder("port").gte(80).lte(443)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); + + // Verify method filter (from MUST) + List methodFilters = filter.getFiltersForDimension("method"); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify status filter (from FILTER) + List statusFilters = filter.getFiltersForDimension("status"); + assertExactMatchValue((ExactMatchDimFilter) statusFilters.getFirst(), 200L); + + // Verify port filter (from FILTER) + List portFilters = filter.getFiltersForDimension("port"); + RangeMatchDimFilter portRange = (RangeMatchDimFilter) portFilters.getFirst(); + assertEquals(80L, portRange.getLow()); + assertEquals(443L, portRange.getHigh()); + assertTrue(portRange.isIncludeLow() && portRange.isIncludeHigh()); + } + + public void testNestedBoolWithMustAndFilter() throws IOException { + // Test nested bool query with both MUST and FILTER clauses + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("method", "GET")) + .must(new BoolQueryBuilder() + .filter(new RangeQueryBuilder("status").gte(200).lt(300)) + .must(new TermQueryBuilder("status", 201))) // Should intersect with range + .filter(new TermQueryBuilder("port", 443)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + assertEquals("Should have three dimensions", 3, filter.getDimensions().size()); + + // Verify method filter + List methodFilters = filter.getFiltersForDimension("method"); + assertExactMatchValue((ExactMatchDimFilter) methodFilters.getFirst(), "GET"); + + // Verify status filter (intersection of range and term) + List statusFilters = filter.getFiltersForDimension("status"); + assertExactMatchValue((ExactMatchDimFilter) statusFilters.getFirst(), 201L); + + // Verify port filter + List portFilters = filter.getFiltersForDimension("port"); + assertExactMatchValue((ExactMatchDimFilter) portFilters.getFirst(), 443L); + } + + public void testInvalidMustAndFilterCombination() throws IOException { + // Test invalid combination - same dimension in MUST and FILTER + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("status", 200)) + .filter(new TermQueryBuilder("status", 404)); // Different value for same dimension + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null for conflicting conditions", filter); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); From 1496990fcbb09a5b266b768ec15b5bce0429c329 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Tue, 22 Apr 2025 16:06:12 +0530 Subject: [PATCH 22/36] refactor Signed-off-by: Rishab Nahata --- .../provider/BoolStarTreeFilterProvider.java | 94 +++++++------------ .../BoolStarTreeFilterProviderTests.java | 16 ++-- 2 files changed, 43 insertions(+), 67 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java index 2d57372783b21..38ec87a189e69 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -30,22 +30,43 @@ public class BoolStarTreeFilterProvider implements StarTreeFilterProvider { @Override public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) throws IOException { - BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) rawFilter; - if (!boolQueryBuilder.hasClauses()) { + return processBoolQuery((BoolQueryBuilder) rawFilter, context, compositeFieldType); + } + + private StarTreeFilter processBoolQuery( + BoolQueryBuilder boolQuery, + SearchContext context, + CompositeDataCubeFieldType compositeFieldType + ) throws IOException { + if (boolQuery.hasClauses() == false) { return null; } - - if (boolQueryBuilder.must().isEmpty() == false) { - return processMustClauses(getCombinedMustAndFilterClauses(boolQueryBuilder), context, compositeFieldType); + if (boolQuery.must().isEmpty() == false || boolQuery.filter().isEmpty() == false) { + return processMustClauses(getCombinedMustAndFilterClauses(boolQuery), context, compositeFieldType); } - - if (boolQueryBuilder.should().isEmpty() == false) { - return processShouldClauses(boolQueryBuilder.should(), context, compositeFieldType); + if (boolQuery.should().isEmpty() == false) { + return processShouldClauses(boolQuery.should(), context, compositeFieldType); } - return null; } + private StarTreeFilter processNonBoolSupportedQueries( + QueryBuilder query, + SearchContext context, + CompositeDataCubeFieldType compositeFieldType + ) throws IOException { + // Only allow other supported QueryBuilders + if (!(query instanceof TermQueryBuilder) && !(query instanceof TermsQueryBuilder) && !(query instanceof RangeQueryBuilder)) { + return null; + } + // Process individual clause + StarTreeFilterProvider provider = SingletonFactory.getProvider(query); + if (provider == null) { + return null; + } + return provider.getFilter(context, query, compositeFieldType); + } + private StarTreeFilter processMustClauses( List mustClauses, SearchContext context, @@ -60,30 +81,9 @@ private StarTreeFilter processMustClauses( StarTreeFilter clauseFilter; if (clause instanceof BoolQueryBuilder) { - BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; - if (boolClause.must().isEmpty() == false) { - // Process nested MUST - clauseFilter = processMustClauses(getCombinedMustAndFilterClauses(boolClause), context, compositeFieldType); - } else if (boolClause.should().isEmpty() == false) { - // Process SHOULD inside MUST - clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); - // Note: clauseFilter now contains all SHOULD conditions as separate filters - } else { - return null; - } + clauseFilter = processBoolQuery((BoolQueryBuilder) clause, context, compositeFieldType); } else { - // Only allow other supported QueryBuilders - if (!(clause instanceof TermQueryBuilder) - && !(clause instanceof TermsQueryBuilder) - && !(clause instanceof RangeQueryBuilder)) { - return null; - } - // Process individual clause - StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); - if (provider == null) { - return null; - } - clauseFilter = provider.getFilter(context, clause, compositeFieldType); + clauseFilter = processNonBoolSupportedQueries(clause, context, compositeFieldType); } if (clauseFilter == null) { @@ -120,10 +120,7 @@ private StarTreeFilter processMustClauses( // Here's where we need the DimensionFilter merging logic // For example: merging range with term, or range with range // And a single dimension filter coming from should clause is as good as must clause - DimensionFilter mergedFilter = DimensionFilterMerger.intersect( - existingFilters.getFirst(), - newFilters.getFirst() - ); + DimensionFilter mergedFilter = DimensionFilterMerger.intersect(existingFilters.getFirst(), newFilters.getFirst()); if (mergedFilter == null) { return null; // No possible matches after merging } @@ -152,27 +149,9 @@ private StarTreeFilter processShouldClauses( StarTreeFilter clauseFilter; if (clause instanceof BoolQueryBuilder) { - BoolQueryBuilder boolClause = (BoolQueryBuilder) clause; - if (boolClause.must().isEmpty() == false) { - clauseFilter = processMustClauses(getCombinedMustAndFilterClauses(boolClause), context, compositeFieldType); - } else if (boolClause.should().isEmpty() == false) { - clauseFilter = processShouldClauses(boolClause.should(), context, compositeFieldType); - } else { - return null; - } + clauseFilter = processBoolQuery((BoolQueryBuilder) clause, context, compositeFieldType); } else { - // Only allow other supported QueryBuilders - if (!(clause instanceof TermQueryBuilder) - && !(clause instanceof TermsQueryBuilder) - && !(clause instanceof RangeQueryBuilder)) { - return null; - } - - StarTreeFilterProvider provider = SingletonFactory.getProvider(clause); - if (provider == null) { - return null; - } - clauseFilter = provider.getFilter(context, clause, compositeFieldType); + clauseFilter = processNonBoolSupportedQueries(clause, context, compositeFieldType); } if (clauseFilter == null) { @@ -192,8 +171,7 @@ private StarTreeFilter processShouldClauses( } // Simply collect all filters - StarTreeTraversal will handle OR operation - dimensionToFilters.computeIfAbsent(dimension, k -> new ArrayList<>()) - .addAll(clauseFilter.getFiltersForDimension(dimension)); + dimensionToFilters.computeIfAbsent(dimension, k -> new ArrayList<>()).addAll(clauseFilter.getFiltersForDimension(dimension)); } return new StarTreeFilter(dimensionToFilters); } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 589149a21c75c..98d989b39da36 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -884,8 +884,7 @@ public void testMustInsideShould() throws IOException { public void testCombinedMustAndFilterClauses() throws IOException { // Test combination of MUST and FILTER clauses - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("method", "GET")) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) .filter(new TermQueryBuilder("status", 200)) .filter(new RangeQueryBuilder("port").gte(80).lte(443)); @@ -913,11 +912,11 @@ public void testCombinedMustAndFilterClauses() throws IOException { public void testNestedBoolWithMustAndFilter() throws IOException { // Test nested bool query with both MUST and FILTER clauses - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("method", "GET")) - .must(new BoolQueryBuilder() - .filter(new RangeQueryBuilder("status").gte(200).lt(300)) - .must(new TermQueryBuilder("status", 201))) // Should intersect with range + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must(new BoolQueryBuilder().filter(new RangeQueryBuilder("status").gte(200).lt(300)).must(new TermQueryBuilder("status", 201))) // Should + // intersect + // with + // range .filter(new TermQueryBuilder("port", 443)); StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); @@ -941,8 +940,7 @@ public void testNestedBoolWithMustAndFilter() throws IOException { public void testInvalidMustAndFilterCombination() throws IOException { // Test invalid combination - same dimension in MUST and FILTER - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("status", 200)) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)) .filter(new TermQueryBuilder("status", 404)); // Different value for same dimension StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); From b82451f32ae2b3a75c8b176a362679e80fc344e3 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Tue, 22 Apr 2025 17:05:24 +0530 Subject: [PATCH 23/36] Add keyword and float range tests Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 121 +++++++++++++++++- 1 file changed, 119 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 98d989b39da36..624213c764e7c 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -8,7 +8,9 @@ package org.opensearch.search.aggregations.startree; +import org.apache.lucene.document.FloatPoint; import org.apache.lucene.util.BytesRef; +import org.apache.lucene.util.NumericUtils; import org.opensearch.index.compositeindex.datacube.Metric; import org.opensearch.index.compositeindex.datacube.MetricStat; import org.opensearch.index.compositeindex.datacube.OrdinalDimension; @@ -69,11 +71,18 @@ public void setup() { "response_time", NumberFieldMapper.NumberType.INTEGER ); + NumberFieldMapper.NumberFieldType latencyType = new NumberFieldMapper.NumberFieldType( + "latency", + NumberFieldMapper.NumberType.FLOAT + ); + KeywordFieldMapper.KeywordFieldType regionType = new KeywordFieldMapper.KeywordFieldType("region"); when(mapperService.fieldType("method")).thenReturn(methodType); when(mapperService.fieldType("status")).thenReturn(statusType); when(mapperService.fieldType("port")).thenReturn(portType); when(mapperService.fieldType("zone")).thenReturn(zoneType); when(mapperService.fieldType("response_time")).thenReturn(responseTimeType); + when(mapperService.fieldType("latency")).thenReturn(latencyType); + when(mapperService.fieldType("region")).thenReturn(regionType); // Create composite field type with dimensions compositeFieldType = new StarTreeMapper.StarTreeFieldType( @@ -85,7 +94,9 @@ public void setup() { new OrdinalDimension("status"), new OrdinalDimension("port"), new OrdinalDimension("zone"), - new OrdinalDimension("response_time") + new OrdinalDimension("response_time"), + new OrdinalDimension("latency"), + new OrdinalDimension("region") ), List.of(new Metric("size", List.of(MetricStat.SUM))), new StarTreeFieldConfiguration( @@ -949,6 +960,113 @@ public void testInvalidMustAndFilterCombination() throws IOException { assertNull("Filter should be null for conflicting conditions", filter); } + public void testKeywordRanges() throws IOException { + BoolQueryBuilder keywordRangeQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("region").gte("eu-").lt("eu-z")) // Range of + // region + // codes + .must(new TermQueryBuilder("method", "GET")); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(keywordRangeQuery); + StarTreeFilter filter = provider.getFilter(searchContext, keywordRangeQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + List regionFilters = filter.getFiltersForDimension("region"); + assertTrue(regionFilters.getFirst() instanceof RangeMatchDimFilter); + RangeMatchDimFilter regionRange = (RangeMatchDimFilter) regionFilters.getFirst(); + assertEquals(new BytesRef("eu-"), regionRange.getLow()); + assertEquals(new BytesRef("eu-z"), regionRange.getHigh()); + assertTrue(regionRange.isIncludeLow()); + assertFalse(regionRange.isIncludeHigh()); + } + + public void testFloatRanges() throws IOException { + BoolQueryBuilder floatRangeQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("latency").gte(0.5f).lte(2.0f)) + .must(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(floatRangeQuery); + StarTreeFilter filter = provider.getFilter(searchContext, floatRangeQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + List latencyFilters = filter.getFiltersForDimension("latency"); + assertTrue(latencyFilters.getFirst() instanceof RangeMatchDimFilter); + RangeMatchDimFilter latencyRange = (RangeMatchDimFilter) latencyFilters.getFirst(); + assertEquals(NumericUtils.floatToSortableInt(0.5f), ((Number) latencyRange.getLow()).floatValue(), 0.0001); + assertEquals(NumericUtils.floatToSortableInt(2.0f), ((Number) latencyRange.getHigh()).floatValue(), 0.0001); + assertTrue(latencyRange.isIncludeLow()); + assertTrue(latencyRange.isIncludeHigh()); + + // Test combined ranges in SHOULD + BoolQueryBuilder combinedRangeQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must( + new BoolQueryBuilder().should(new RangeQueryBuilder("latency").gte(0.0).lt(1.0)) + .should(new RangeQueryBuilder("latency").gte(2.0).lt(3.0)) + ); + + filter = provider.getFilter(searchContext, combinedRangeQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + latencyFilters = filter.getFiltersForDimension("latency"); + assertEquals(2, latencyFilters.size()); + for (DimensionFilter dimFilter : latencyFilters) { + assertTrue(dimFilter instanceof RangeMatchDimFilter); + } + } + + public void testFloatRanges_Exclusive() throws IOException { + // Test float range with different inclusivity combinations + BoolQueryBuilder floatRangeQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("latency").gt(0.5).lt(2.0)) // exclusive + // bounds + .must(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(floatRangeQuery); + StarTreeFilter filter = provider.getFilter(searchContext, floatRangeQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + List latencyFilters = filter.getFiltersForDimension("latency"); + assertTrue(latencyFilters.getFirst() instanceof RangeMatchDimFilter); + RangeMatchDimFilter latencyRange = (RangeMatchDimFilter) latencyFilters.getFirst(); + + // For exclusive bounds (gt/lt), we need to use next/previous float values + long expectedLow = NumericUtils.floatToSortableInt(FloatPoint.nextUp(0.5f)); + long expectedHigh = NumericUtils.floatToSortableInt(FloatPoint.nextDown(2.0f)); + + assertEquals(expectedLow, ((Number) latencyRange.getLow()).longValue()); + assertEquals(expectedHigh, ((Number) latencyRange.getHigh()).longValue()); + assertTrue(latencyRange.isIncludeLow()); // After using nextUp, bound becomes inclusive + assertTrue(latencyRange.isIncludeHigh()); // After using nextDown, bound becomes inclusive + } + + public void testKeywordRangeEdgeCases() throws IOException { + // Test unbounded ranges + BoolQueryBuilder unboundedQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("region").gt("eu-")) // No upper bound + .must(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(unboundedQuery); + StarTreeFilter filter = provider.getFilter(searchContext, unboundedQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + List regionFilters = filter.getFiltersForDimension("region"); + RangeMatchDimFilter regionRange = (RangeMatchDimFilter) regionFilters.get(0); + assertEquals(new BytesRef("eu-"), regionRange.getLow()); + assertNull(regionRange.getHigh()); // Unbounded high + assertFalse(regionRange.isIncludeLow()); + assertTrue(regionRange.isIncludeHigh()); + + // Test range intersection + BoolQueryBuilder intersectionQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("region").gte("eu-").lt("eu-z")) + .must(new RangeQueryBuilder("region").gt("eu-a").lte("eu-m")); + + filter = provider.getFilter(searchContext, intersectionQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + regionFilters = filter.getFiltersForDimension("region"); + regionRange = (RangeMatchDimFilter) regionFilters.get(0); + assertEquals(new BytesRef("eu-a"), regionRange.getLow()); + assertEquals(new BytesRef("eu-m"), regionRange.getHigh()); + assertFalse(regionRange.isIncludeLow()); + assertTrue(regionRange.isIncludeHigh()); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); @@ -957,5 +1075,4 @@ private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedVa private void assertExactMatchValue(ExactMatchDimFilter filter, Long expectedValue) { assertEquals(expectedValue, filter.getRawValues().getFirst()); } - } From 8750aa4ecf4d6c3add1385b991aca952f6d53215 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Tue, 22 Apr 2025 17:08:20 +0530 Subject: [PATCH 24/36] Add float intersection test Signed-off-by: Rishab Nahata --- .../BoolStarTreeFilterProviderTests.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 624213c764e7c..96652b5fd7138 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -1036,6 +1036,31 @@ public void testFloatRanges_Exclusive() throws IOException { assertTrue(latencyRange.isIncludeHigh()); // After using nextDown, bound becomes inclusive } + public void testFloatRanges_Intersection() throws IOException { + // Test float range with different inclusivity combinations + BoolQueryBuilder floatRangeQuery = new BoolQueryBuilder() + .must(new RangeQueryBuilder("latency").gt(0.5).lt(2.0)) + .must(new RangeQueryBuilder("latency").gte(0.6).lt(1.8)) + .must(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(floatRangeQuery); + StarTreeFilter filter = provider.getFilter(searchContext, floatRangeQuery, compositeFieldType); + + assertNotNull("Filter should not be null", filter); + List latencyFilters = filter.getFiltersForDimension("latency"); + assertTrue(latencyFilters.getFirst() instanceof RangeMatchDimFilter); + RangeMatchDimFilter latencyRange = (RangeMatchDimFilter) latencyFilters.getFirst(); + + // For exclusive bounds (gt/lt), we need to use next/previous float values + long expectedLow = NumericUtils.floatToSortableInt(0.6f); + long expectedHigh = NumericUtils.floatToSortableInt(FloatPoint.nextDown(1.8f)); + + assertEquals(expectedLow, ((Number) latencyRange.getLow()).longValue()); + assertEquals(expectedHigh, ((Number) latencyRange.getHigh()).longValue()); + assertTrue(latencyRange.isIncludeLow()); + assertTrue(latencyRange.isIncludeHigh()); + } + public void testKeywordRangeEdgeCases() throws IOException { // Test unbounded ranges BoolQueryBuilder unboundedQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("region").gt("eu-")) // No upper bound From 00175f9ca0153d6f975120a6fb6eb5b3b16ac992 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Wed, 23 Apr 2025 01:28:38 +0530 Subject: [PATCH 25/36] Move comparator to DimensionFilterMapper Signed-off-by: Rishab Nahata --- .../filter/DimensionFilterMerger.java | 53 ++++---- .../provider/BoolStarTreeFilterProvider.java | 17 ++- .../provider/DimensionFilterMapper.java | 18 +++ .../BoolStarTreeFilterProviderTests.java | 3 +- .../startree/DimensionFilterMergerTests.java | 114 +++++++++++++----- 5 files changed, 139 insertions(+), 66 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java index dbac29a367fdd..590f66ac3a570 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java @@ -8,7 +8,7 @@ package org.opensearch.search.startree.filter; -import org.apache.lucene.util.BytesRef; +import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import java.util.ArrayList; import java.util.List; @@ -19,7 +19,7 @@ public class DimensionFilterMerger { * Gets intersection of two DimensionFilters * Returns null if intersection results in no possible matches. */ - public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter filter2) { + public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter filter2, DimensionFilterMapper mapper) { if (filter1 == null || filter2 == null) { return null; } @@ -37,7 +37,7 @@ public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter // Handle Range + Range combination if (filter1 instanceof RangeMatchDimFilter && filter2 instanceof RangeMatchDimFilter) { - return intersectRangeFilters((RangeMatchDimFilter) filter1, (RangeMatchDimFilter) filter2); + return intersectRangeFilters((RangeMatchDimFilter) filter1, (RangeMatchDimFilter) filter2, mapper); } // Handle ExactMatch + ExactMatch combination @@ -47,12 +47,12 @@ public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter // Handle Range + ExactMatch combination if (filter1 instanceof RangeMatchDimFilter && filter2 instanceof ExactMatchDimFilter) { - return intersectRangeWithExactMatch((RangeMatchDimFilter) filter1, (ExactMatchDimFilter) filter2); + return intersectRangeWithExactMatch((RangeMatchDimFilter) filter1, (ExactMatchDimFilter) filter2, mapper); } // Handle ExactMatch + Range combination if (filter1 instanceof ExactMatchDimFilter && filter2 instanceof RangeMatchDimFilter) { - return intersectRangeWithExactMatch((RangeMatchDimFilter) filter2, (ExactMatchDimFilter) filter1); + return intersectRangeWithExactMatch((RangeMatchDimFilter) filter2, (ExactMatchDimFilter) filter1, mapper); } // throw exception for unsupported exception @@ -65,7 +65,11 @@ public static DimensionFilter intersect(DimensionFilter filter1, DimensionFilter * Intersects two range filters * Returns null if ranges don't overlap */ - private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, RangeMatchDimFilter range2) { + private static DimensionFilter intersectRangeFilters( + RangeMatchDimFilter range1, + RangeMatchDimFilter range2, + DimensionFilterMapper mapper + ) { Object low1 = range1.getLow(); Object high1 = range1.getHigh(); Object low2 = range2.getLow(); @@ -81,7 +85,7 @@ private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, newLow = low1; includeLow = range1.isIncludeLow(); } else { - int comparison = compareValues(low1, low2); + int comparison = mapper.compareValues(low1, low2); if (comparison > 0) { newLow = low1; includeLow = range1.isIncludeLow(); @@ -103,7 +107,7 @@ private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, newHigh = high1; includeHigh = range1.isIncludeHigh(); } else { - int comparison = compareValues(high1, high2); + int comparison = mapper.compareValues(high1, high2); if (comparison < 0) { newHigh = high1; includeHigh = range1.isIncludeHigh(); @@ -118,7 +122,7 @@ private static DimensionFilter intersectRangeFilters(RangeMatchDimFilter range1, // Check if range is valid if (newLow != null && newHigh != null) { - int comparison = compareValues(newLow, newHigh); + int comparison = mapper.compareValues(newLow, newHigh); if (comparison > 0 || (comparison == 0 && (!includeLow || !includeHigh))) { return null; // No overlap } @@ -153,11 +157,15 @@ private static DimensionFilter intersectExactMatchFilters(ExactMatchDimFilter ex * Intersects a range filter with an exact match filter. * Returns null if no values from exact match are within range. */ - private static DimensionFilter intersectRangeWithExactMatch(RangeMatchDimFilter range, ExactMatchDimFilter exact) { + private static DimensionFilter intersectRangeWithExactMatch( + RangeMatchDimFilter range, + ExactMatchDimFilter exact, + DimensionFilterMapper mapper + ) { List validValues = new ArrayList<>(); for (Object value : exact.getRawValues()) { - if (isValueInRange(value, range)) { + if (isValueInRange(value, range, mapper)) { validValues.add(value); } } @@ -172,37 +180,20 @@ private static DimensionFilter intersectRangeWithExactMatch(RangeMatchDimFilter /** * Checks if a value falls within a range. */ - private static boolean isValueInRange(Object value, RangeMatchDimFilter range) { + private static boolean isValueInRange(Object value, RangeMatchDimFilter range, DimensionFilterMapper mapper) { if (range.getLow() != null) { - int comparison = compareValues(value, range.getLow()); + int comparison = mapper.compareValues(value, range.getLow()); if (comparison < 0 || (comparison == 0 && !range.isIncludeLow())) { return false; } } if (range.getHigh() != null) { - int comparison = compareValues(value, range.getHigh()); + int comparison = mapper.compareValues(value, range.getHigh()); if (comparison > 0 || (comparison == 0 && !range.isIncludeHigh())) { return false; } } return true; } - - /** - * Compares two values - Long for numeric fields and BytesRef for keywords field - */ - private static int compareValues(Object v1, Object v2) { - if (v1 instanceof Long && v2 instanceof Long) { - return Long.compare((Long) v1, (Long) v2); - } - - if (v1 instanceof BytesRef && v2 instanceof BytesRef) { - return ((BytesRef) v1).compareTo((BytesRef) v2); - } - - throw new IllegalArgumentException( - "Can only compare Long or BytesRef values, got types: " + v1.getClass().getName() + " and " + v2.getClass().getName() - ); - } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java index 38ec87a189e69..db3231f91f134 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -99,6 +99,15 @@ private StarTreeFilter processMustClauses( // No existing filters for this dimension dimensionToFilters.put(dimension, new ArrayList<>(newFilters)); } else { + // We have existing filters for this dimension + // Get the appropriate mapper for this dimension + DimensionFilterMapper mapper = DimensionFilterMapper.Factory.fromMappedFieldType( + context.mapperService().fieldType(dimension) + ); + if (mapper == null) { + return null; // Unsupported field type + } + // We have existing filters for this dimension if (newFilters.size() > 1) { // New filters are from SHOULD clause (multiple filters = OR condition) @@ -106,7 +115,7 @@ private StarTreeFilter processMustClauses( List intersectedFilters = new ArrayList<>(); for (DimensionFilter shouldFilter : newFilters) { for (DimensionFilter existingFilter : existingFilters) { - DimensionFilter intersected = DimensionFilterMerger.intersect(existingFilter, shouldFilter); + DimensionFilter intersected = DimensionFilterMerger.intersect(existingFilter, shouldFilter, mapper); if (intersected != null) { intersectedFilters.add(intersected); } @@ -120,7 +129,11 @@ private StarTreeFilter processMustClauses( // Here's where we need the DimensionFilter merging logic // For example: merging range with term, or range with range // And a single dimension filter coming from should clause is as good as must clause - DimensionFilter mergedFilter = DimensionFilterMerger.intersect(existingFilters.getFirst(), newFilters.getFirst()); + DimensionFilter mergedFilter = DimensionFilterMerger.intersect( + existingFilters.getFirst(), + newFilters.getFirst(), + mapper + ); if (mergedFilter == null) { return null; // No possible matches after merging } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java index 8afdb00864b22..2dac7d5f8aee0 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java @@ -88,6 +88,8 @@ Optional getMatchingOrdinal( DimensionFilter.MatchType matchType ); + int compareValues(Object v1, Object v2); + /** * Singleton Factory for @{@link DimensionFilterMapper} */ @@ -134,6 +136,14 @@ public Optional getMatchingOrdinal( // Casting to long ensures that all numeric fields have been converted to equivalent long at request parsing time. return Optional.of((long) value); } + + @Override + public int compareValues(Object v1, Object v2) { + if (!(v1 instanceof Long) || !(v2 instanceof Long)) { + throw new IllegalArgumentException("Expected Long values for numeric comparison"); + } + return Long.compare((Long) v1, (Long) v2); + } } abstract class NumericNonDecimalMapper extends NumericMapper { @@ -407,4 +417,12 @@ private Object parseRawKeyword(String field, Object rawValue, KeywordFieldType k return parsedValue; } + @Override + public int compareValues(Object v1, Object v2) { + if (!(v1 instanceof BytesRef) || !(v2 instanceof BytesRef)) { + throw new IllegalArgumentException("Expected BytesRef values for keyword comparison"); + } + return ((BytesRef) v1).compareTo((BytesRef) v2); + } + } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 96652b5fd7138..8d34d967745e4 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -1038,8 +1038,7 @@ public void testFloatRanges_Exclusive() throws IOException { public void testFloatRanges_Intersection() throws IOException { // Test float range with different inclusivity combinations - BoolQueryBuilder floatRangeQuery = new BoolQueryBuilder() - .must(new RangeQueryBuilder("latency").gt(0.5).lt(2.0)) + BoolQueryBuilder floatRangeQuery = new BoolQueryBuilder().must(new RangeQueryBuilder("latency").gt(0.5).lt(2.0)) .must(new RangeQueryBuilder("latency").gte(0.6).lt(1.8)) .must(new TermQueryBuilder("status", 200)); diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java index 0722a8a302366..82640a3bb0787 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java @@ -8,11 +8,15 @@ package org.opensearch.search.aggregations.startree; +import org.opensearch.index.mapper.KeywordFieldMapper; +import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.search.startree.filter.DimensionFilter; import org.opensearch.search.startree.filter.DimensionFilterMerger; import org.opensearch.search.startree.filter.ExactMatchDimFilter; import org.opensearch.search.startree.filter.RangeMatchDimFilter; +import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import org.opensearch.test.OpenSearchTestCase; +import org.junit.Before; import java.util.Arrays; import java.util.Collections; @@ -21,62 +25,83 @@ public class DimensionFilterMergerTests extends OpenSearchTestCase { + private DimensionFilterMapper numericMapper; + private DimensionFilterMapper keywordMapper; + + @Before + public void setup() { + numericMapper = DimensionFilterMapper.Factory.fromMappedFieldType( + new NumberFieldMapper.NumberFieldType("status", NumberFieldMapper.NumberType.LONG) + ); + keywordMapper = DimensionFilterMapper.Factory.fromMappedFieldType(new KeywordFieldMapper.KeywordFieldType("method")); + } + public void testRangeIntersection() { // Basic range intersection assertRangeIntersection( range("status", 200L, 500L, true, true), range("status", 300L, 400L, true, true), - range("status", 300L, 400L, true, true) + range("status", 300L, 400L, true, true), + numericMapper ); // Boundary conditions assertRangeIntersection( range("status", 200L, 200L, true, true), range("status", 200L, 200L, true, true), - range("status", 200L, 200L, true, true) + range("status", 200L, 200L, true, true), + numericMapper ); // Inclusive/Exclusive boundaries assertRangeIntersection( range("status", 200L, 300L, true, false), range("status", 200L, 300L, false, true), - range("status", 200L, 300L, false, false) + range("status", 200L, 300L, false, false), + numericMapper ); // Non-overlapping ranges - assertNoIntersection(range("status", 200L, 300L, true, true), range("status", 301L, 400L, true, true)); + assertNoIntersection(range("status", 200L, 300L, true, true), range("status", 301L, 400L, true, true), numericMapper); // Exactly touching ranges (no overlap) - assertNoIntersection(range("status", 200L, 300L, true, false), range("status", 300L, 400L, true, true)); + assertNoIntersection(range("status", 200L, 300L, true, false), range("status", 300L, 400L, true, true), numericMapper); // Null bounds (unbounded ranges) assertRangeIntersection( range("status", null, 500L, true, true), range("status", 200L, null, true, true), - range("status", 200L, 500L, true, true) + range("status", 200L, 500L, true, true), + numericMapper ); // Single point overlap assertRangeIntersection( range("status", 200L, 300L, true, true), range("status", 300L, 400L, true, true), - range("status", 300L, 300L, true, true) + range("status", 300L, 300L, true, true), + numericMapper ); // Very large ranges assertRangeIntersection( range("status", Long.MIN_VALUE, Long.MAX_VALUE, true, true), range("status", 200L, 300L, true, true), - range("status", 200L, 300L, true, true) + range("status", 200L, 300L, true, true), + numericMapper ); // Zero-width ranges - assertNoIntersection(range("status", 200L, 200L, true, true), range("status", 200L, 200L, false, false)); + assertNoIntersection(range("status", 200L, 200L, true, true), range("status", 200L, 200L, false, false), numericMapper); // incompatible types assertThrows( IllegalArgumentException.class, - () -> DimensionFilterMerger.intersect(range("status", "200", "300", true, true), range("status", 200, 300, true, true)) + () -> DimensionFilterMerger.intersect( + range("status", "200", "300", true, true), + range("status", 200, 300, true, true), + numericMapper + ) ); } @@ -85,38 +110,46 @@ public void testExactMatchIntersection() { assertExactMatchIntersection( exactMatch("status", List.of(200)), exactMatch("status", List.of(200)), - exactMatch("status", List.of(200)) + exactMatch("status", List.of(200)), + numericMapper ); // Multiple values intersection assertExactMatchIntersection( exactMatch("status", Arrays.asList(200, 300, 400)), exactMatch("status", Arrays.asList(300, 400, 500)), - exactMatch("status", Arrays.asList(300, 400)) + exactMatch("status", Arrays.asList(300, 400)), + numericMapper ); // No intersection - assertNoIntersection(exactMatch("status", List.of(200)), exactMatch("status", List.of(300))); + assertNoIntersection(exactMatch("status", List.of(200)), exactMatch("status", List.of(300)), numericMapper); // Empty list - assertNoIntersection(exactMatch("status", Collections.emptyList()), exactMatch("status", List.of(200))); + assertNoIntersection(exactMatch("status", Collections.emptyList()), exactMatch("status", List.of(200)), numericMapper); // Duplicate values assertExactMatchIntersection( exactMatch("status", Arrays.asList(200, 200, 300)), exactMatch("status", Arrays.asList(200, 300, 300)), - exactMatch("status", Arrays.asList(200, 300)) + exactMatch("status", Arrays.asList(200, 300)), + numericMapper ); // Special characters in string values assertExactMatchIntersection( exactMatch("method", Arrays.asList("GET", "GET*")), exactMatch("method", Arrays.asList("GET", "GET/")), - exactMatch("method", List.of("GET")) + exactMatch("method", List.of("GET")), + keywordMapper ); // Case sensitivity - assertNoIntersection(exactMatch("method", Arrays.asList("GET", "Post")), exactMatch("method", Arrays.asList("get", "POST"))); + assertNoIntersection( + exactMatch("method", Arrays.asList("GET", "Post")), + exactMatch("method", Arrays.asList("get", "POST")), + keywordMapper + ); } public void testRangeExactMatchIntersection() { @@ -124,31 +157,35 @@ public void testRangeExactMatchIntersection() { assertRangeExactMatchIntersection( range("status", 200L, 300L, true, true), exactMatch("status", List.of(250L)), - exactMatch("status", List.of(250L)) + exactMatch("status", List.of(250L)), + numericMapper ); // Value at range boundaries assertRangeExactMatchIntersection( range("status", 200L, 300L, true, true), exactMatch("status", Arrays.asList(200L, 300L)), - exactMatch("status", Arrays.asList(200L, 300L)) + exactMatch("status", Arrays.asList(200L, 300L)), + numericMapper ); // Value at exclusive boundaries assertRangeExactMatchIntersection( range("status", 200L, 300L, false, false), exactMatch("status", Arrays.asList(201L, 299L)), - exactMatch("status", Arrays.asList(201L, 299L)) + exactMatch("status", Arrays.asList(201L, 299L)), + numericMapper ); // No values in range - assertNoIntersection(range("status", 200L, 300L, true, true), exactMatch("status", Arrays.asList(199L, 301L))); + assertNoIntersection(range("status", 200L, 300L, true, true), exactMatch("status", Arrays.asList(199L, 301L)), numericMapper); // Multiple values, some in range assertRangeExactMatchIntersection( range("status", 200L, 300L, true, true), exactMatch("status", Arrays.asList(199L, 200L, 250L, 300L, 301L)), - exactMatch("status", Arrays.asList(200L, 250L, 300L)) + exactMatch("status", Arrays.asList(200L, 250L, 300L)), + numericMapper ); } @@ -156,7 +193,7 @@ public void testDifferentDimensions() { // Cannot intersect different dimensions assertThrows( IllegalArgumentException.class, - () -> DimensionFilterMerger.intersect(range("status", 200, 300, true, true), range("port", 80, 443, true, true)) + () -> DimensionFilterMerger.intersect(range("status", 200, 300, true, true), range("port", 80, 443, true, true), numericMapper) ); } @@ -169,8 +206,13 @@ private ExactMatchDimFilter exactMatch(String dimension, List values) { return new ExactMatchDimFilter(dimension, values); } - private void assertRangeIntersection(RangeMatchDimFilter filter1, RangeMatchDimFilter filter2, RangeMatchDimFilter expected) { - DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2); + private void assertRangeIntersection( + RangeMatchDimFilter filter1, + RangeMatchDimFilter filter2, + RangeMatchDimFilter expected, + DimensionFilterMapper mapper + ) { + DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2, mapper); assertTrue(result instanceof RangeMatchDimFilter); RangeMatchDimFilter rangeResult = (RangeMatchDimFilter) result; assertEquals(expected.getLow(), rangeResult.getLow()); @@ -179,21 +221,31 @@ private void assertRangeIntersection(RangeMatchDimFilter filter1, RangeMatchDimF assertEquals(expected.isIncludeHigh(), rangeResult.isIncludeHigh()); } - private void assertExactMatchIntersection(ExactMatchDimFilter filter1, ExactMatchDimFilter filter2, ExactMatchDimFilter expected) { - DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2); + private void assertExactMatchIntersection( + ExactMatchDimFilter filter1, + ExactMatchDimFilter filter2, + ExactMatchDimFilter expected, + DimensionFilterMapper mapper + ) { + DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2, mapper); assertTrue(result instanceof ExactMatchDimFilter); ExactMatchDimFilter exactResult = (ExactMatchDimFilter) result; assertEquals(new HashSet<>(expected.getRawValues()), new HashSet<>(exactResult.getRawValues())); } - private void assertRangeExactMatchIntersection(RangeMatchDimFilter range, ExactMatchDimFilter exact, ExactMatchDimFilter expected) { - DimensionFilter result = DimensionFilterMerger.intersect(range, exact); + private void assertRangeExactMatchIntersection( + RangeMatchDimFilter range, + ExactMatchDimFilter exact, + ExactMatchDimFilter expected, + DimensionFilterMapper mapper + ) { + DimensionFilter result = DimensionFilterMerger.intersect(range, exact, mapper); assertTrue(result instanceof ExactMatchDimFilter); ExactMatchDimFilter exactResult = (ExactMatchDimFilter) result; assertEquals(new HashSet<>(expected.getRawValues()), new HashSet<>(exactResult.getRawValues())); } - private void assertNoIntersection(DimensionFilter filter1, DimensionFilter filter2) { - assertNull(DimensionFilterMerger.intersect(filter1, filter2)); + private void assertNoIntersection(DimensionFilter filter1, DimensionFilter filter2, DimensionFilterMapper mapper) { + assertNull(DimensionFilterMerger.intersect(filter1, filter2, mapper)); } } From c38e5c6095fb18ef89744dc6de2db9015fb669d1 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 24 Apr 2025 03:02:39 +0530 Subject: [PATCH 26/36] Handle unsigned long Signed-off-by: Rishab Nahata --- .../filter/DimensionFilterMerger.java | 18 +----- .../provider/DimensionFilterMapper.java | 58 +++++++++++++++++++ .../BoolStarTreeFilterProviderTests.java | 4 +- .../startree/DimensionFilterMergerTests.java | 54 +++++++++++++++++ 4 files changed, 116 insertions(+), 18 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java index 590f66ac3a570..38f25b82c59be 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java @@ -122,8 +122,7 @@ private static DimensionFilter intersectRangeFilters( // Check if range is valid if (newLow != null && newHigh != null) { - int comparison = mapper.compareValues(newLow, newHigh); - if (comparison > 0 || (comparison == 0 && (!includeLow || !includeHigh))) { + if (!mapper.isValidRange(newLow, newHigh, includeLow, includeHigh)) { return null; // No overlap } } @@ -181,19 +180,6 @@ private static DimensionFilter intersectRangeWithExactMatch( * Checks if a value falls within a range. */ private static boolean isValueInRange(Object value, RangeMatchDimFilter range, DimensionFilterMapper mapper) { - if (range.getLow() != null) { - int comparison = mapper.compareValues(value, range.getLow()); - if (comparison < 0 || (comparison == 0 && !range.isIncludeLow())) { - return false; - } - } - - if (range.getHigh() != null) { - int comparison = mapper.compareValues(value, range.getHigh()); - if (comparison > 0 || (comparison == 0 && !range.isIncludeHigh())) { - return false; - } - } - return true; + return mapper.isValueInRange(value, range.getLow(), range.getHigh(), range.isIncludeLow(), range.isIncludeHigh()); } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java index a95031d8ed12f..fc5d3dd64058e 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/DimensionFilterMapper.java @@ -92,8 +92,43 @@ Optional getMatchingOrdinal( DimensionFilter.MatchType matchType ); + /** + * Compares two values of the same type. + * @param v1 first object + * @param v2 second object + * @return : + */ int compareValues(Object v1, Object v2); + /** + * Checks if a value falls within a range. + * Default implementation for regular types. + */ + default boolean isValueInRange(Object value, Object low, Object high, boolean includeLow, boolean includeHigh) { + if (low != null) { + int comparison = compareValues(value, low); + if (comparison < 0 || (comparison == 0 && !includeLow)) { + return false; + } + } + + if (high != null) { + int comparison = compareValues(value, high); + if (comparison > 0 || (comparison == 0 && !includeHigh)) { + return false; + } + } + return true; + } + + default boolean isValidRange(Object low, Object high, boolean includeLow, boolean includeHigh) { + if (low == null || high == null) { + return true; + } + int comparison = compareValues(low, high); + return comparison < 0 || (comparison == 0 && includeLow && includeHigh); + } + default Comparator comparator() { return DimensionDataType.LONG::compare; } @@ -245,6 +280,29 @@ public Comparator comparator() { return DimensionDataType.UNSIGNED_LONG::compare; } + @Override + public int compareValues(Object v1, Object v2) { + if (!(v1 instanceof Long) || !(v2 instanceof Long)) { + throw new IllegalArgumentException("Expected Long values for unsigned comparison"); + } + return Long.compareUnsigned((Long) v1, (Long) v2); + } + + @Override + public boolean isValueInRange(Object value, Object low, Object high, boolean includeLow, boolean includeHigh) { + long v = (Long) value; + long l = low != null ? (Long) low : 0L; + long h = high != null ? (Long) high : -1L; // -1L is max unsigned + + if (Long.compareUnsigned(l, h) > 0) { + return (Long.compareUnsigned(v, l) > 0 || (Long.compareUnsigned(v, l) == 0 && includeLow)) + || (Long.compareUnsigned(v, h) < 0 || (Long.compareUnsigned(v, h) == 0 && includeHigh)); + } + + // Normal case + return super.isValueInRange(value, low, high, includeLow, includeHigh); + } + } abstract class NumericDecimalFieldMapper extends NumericMapper { diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 8d34d967745e4..c052baa903e4d 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -441,8 +441,8 @@ public void testShouldInsideShouldSameDimension() throws IOException { assertEquals("Should contain all expected values", expectedValues, actualValues); } - public void testMustInsideShouldRejected() throws IOException { - // MUST inside SHOULD (should be rejected) + public void testMustInsideShouldDifferentDimensionRejected() throws IOException { + // MUST inside SHOULD for different dimension (should be rejected) BoolQueryBuilder boolQuery = new BoolQueryBuilder().should( new BoolQueryBuilder().must(new TermQueryBuilder("status", 200)).must(new TermQueryBuilder("method", "GET")) ); diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java index 82640a3bb0787..08530eb45be63 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java @@ -197,6 +197,60 @@ public void testDifferentDimensions() { ); } + public void testUnsignedLongRangeIntersection() { + // Setup unsigned long mapper + DimensionFilterMapper unsignedLongMapper = DimensionFilterMapper.Factory.fromMappedFieldType( + new NumberFieldMapper.NumberFieldType("unsigned_field", NumberFieldMapper.NumberType.UNSIGNED_LONG) + ); + + // Test case 1: Regular positive values + assertRangeIntersection( + range("unsigned_field", 100L, 500L, true, true), + range("unsigned_field", 200L, 300L, true, true), + range("unsigned_field", 200L, 300L, true, true), + unsignedLongMapper + ); + + // Test case 2: High values (near max unsigned long) + assertRangeIntersection( + range("unsigned_field", -10L, -1L, true, true), // -1L is max unsigned long (2^64 - 1) + range("unsigned_field", -5L, -2L, true, true), + range("unsigned_field", -5L, -2L, true, true), + unsignedLongMapper + ); + + // Test case 3: Crossing the unsigned boundary + assertRangeIntersection( + range("unsigned_field", 2L, -2L, true, true), // -2L is near max unsigned long + range("unsigned_field", 1L, -1L, true, true), // -1L is max unsigned long + range("unsigned_field", 2L, -2L, true, true), + unsignedLongMapper + ); + + // Test case 4: Non-overlapping ranges in unsigned space + assertNoIntersection( + range("unsigned_field", -10L, -5L, true, true), // High unsigned values + range("unsigned_field", 1L, 10L, true, true), // Low unsigned values + unsignedLongMapper + ); + + // Test case 5: Single point intersection at max unsigned value + assertRangeIntersection( + range("unsigned_field", -2L, -1L, true, true), // -1L is max unsigned long + range("unsigned_field", 0L, -1L, true, true), + range("unsigned_field", -2L, -1L, true, true), + unsignedLongMapper + ); + + // Test case 6: Full range + assertRangeIntersection( + range("unsigned_field", 0L, -1L, true, true), // 0 to max unsigned + range("unsigned_field", 100L, 200L, true, true), + range("unsigned_field", 100L, 200L, true, true), + unsignedLongMapper + ); + } + // Helper methods private RangeMatchDimFilter range(String dimension, Object low, Object high, boolean includeLow, boolean includeHigh) { return new RangeMatchDimFilter(dimension, low, high, includeLow, includeHigh); From e35753bd4813ecd0cd8016cc95d0e906939d64d3 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 24 Apr 2025 03:41:06 +0530 Subject: [PATCH 27/36] Handle min should match Signed-off-by: Rishab Nahata --- .../provider/BoolStarTreeFilterProvider.java | 3 +++ .../BoolStarTreeFilterProviderTests.java | 24 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java index db3231f91f134..071f968472f4f 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -41,6 +41,9 @@ private StarTreeFilter processBoolQuery( if (boolQuery.hasClauses() == false) { return null; } + if (boolQuery.minimumShouldMatch() != null) { + return null; // We cannot support this yet and would need special handling while processing SHOULD clause + } if (boolQuery.must().isEmpty() == false || boolQuery.filter().isEmpty() == false) { return processMustClauses(getCombinedMustAndFilterClauses(boolQuery), context, compositeFieldType); } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index c052baa903e4d..e330b5d0abd9f 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -1091,6 +1091,30 @@ public void testKeywordRangeEdgeCases() throws IOException { assertTrue(regionRange.isIncludeHigh()); } + public void testMinimumShouldMatch() throws IOException { + // Test with minimum_should_match set + BoolQueryBuilder boolQuery = new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("status", 404)) + .minimumShouldMatch(2); // Explicitly set minimum_should_match + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + assertNull("Filter should be null when minimum_should_match is set", filter); + + // Test nested bool with minimum_should_match + BoolQueryBuilder nestedBoolQuery = new BoolQueryBuilder() + .must(new TermQueryBuilder("method", "GET")) + .must(new BoolQueryBuilder() + .should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("status", 404)) + .minimumShouldMatch(1)); // Set in nested bool + + filter = provider.getFilter(searchContext, nestedBoolQuery, compositeFieldType); + assertNull("Filter should be null when minimum_should_match is set in nested query", filter); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); From 761a2c5792acfc6482cea09fc17c623507a2cbbc Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 24 Apr 2025 03:44:37 +0530 Subject: [PATCH 28/36] Add Changelog Signed-off-by: Rishab Nahata --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc027b7c7b101..d9f02456b65fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Unset discovery nodes for every transport node actions request ([#17682](https://github.com/opensearch-project/OpenSearch/pull/17682)) - Implement parallel shard refresh behind cluster settings ([#17782](https://github.com/opensearch-project/OpenSearch/pull/17782)) - Bump OpenSearch Core main branch to 3.0.0 ([#18039](https://github.com/opensearch-project/OpenSearch/pull/18039)) +- [Star Tree] Support of Boolean Queries in Aggregations ([#17941](https://github.com/opensearch-project/OpenSearch/pull/17941)) ### Changed - Change the default max header size from 8KB to 16KB. ([#18024](https://github.com/opensearch-project/OpenSearch/pull/18024)) From 67ea84290dc82a90d44cc0c19ba98e9dfe43feca Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 24 Apr 2025 03:49:51 +0530 Subject: [PATCH 29/36] Fix spotless check Signed-off-by: Rishab Nahata --- .../startree/BoolStarTreeFilterProviderTests.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index e330b5d0abd9f..368c5e0befd7b 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -1093,8 +1093,7 @@ public void testKeywordRangeEdgeCases() throws IOException { public void testMinimumShouldMatch() throws IOException { // Test with minimum_should_match set - BoolQueryBuilder boolQuery = new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) + BoolQueryBuilder boolQuery = new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) .should(new TermQueryBuilder("status", 404)) .minimumShouldMatch(2); // Explicitly set minimum_should_match @@ -1104,12 +1103,12 @@ public void testMinimumShouldMatch() throws IOException { assertNull("Filter should be null when minimum_should_match is set", filter); // Test nested bool with minimum_should_match - BoolQueryBuilder nestedBoolQuery = new BoolQueryBuilder() - .must(new TermQueryBuilder("method", "GET")) - .must(new BoolQueryBuilder() - .should(new TermQueryBuilder("status", 200)) - .should(new TermQueryBuilder("status", 404)) - .minimumShouldMatch(1)); // Set in nested bool + BoolQueryBuilder nestedBoolQuery = new BoolQueryBuilder().must(new TermQueryBuilder("method", "GET")) + .must( + new BoolQueryBuilder().should(new TermQueryBuilder("status", 200)) + .should(new TermQueryBuilder("status", 404)) + .minimumShouldMatch(1) + ); // Set in nested bool filter = provider.getFilter(searchContext, nestedBoolQuery, compositeFieldType); assertNull("Filter should be null when minimum_should_match is set in nested query", filter); From 80cc481b9fb0de07f014f4026516e18ba740d927 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 24 Apr 2025 03:52:15 +0530 Subject: [PATCH 30/36] Revert ff change Signed-off-by: Rishab Nahata --- .../src/main/java/org/opensearch/common/util/FeatureFlags.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 34fea4aaaae79..5633fe91d51a9 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -114,7 +114,7 @@ public class FeatureFlags { * aggregations. */ public static final String STAR_TREE_INDEX = FEATURE_FLAG_PREFIX + "composite_index.star_tree.enabled"; - public static final Setting STAR_TREE_INDEX_SETTING = Setting.boolSetting(STAR_TREE_INDEX, true, Property.NodeScope); + public static final Setting STAR_TREE_INDEX_SETTING = Setting.boolSetting(STAR_TREE_INDEX, false, Property.NodeScope); /** * Gates the functionality of application based configuration templates. From 4ad00400c64d94e874bfe22b100a9b2a447d6dd0 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Thu, 24 Apr 2025 14:10:06 +0530 Subject: [PATCH 31/36] Add java doc Signed-off-by: Rishab Nahata --- .../search/startree/filter/DimensionFilterMerger.java | 4 ++++ .../startree/filter/provider/BoolStarTreeFilterProvider.java | 3 +++ 2 files changed, 7 insertions(+) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java index 38f25b82c59be..6efa895c1014d 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java @@ -13,6 +13,10 @@ import java.util.ArrayList; import java.util.List; +/** + * Utility class for merging different types of {@link DimensionFilter} + * Handles intersection operations between {@link ExactMatchDimFilter} and {@link RangeMatchDimFilter} + */ public class DimensionFilterMerger { /** diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java index 071f968472f4f..15598af793148 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -26,6 +26,9 @@ import java.util.List; import java.util.Map; +/** + * Converts {@link BoolQueryBuilder} into {@link StarTreeFilter} + */ public class BoolStarTreeFilterProvider implements StarTreeFilterProvider { @Override public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) From 02452fbac6fb923e1d2dc4d1c71f95ba15d795c4 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Fri, 25 Apr 2025 00:25:47 +0530 Subject: [PATCH 32/36] Rename merger class Signed-off-by: Rishab Nahata --- ...rger.java => DimensionFilterMergerUtils.java} | 2 +- .../provider/BoolStarTreeFilterProvider.java | 6 +++--- ...java => DimensionFilterMergerUtilsTests.java} | 16 ++++++++-------- 3 files changed, 12 insertions(+), 12 deletions(-) rename server/src/main/java/org/opensearch/search/startree/filter/{DimensionFilterMerger.java => DimensionFilterMergerUtils.java} (99%) rename server/src/test/java/org/opensearch/search/aggregations/startree/{DimensionFilterMergerTests.java => DimensionFilterMergerUtilsTests.java} (94%) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMergerUtils.java similarity index 99% rename from server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java rename to server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMergerUtils.java index 6efa895c1014d..492ed7924103b 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMerger.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMergerUtils.java @@ -17,7 +17,7 @@ * Utility class for merging different types of {@link DimensionFilter} * Handles intersection operations between {@link ExactMatchDimFilter} and {@link RangeMatchDimFilter} */ -public class DimensionFilterMerger { +public class DimensionFilterMergerUtils { /** * Gets intersection of two DimensionFilters diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java index 15598af793148..e2e05d0d586c8 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -16,7 +16,7 @@ import org.opensearch.index.query.TermsQueryBuilder; import org.opensearch.search.internal.SearchContext; import org.opensearch.search.startree.filter.DimensionFilter; -import org.opensearch.search.startree.filter.DimensionFilterMerger; +import org.opensearch.search.startree.filter.DimensionFilterMergerUtils; import org.opensearch.search.startree.filter.StarTreeFilter; import java.io.IOException; @@ -121,7 +121,7 @@ private StarTreeFilter processMustClauses( List intersectedFilters = new ArrayList<>(); for (DimensionFilter shouldFilter : newFilters) { for (DimensionFilter existingFilter : existingFilters) { - DimensionFilter intersected = DimensionFilterMerger.intersect(existingFilter, shouldFilter, mapper); + DimensionFilter intersected = DimensionFilterMergerUtils.intersect(existingFilter, shouldFilter, mapper); if (intersected != null) { intersectedFilters.add(intersected); } @@ -135,7 +135,7 @@ private StarTreeFilter processMustClauses( // Here's where we need the DimensionFilter merging logic // For example: merging range with term, or range with range // And a single dimension filter coming from should clause is as good as must clause - DimensionFilter mergedFilter = DimensionFilterMerger.intersect( + DimensionFilter mergedFilter = DimensionFilterMergerUtils.intersect( existingFilters.getFirst(), newFilters.getFirst(), mapper diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java similarity index 94% rename from server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java rename to server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java index 08530eb45be63..4abe4d190927f 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java @@ -11,7 +11,7 @@ import org.opensearch.index.mapper.KeywordFieldMapper; import org.opensearch.index.mapper.NumberFieldMapper; import org.opensearch.search.startree.filter.DimensionFilter; -import org.opensearch.search.startree.filter.DimensionFilterMerger; +import org.opensearch.search.startree.filter.DimensionFilterMergerUtils; import org.opensearch.search.startree.filter.ExactMatchDimFilter; import org.opensearch.search.startree.filter.RangeMatchDimFilter; import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; @@ -23,7 +23,7 @@ import java.util.HashSet; import java.util.List; -public class DimensionFilterMergerTests extends OpenSearchTestCase { +public class DimensionFilterMergerUtilsTests extends OpenSearchTestCase { private DimensionFilterMapper numericMapper; private DimensionFilterMapper keywordMapper; @@ -97,7 +97,7 @@ public void testRangeIntersection() { // incompatible types assertThrows( IllegalArgumentException.class, - () -> DimensionFilterMerger.intersect( + () -> DimensionFilterMergerUtils.intersect( range("status", "200", "300", true, true), range("status", 200, 300, true, true), numericMapper @@ -193,7 +193,7 @@ public void testDifferentDimensions() { // Cannot intersect different dimensions assertThrows( IllegalArgumentException.class, - () -> DimensionFilterMerger.intersect(range("status", 200, 300, true, true), range("port", 80, 443, true, true), numericMapper) + () -> DimensionFilterMergerUtils.intersect(range("status", 200, 300, true, true), range("port", 80, 443, true, true), numericMapper) ); } @@ -266,7 +266,7 @@ private void assertRangeIntersection( RangeMatchDimFilter expected, DimensionFilterMapper mapper ) { - DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2, mapper); + DimensionFilter result = DimensionFilterMergerUtils.intersect(filter1, filter2, mapper); assertTrue(result instanceof RangeMatchDimFilter); RangeMatchDimFilter rangeResult = (RangeMatchDimFilter) result; assertEquals(expected.getLow(), rangeResult.getLow()); @@ -281,7 +281,7 @@ private void assertExactMatchIntersection( ExactMatchDimFilter expected, DimensionFilterMapper mapper ) { - DimensionFilter result = DimensionFilterMerger.intersect(filter1, filter2, mapper); + DimensionFilter result = DimensionFilterMergerUtils.intersect(filter1, filter2, mapper); assertTrue(result instanceof ExactMatchDimFilter); ExactMatchDimFilter exactResult = (ExactMatchDimFilter) result; assertEquals(new HashSet<>(expected.getRawValues()), new HashSet<>(exactResult.getRawValues())); @@ -293,13 +293,13 @@ private void assertRangeExactMatchIntersection( ExactMatchDimFilter expected, DimensionFilterMapper mapper ) { - DimensionFilter result = DimensionFilterMerger.intersect(range, exact, mapper); + DimensionFilter result = DimensionFilterMergerUtils.intersect(range, exact, mapper); assertTrue(result instanceof ExactMatchDimFilter); ExactMatchDimFilter exactResult = (ExactMatchDimFilter) result; assertEquals(new HashSet<>(expected.getRawValues()), new HashSet<>(exactResult.getRawValues())); } private void assertNoIntersection(DimensionFilter filter1, DimensionFilter filter2, DimensionFilterMapper mapper) { - assertNull(DimensionFilterMerger.intersect(filter1, filter2, mapper)); + assertNull(DimensionFilterMergerUtils.intersect(filter1, filter2, mapper)); } } From 65bf04381d575ebdaa5c104e978e08326615b9ca Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Fri, 25 Apr 2025 12:54:34 +0530 Subject: [PATCH 33/36] Fix spotless check Signed-off-by: Rishab Nahata --- .../startree/DimensionFilterMergerUtilsTests.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java index 4abe4d190927f..15fe0920cc2bd 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java @@ -193,7 +193,11 @@ public void testDifferentDimensions() { // Cannot intersect different dimensions assertThrows( IllegalArgumentException.class, - () -> DimensionFilterMergerUtils.intersect(range("status", 200, 300, true, true), range("port", 80, 443, true, true), numericMapper) + () -> DimensionFilterMergerUtils.intersect( + range("status", 200, 300, true, true), + range("port", 80, 443, true, true), + numericMapper + ) ); } From fa3382c9371d1ee4ddb12c5d4b530ff2035d363e Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Fri, 25 Apr 2025 14:27:43 +0530 Subject: [PATCH 34/36] Refactor Signed-off-by: Rishab Nahata --- .../filter/provider/BoolStarTreeFilterProvider.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java index e2e05d0d586c8..315999b0d78d5 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/provider/BoolStarTreeFilterProvider.java @@ -25,11 +25,19 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; /** * Converts {@link BoolQueryBuilder} into {@link StarTreeFilter} */ public class BoolStarTreeFilterProvider implements StarTreeFilterProvider { + + private static final Set> SUPPORTED_NON_BOOL_QUERIES = Set.of( + TermQueryBuilder.class, + TermsQueryBuilder.class, + RangeQueryBuilder.class + ); + @Override public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, CompositeDataCubeFieldType compositeFieldType) throws IOException { @@ -62,7 +70,7 @@ private StarTreeFilter processNonBoolSupportedQueries( CompositeDataCubeFieldType compositeFieldType ) throws IOException { // Only allow other supported QueryBuilders - if (!(query instanceof TermQueryBuilder) && !(query instanceof TermsQueryBuilder) && !(query instanceof RangeQueryBuilder)) { + if (SUPPORTED_NON_BOOL_QUERIES.contains(query.getClass()) == false) { return null; } // Process individual clause From 4cbf84685ce6750675cbd15955a1ce24b5470217 Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Mon, 28 Apr 2025 10:47:47 +0530 Subject: [PATCH 35/36] Add tests for null checks Signed-off-by: Rishab Nahata --- .../DimensionFilterMergerUtilsTests.java | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java index 15fe0920cc2bd..48369bb112bd3 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java @@ -255,6 +255,51 @@ public void testUnsignedLongRangeIntersection() { ); } + public void testIntersectValidation() { + // Test null filters + assertNull( + "Should return null for null first filter", + DimensionFilterMergerUtils.intersect(null, exactMatch("status", List.of(200L)), numericMapper) + ); + assertNull( + "Should return null for null second filter", + DimensionFilterMergerUtils.intersect(exactMatch("status", List.of(200L)), null, numericMapper) + ); + assertNull("Should return null for both null filters", DimensionFilterMergerUtils.intersect(null, null, numericMapper)); + + // Test null dimension names + DimensionFilter nullDimFilter1 = new ExactMatchDimFilter(null, List.of(200L)); + DimensionFilter nullDimFilter2 = new ExactMatchDimFilter(null, List.of(300L)); + IllegalArgumentException e1 = assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMergerUtils.intersect(nullDimFilter1, exactMatch("status", List.of(200L)), numericMapper) + ); + assertEquals("Cannot intersect filters with null dimension name", e1.getMessage()); + + IllegalArgumentException e2 = assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMergerUtils.intersect(exactMatch("status", List.of(200L)), nullDimFilter2, numericMapper) + ); + assertEquals("Cannot intersect filters with null dimension name", e2.getMessage()); + + IllegalArgumentException e3 = assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMergerUtils.intersect(nullDimFilter1, nullDimFilter2, numericMapper) + ); + assertEquals("Cannot intersect filters with null dimension name", e3.getMessage()); + + // Test different dimensions + IllegalArgumentException e4 = assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMergerUtils.intersect( + exactMatch("status", List.of(200L)), + exactMatch("method", List.of("GET")), + numericMapper + ) + ); + assertEquals("Cannot intersect filters for different dimensions: status and method", e4.getMessage()); + } + // Helper methods private RangeMatchDimFilter range(String dimension, Object low, Object high, boolean includeLow, boolean includeHigh) { return new RangeMatchDimFilter(dimension, low, high, includeLow, includeHigh); From 63b786d989ba2bb13ff0be1e723d04255acd991f Mon Sep 17 00:00:00 2001 From: Rishab Nahata Date: Mon, 28 Apr 2025 11:44:01 +0530 Subject: [PATCH 36/36] Minor changes and add tests Signed-off-by: Rishab Nahata --- .../filter/DimensionFilterMergerUtils.java | 6 ++- .../BoolStarTreeFilterProviderTests.java | 10 ++++ .../DimensionFilterMergerUtilsTests.java | 47 +++++++++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMergerUtils.java b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMergerUtils.java index 492ed7924103b..4ee0be7c05835 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMergerUtils.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/DimensionFilterMergerUtils.java @@ -11,7 +11,9 @@ import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; /** * Utility class for merging different types of {@link DimensionFilter} @@ -140,11 +142,11 @@ private static DimensionFilter intersectRangeFilters( */ private static DimensionFilter intersectExactMatchFilters(ExactMatchDimFilter exact1, ExactMatchDimFilter exact2) { List values1 = exact1.getRawValues(); - List values2 = exact2.getRawValues(); + Set values2Set = new HashSet<>(exact2.getRawValues()); List intersection = new ArrayList<>(); for (Object value : values1) { - if (values2.contains(value)) { + if (values2Set.contains(value)) { intersection.add(value); } } diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java index 368c5e0befd7b..631033a15684e 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/BoolStarTreeFilterProviderTests.java @@ -1114,6 +1114,16 @@ public void testMinimumShouldMatch() throws IOException { assertNull("Filter should be null when minimum_should_match is set in nested query", filter); } + public void testMustNotClauseReturnsNull() throws IOException { + BoolQueryBuilder boolQuery = new BoolQueryBuilder().mustNot(new TermQueryBuilder("status", 200)); + + StarTreeFilterProvider provider = StarTreeFilterProvider.SingletonFactory.getProvider(boolQuery); + StarTreeFilter filter = provider.getFilter(searchContext, boolQuery, compositeFieldType); + + // this should return null as must not clause is not supported + assertNull("Filter should be null for same dimension in MUST", filter); + } + // Helper methods for assertions private void assertExactMatchValue(ExactMatchDimFilter filter, String expectedValue) { assertEquals(new BytesRef(expectedValue), filter.getRawValues().getFirst()); diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java index 48369bb112bd3..cfaf5823428e1 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/DimensionFilterMergerUtilsTests.java @@ -8,8 +8,12 @@ package org.opensearch.search.aggregations.startree; +import org.opensearch.index.compositeindex.datacube.startree.index.StarTreeValues; +import org.opensearch.index.compositeindex.datacube.startree.node.StarTreeNode; import org.opensearch.index.mapper.KeywordFieldMapper; import org.opensearch.index.mapper.NumberFieldMapper; +import org.opensearch.search.internal.SearchContext; +import org.opensearch.search.startree.StarTreeNodeCollector; import org.opensearch.search.startree.filter.DimensionFilter; import org.opensearch.search.startree.filter.DimensionFilterMergerUtils; import org.opensearch.search.startree.filter.ExactMatchDimFilter; @@ -74,6 +78,12 @@ public void testRangeIntersection() { range("status", 200L, 500L, true, true), numericMapper ); + assertRangeIntersection( + range("status", 200L, null, true, true), + range("status", null, 500L, true, true), + range("status", 200L, 500L, true, true), + numericMapper + ); // Single point overlap assertRangeIntersection( @@ -300,6 +310,43 @@ public void testIntersectValidation() { assertEquals("Cannot intersect filters for different dimensions: status and method", e4.getMessage()); } + public void testUnsupportedFilterCombination() { + // Create a custom filter type for testing + class CustomDimensionFilter implements DimensionFilter { + @Override + public String getDimensionName() { + return "status"; + } + + @Override + public void initialiseForSegment(StarTreeValues starTreeValues, SearchContext searchContext) {} + + @Override + public void matchStarTreeNodes(StarTreeNode parentNode, StarTreeValues starTreeValues, StarTreeNodeCollector collector) {} + + @Override + public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { + return false; + } + } + + DimensionFilter customFilter = new CustomDimensionFilter(); + + // Test unsupported combination with ExactMatchDimFilter + IllegalArgumentException e1 = assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMergerUtils.intersect(customFilter, exactMatch("status", List.of(200L)), numericMapper) + ); + assertEquals("Unsupported filter combination: CustomDimensionFilter and ExactMatchDimFilter", e1.getMessage()); + + // Test unsupported combination with RangeMatchDimFilter + IllegalArgumentException e2 = assertThrows( + IllegalArgumentException.class, + () -> DimensionFilterMergerUtils.intersect(range("status", 200L, 300L, true, true), customFilter, numericMapper) + ); + assertEquals("Unsupported filter combination: RangeMatchDimFilter and CustomDimensionFilter", e2.getMessage()); + } + // Helper methods private RangeMatchDimFilter range(String dimension, Object low, Object high, boolean includeLow, boolean includeHigh) { return new RangeMatchDimFilter(dimension, low, high, includeLow, includeHigh);