Removed equals and hashCode from filters and changed back to using List

Signed-off-by: expani <[email protected]>
opensearch-project · Jan 27, 2025 · 97a21b1 · 97a21b1
1 parent 197e885
commit 97a21b1
Show file tree

Hide file tree

Showing 8 changed files with 25 additions and 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
@@ -82,7 +82,7 @@ public static FixedBitSet getStarTreeResult(StarTreeValues starTreeValues, StarT
 
             StarTreeValuesIterator valuesIterator = starTreeValues.getDimensionValuesIterator(remainingPredicateColumn);
             // Get the query value directly
-            Set<DimensionFilter> dimensionFilters = starTreeFilter.getFiltersForDimension(remainingPredicateColumn);
+            List<DimensionFilter> 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<DimensionFilter> dimensionFilters = starTreeFilter.getFiltersForDimension(childDimension);
+                List<DimensionFilter> 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
@@ -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<Object> rawValues;
+    private final List<Object> rawValues;
 
     // Order is essential for successive binary search
     private TreeSet<Long> convertedOrdinals;
 
     public ExactMatchDimFilter(String dimensionName, List<Object> 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
@@ -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
@@ -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<String, Set<DimensionFilter>> dimensionFilterMap;
+    private final Map<String, List<DimensionFilter>> dimensionFilterMap;
 
-    public StarTreeFilter(Map<String, Set<DimensionFilter>> dimensionFilterMap) {
+    public StarTreeFilter(Map<String, List<DimensionFilter>> dimensionFilterMap) {
         this.dimensionFilterMap = dimensionFilterMap;
     }
 
-    public Set<DimensionFilter> getFiltersForDimension(String dimension) {
+    public List<DimensionFilter> getFiltersForDimension(String dimension) {
         return dimensionFilterMap.get(dimension);
     }
 
     public Set<String> 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/.../src/main/java/org/opensearch/search/startree/filter/provider/StarTreeFilterProvider.java b/.../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/.../test/java/org/opensearch/search/aggregations/startree/DimensionFilterAndMapperTests.java b/.../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
@@ -395,7 +395,7 @@ private void testStarTreeDocValuesInternal(Codec codec, List<DimensionFieldData>
 
         // 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
@@ -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
             );