From 629866d92d13b4bf57fa79b775ae1d7c6a1bc2f1 Mon Sep 17 00:00:00 2001 From: expani Date: Mon, 27 Jan 2025 08:15:03 -0800 Subject: [PATCH] Removed equals and hashCode from filters and changed back to using List Signed-off-by: expani --- .../startree/StarTreeTraversalUtil.java | 4 +- .../startree/filter/ExactMatchDimFilter.java | 19 +------ .../startree/filter/RangeMatchDimFilter.java | 16 ------ .../startree/filter/StarTreeFilter.java | 20 ++----- .../provider/StarTreeFilterProvider.java | 10 ++-- .../DimensionFilterAndMapperTests.java | 53 ------------------- .../startree/MetricAggregatorTests.java | 2 +- .../startree/StarTreeQueryTests.java | 21 ++++---- 8 files changed, 25 insertions(+), 120 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/startree/StarTreeTraversalUtil.java b/server/src/main/java/org/opensearch/search/startree/StarTreeTraversalUtil.java index a82216b231322..eb7ef3fbb489a 100644 --- a/server/src/main/java/org/opensearch/search/startree/StarTreeTraversalUtil.java +++ b/server/src/main/java/org/opensearch/search/startree/StarTreeTraversalUtil.java @@ -82,7 +82,7 @@ public static FixedBitSet getStarTreeResult(StarTreeValues starTreeValues, StarT StarTreeValuesIterator valuesIterator = starTreeValues.getDimensionValuesIterator(remainingPredicateColumn); // Get the query value directly - Set dimensionFilters = starTreeFilter.getFiltersForDimension(remainingPredicateColumn); + List dimensionFilters = starTreeFilter.getFiltersForDimension(remainingPredicateColumn); // Clear the temporary bit set before reuse tempBitSet.clear(0, starTreeResult.maxMatchedDoc + 1); @@ -167,7 +167,7 @@ private static StarTreeResult traverseStarTree(StarTreeValues starTreeValues, St } if (remainingPredicateColumns.contains(childDimension)) { - Set dimensionFilters = starTreeFilter.getFiltersForDimension(childDimension); + List dimensionFilters = starTreeFilter.getFiltersForDimension(childDimension); final boolean[] tempFoundLeafNodes = new boolean[1]; for (DimensionFilter dimensionFilter : dimensionFilters) { dimensionFilter.matchStarTreeNodes(starTreeNode, starTreeValues, node -> { 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 db1d451976f94..f0c07f011e264 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 @@ -18,11 +18,8 @@ import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import java.io.IOException; -import java.util.HashSet; import java.util.List; -import java.util.Objects; import java.util.Optional; -import java.util.Set; import java.util.TreeSet; /** @@ -33,14 +30,14 @@ public class ExactMatchDimFilter implements DimensionFilter { private final String dimensionName; - private final Set rawValues; + private final List rawValues; // Order is essential for successive binary search private TreeSet convertedOrdinals; public ExactMatchDimFilter(String dimensionName, List valuesToMatch) { this.dimensionName = dimensionName; - this.rawValues = new HashSet<>(valuesToMatch); + this.rawValues = valuesToMatch; } @Override @@ -80,18 +77,6 @@ public void matchStarTreeNodes(StarTreeNode parentNode, StarTreeValues starTreeV } } - @Override - public boolean equals(Object o) { - if (!(o instanceof ExactMatchDimFilter)) return false; - ExactMatchDimFilter that = (ExactMatchDimFilter) o; - return Objects.equals(dimensionName, that.dimensionName) && Objects.equals(rawValues, that.rawValues); - } - - @Override - public int hashCode() { - return Objects.hash(dimensionName, rawValues); - } - @Override public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return convertedOrdinals.contains(ordinal); 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 dd9c5bf0eeebb..fecf1a9ebf76b 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 @@ -16,7 +16,6 @@ import org.opensearch.search.startree.filter.provider.DimensionFilterMapper; import java.io.IOException; -import java.util.Objects; import java.util.Optional; /** @@ -87,19 +86,4 @@ public boolean matchDimValue(long ordinal, StarTreeValues starTreeValues) { return lowOrdinal <= ordinal && ordinal <= highOrdinal; } - @Override - public boolean equals(Object o) { - if (!(o instanceof RangeMatchDimFilter)) return false; - RangeMatchDimFilter that = (RangeMatchDimFilter) o; - return includeLow == that.includeLow - && includeHigh == that.includeHigh - && Objects.equals(dimensionName, that.dimensionName) - && Objects.equals(low, that.low) - && Objects.equals(high, that.high); - } - - @Override - public int hashCode() { - return Objects.hash(dimensionName, low, high, includeLow, includeHigh); - } } diff --git a/server/src/main/java/org/opensearch/search/startree/filter/StarTreeFilter.java b/server/src/main/java/org/opensearch/search/startree/filter/StarTreeFilter.java index 9536f02499813..38a1f092adc6f 100644 --- a/server/src/main/java/org/opensearch/search/startree/filter/StarTreeFilter.java +++ b/server/src/main/java/org/opensearch/search/startree/filter/StarTreeFilter.java @@ -10,8 +10,8 @@ import org.opensearch.common.annotation.ExperimentalApi; +import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; /** @@ -20,31 +20,19 @@ @ExperimentalApi public class StarTreeFilter { - private final Map> dimensionFilterMap; + private final Map> dimensionFilterMap; - public StarTreeFilter(Map> dimensionFilterMap) { + public StarTreeFilter(Map> dimensionFilterMap) { this.dimensionFilterMap = dimensionFilterMap; } - public Set getFiltersForDimension(String dimension) { + public List getFiltersForDimension(String dimension) { return dimensionFilterMap.get(dimension); } public Set getDimensions() { return dimensionFilterMap.keySet(); } - - @Override - public boolean equals(Object o) { - if (!(o instanceof StarTreeFilter)) return false; - StarTreeFilter that = (StarTreeFilter) o; - return Objects.equals(dimensionFilterMap, that.dimensionFilterMap); - } - - @Override - public int hashCode() { - return Objects.hashCode(dimensionFilterMap); - } // TODO : Implement Merging of 2 Star Tree Filters // This would also involve merging 2 different types of dimension filters. // It also brings in the challenge of sorting input values in user query for efficient merging. 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 33a9113dbb615..9932c974cefd6 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 @@ -27,7 +27,6 @@ import java.util.Collections; 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} @@ -95,7 +94,10 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C return new StarTreeFilter(Collections.emptyMap()); } else { return new StarTreeFilter( - Map.of(field, Set.of(dimensionFilterMapper.getExactMatchFilter(mappedFieldType, List.of(termQueryBuilder.value())))) + Map.of( + field, + List.of(dimensionFilterMapper.getExactMatchFilter(mappedFieldType, List.of(termQueryBuilder.value()))) + ) ); } } @@ -122,7 +124,7 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C return new StarTreeFilter(Collections.emptyMap()); } else { return new StarTreeFilter( - Map.of(field, Set.of(dimensionFilterMapper.getExactMatchFilter(mappedFieldType, termsQueryBuilder.values()))) + Map.of(field, List.of(dimensionFilterMapper.getExactMatchFilter(mappedFieldType, termsQueryBuilder.values()))) ); } } @@ -152,7 +154,7 @@ public StarTreeFilter getFilter(SearchContext context, QueryBuilder rawFilter, C return new StarTreeFilter( Map.of( field, - Set.of( + List.of( dimensionFilterMapper.getRangeMatchFilter( mappedFieldType, rangeQueryBuilder.from(), 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 e8bd62b10a3b5..824e2b6c36d81 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 @@ -14,17 +14,11 @@ import org.opensearch.index.compositeindex.datacube.startree.utils.iterator.SortedSetStarTreeValuesIterator; import org.opensearch.index.mapper.KeywordFieldMapper; import org.opensearch.search.startree.filter.DimensionFilter.MatchType; -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.DimensionFilterMapper; import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; -import java.util.List; -import java.util.Map; import java.util.Optional; -import java.util.Set; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -115,51 +109,4 @@ public void testKeywordOrdinalMapping() throws IOException { } - public void testFilterComparison() { - - // Range Filter Same - RangeMatchDimFilter rangeFilter1 = new RangeMatchDimFilter("field", 12, 14, true, false); - RangeMatchDimFilter rangeFilter2 = new RangeMatchDimFilter("field", 12, 14, true, false); - assertEquals(rangeFilter1, rangeFilter2); - - // Range Filter Different - rangeFilter2 = new RangeMatchDimFilter("field", 12, 14, true, true); - assertNotEquals(rangeFilter1, rangeFilter2); - rangeFilter2 = new RangeMatchDimFilter("field", 12, 15, true, false); - assertNotEquals(rangeFilter1, rangeFilter2); - rangeFilter2 = new RangeMatchDimFilter("fields", 12, 14, true, false); - assertNotEquals(rangeFilter1, rangeFilter2); - rangeFilter2 = new RangeMatchDimFilter("field", 12, 14, false, false); - assertNotEquals(rangeFilter1, rangeFilter2); - rangeFilter2 = new RangeMatchDimFilter("field", 11, 14, true, false); - assertNotEquals(rangeFilter1, rangeFilter2); - - // ExactMatch Filter Same - ExactMatchDimFilter exactFilter1 = new ExactMatchDimFilter("field", List.of("hello", "world")); - ExactMatchDimFilter exactFilter2 = new ExactMatchDimFilter("field", List.of("world", "hello")); - assertEquals(exactFilter1, exactFilter2); - - // ExactMatch Filter Different - exactFilter2 = new ExactMatchDimFilter("field2", List.of("hello", "world")); - assertNotEquals(exactFilter1, exactFilter2); - exactFilter2 = new ExactMatchDimFilter("field", List.of("hello", "worlds")); - assertNotEquals(exactFilter1, exactFilter2); - - // Same StarTreeFilter - StarTreeFilter starTreeFilter1 = new StarTreeFilter(Map.of("field", Set.of(rangeFilter1))); - StarTreeFilter starTreeFilter2 = new StarTreeFilter(Map.of("field", Set.of(rangeFilter1))); - assertEquals(starTreeFilter1, starTreeFilter2); - rangeFilter2 = new RangeMatchDimFilter("field", 12, 14, true, false); - starTreeFilter2 = new StarTreeFilter(Map.of("field", Set.of(rangeFilter2))); - assertEquals(starTreeFilter1, starTreeFilter2); - - // Different StarTreeFilter - starTreeFilter2 = new StarTreeFilter(Map.of("field1", Set.of(rangeFilter2))); - assertNotEquals(starTreeFilter1, starTreeFilter2); - rangeFilter2 = new RangeMatchDimFilter("field", 12, 15, true, false); - starTreeFilter2 = new StarTreeFilter(Map.of("field", Set.of(rangeFilter2))); - assertNotEquals(starTreeFilter1, starTreeFilter2); - - } - } 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 f7d2b95b688e1..20f682899f136 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 @@ -395,7 +395,7 @@ private void testStarTreeDocValuesInternal(Codec codec, List // Keyword Range query with missing Low Ordinal RangeQueryBuilder rangeQueryBuilder = new RangeQueryBuilder("keyword_field"); - rangeQueryBuilder.from("non_existing_value_here").includeLower(random().nextBoolean()); + rangeQueryBuilder.from(Long.MAX_VALUE).includeLower(random().nextBoolean()); testCase( indexSearcher, rangeQueryBuilder.toQuery(queryShardContext), diff --git a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeQueryTests.java b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeQueryTests.java index 7e2b2866fd34e..cfa619e8f4592 100644 --- a/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeQueryTests.java +++ b/server/src/test/java/org/opensearch/search/aggregations/startree/StarTreeQueryTests.java @@ -52,7 +52,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Set; import org.mockito.Mockito; @@ -172,7 +171,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS // single filter - matches docs starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, - new StarTreeFilter(Map.of(SNDV, Set.of(new ExactMatchDimFilter(SNDV, List.of(0L))))), + new StarTreeFilter(Map.of(SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(0L))))), context, searchContext ); @@ -183,7 +182,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS // single filter on 3rd field in ordered dimension - matches docs starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, - new StarTreeFilter(Map.of(DV, Set.of(new ExactMatchDimFilter(DV, List.of(0L))))), + new StarTreeFilter(Map.of(DV, List.of(new ExactMatchDimFilter(DV, List.of(0L))))), context, searchContext ); @@ -194,7 +193,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS // single filter - does not match docs starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, - new StarTreeFilter(Map.of(SNDV, Set.of(new ExactMatchDimFilter(SNDV, List.of(101L))))), + new StarTreeFilter(Map.of(SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(101L))))), context, searchContext ); @@ -205,7 +204,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS // single filter on 3rd field in ordered dimension - does not match docs starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, - new StarTreeFilter(Map.of(SNDV, Set.of(new ExactMatchDimFilter(SNDV, List.of(-101L))))), + new StarTreeFilter(Map.of(SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(-101L))))), context, searchContext ); @@ -217,7 +216,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, new StarTreeFilter( - Map.of(SNDV, Set.of(new ExactMatchDimFilter(SNDV, List.of(0L))), DV, Set.of(new ExactMatchDimFilter(DV, List.of(0L)))) + Map.of(SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(0L))), DV, List.of(new ExactMatchDimFilter(DV, List.of(0L)))) ), context, searchContext @@ -230,7 +229,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, new StarTreeFilter( - Map.of(SNDV, Set.of(new ExactMatchDimFilter(SNDV, List.of(0L))), DV, Set.of(new ExactMatchDimFilter(DV, List.of(-11L)))) + Map.of(SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(0L))), DV, List.of(new ExactMatchDimFilter(DV, List.of(-11L)))) ), context, searchContext @@ -243,7 +242,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, new StarTreeFilter( - Map.of(SNDV, Set.of(new ExactMatchDimFilter(SNDV, List.of(0L))), DV, Set.of(new ExactMatchDimFilter(DV, List.of(-100L)))) + Map.of(SNDV, List.of(new ExactMatchDimFilter(SNDV, List.of(0L))), DV, List.of(new ExactMatchDimFilter(DV, List.of(-100L)))) ), context, searchContext @@ -257,7 +256,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS IllegalStateException.class, () -> getDocCountFromStarTree( starTreeDocValuesReader, - new StarTreeFilter(Map.of(FIELD_NAME, Set.of(new ExactMatchDimFilter(FIELD_NAME, List.of(0L))))), + new StarTreeFilter(Map.of(FIELD_NAME, List.of(new ExactMatchDimFilter(FIELD_NAME, List.of(0L))))), context, searchContext ) @@ -267,7 +266,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS // Documents are not indexed starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, - new StarTreeFilter(Map.of(SDV, Set.of(new ExactMatchDimFilter(SDV, List.of(4L))))), + new StarTreeFilter(Map.of(SDV, List.of(new ExactMatchDimFilter(SDV, List.of(4L))))), context, searchContext ); @@ -278,7 +277,7 @@ private void testStarTreeFilter(int maxLeafDoc, boolean skipStarNodeCreationForS // Documents are indexed starTreeDocCount = getDocCountFromStarTree( starTreeDocValuesReader, - new StarTreeFilter(Map.of(SDV, Set.of(new ExactMatchDimFilter(SDV, List.of(4L))))), + new StarTreeFilter(Map.of(SDV, List.of(new ExactMatchDimFilter(SDV, List.of(4L))))), context, searchContext );