Simplify: ignore DV for IP masks.

Signed-off-by: Mikhail Khludnev <[email protected]>
mkhludnev · Nov 20, 2024 · a4d65db · a4d65db
1 parent cfa3904
commit a4d65db
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 50 deletions.
diff --git a/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/IpFieldMapper.java
@@ -41,7 +41,6 @@
 import org.apache.lucene.search.BooleanQuery;
 import org.apache.lucene.search.ConstantScoreQuery;
 import org.apache.lucene.search.IndexOrDocValuesQuery;
-import org.apache.lucene.search.IndexSearcher;
 import org.apache.lucene.search.MatchNoDocsQuery;
 import org.apache.lucene.search.PointRangeQuery;
 import org.apache.lucene.search.Query;
@@ -318,55 +317,29 @@ private void convertIps(List<InetAddress> inetAddresses, List<Query> sink) {
 
         private void convertMasks(List<String> masks, QueryShardContext context, List<Query> sink) {
             if (!masks.isEmpty() && (isSearchable() || hasDocValues())) {
-                // scalar IPs might already take some place
-                boolean tooMany = masks.size() + sink.size() > IndexSearcher.getMaxClauseCount();
-                if (tooMany) {
-                    if (!isSearchable()) {
-                        throw new IndexSearcher.TooManyClauses(
-                            "can't search for " + masks.size() + " IP masks in `index:false` field " + name()
-                        );
-                    }
-                } // let's collect multirange and bq of dv-range
-                  // loop masks, collect ranges
                 IpMultiRangeQueryBuilder multiRange = null;
-                List<Query> dvQueries = null;
                 for (String mask : masks) {
                     final Tuple<InetAddress, Integer> cidr = InetAddresses.parseCidr(mask);
                     PointRangeQuery query = (PointRangeQuery) InetAddressPoint.newPrefixQuery(name(), cidr.v1(), cidr.v2());
-                    if (isSearchable()) {
+                    if (isSearchable()) { // even there is DV we don't go with it, since we can't guess clauses limit
                         if (multiRange == null) {
                             multiRange = new IpMultiRangeQueryBuilder(name());
                         }
                         multiRange.add(query.getLowerPoint(), query.getUpperPoint());
-                    }
-                    if (hasDocValues() && !tooMany) {// note searchable && dv && tooMany -> MulirangePoints
+                    } else { // it may hit clauses limit sooner or later
                         Query dvRange = SortedSetDocValuesField.newSlowRangeQuery(
                             name(),
                             new BytesRef(query.getLowerPoint()),
                             new BytesRef(query.getUpperPoint()),
                             true,
                             true
                         );
-                        if (isSearchable()) {
-                            if (dvQueries == null) {
-                                dvQueries = new ArrayList<>();
-                            }
-                            dvQueries.add(dvRange);
-                        } else { // straight to sink
-                            sink.add(dvRange);
-                        }
+                        sink.add(dvRange);
                     }
                 }
-                // && isSearchable()
-                if (multiRange != null && dvQueries != null) {
-                    sink.add(new IndexOrDocValuesQuery(multiRange.build(), union(dvQueries)));
-                } else {
-                    if (multiRange != null) {
-                        sink.add(multiRange.build());
-                    }
-                    if (dvQueries != null) {
-                        sink.addAll(dvQueries);
-                    }
+                // never IndexOrDocValuesQuery() since we can't guess clauses limit
+                if (multiRange != null) {
+                    sink.add(multiRange.build());
                 }
             }
         }

diff --git a/server/src/test/java/org/opensearch/index/mapper/IpFieldTypeTests.java b/server/src/test/java/org/opensearch/index/mapper/IpFieldTypeTests.java
@@ -236,6 +236,14 @@ public void testDvOnlyTermsQuery() {
         );
     }
 
+    public void testDvVsPoint() {
+        MappedFieldType indexOnly = new IpFieldMapper.IpFieldType("field", true, false, false, null, Collections.emptyMap());
+        MappedFieldType dvOnly = new IpFieldMapper.IpFieldType("field", false, false, true, null, Collections.emptyMap());
+        MappedFieldType indexDv = new IpFieldMapper.IpFieldType("field", true, false, true, null, Collections.emptyMap());
+        assertEquals("ignore DV", indexOnly.termsQuery(List.of("::2/16"), null), indexDv.termsQuery(List.of("::2/16"), null));
+        assertEquals(dvOnly.termQuery("::2/16", null), dvOnly.termsQuery(List.of("::2/16"), null));
+    }
+
     public void testRangeQuery() {
         MappedFieldType ft = new IpFieldMapper.IpFieldType("field");
         Query query = InetAddressPoint.newRangeQuery("field", InetAddresses.forString("::"), InetAddressPoint.MAX_VALUE);

diff --git a/server/src/test/java/org/opensearch/search/SearchIpFieldTermsTests.java b/server/src/test/java/org/opensearch/search/SearchIpFieldTermsTests.java
@@ -9,8 +9,8 @@
 package org.opensearch.search;
 
 import org.apache.lucene.search.IndexSearcher;
-import org.opensearch.OpenSearchException;
 import org.opensearch.action.bulk.BulkRequestBuilder;
+import org.opensearch.action.search.SearchPhaseExecutionException;
 import org.opensearch.action.search.SearchResponse;
 import org.opensearch.common.network.InetAddresses;
 import org.opensearch.common.xcontent.XContentFactory;
@@ -22,8 +22,6 @@
 import org.hamcrest.MatcherAssert;
 
 import java.io.IOException;
-import java.io.PrintWriter;
-import java.io.StringWriter;
 import java.net.InetAddress;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -119,8 +117,8 @@ public void testLessThanMaxClauses() throws IOException {
                     .filter(QueryBuilders.termsQuery("dummy_filter", "1", "2", "3", "4", "5"));
             });
             fail();
-        } catch (OpenSearchException ose) {
-            assertTrue(dumpException(ose).contains(".IndexSearcher.rewrite("));
+        } catch (SearchPhaseExecutionException ose) {
+            assertTrue("exceeding on query rewrite", ose.shardFailures()[0].getCause() instanceof IndexSearcher.TooManyNestedClauses);
         }
     }
 
@@ -135,21 +133,11 @@ public void testExceedMaxClauses() throws IOException {
         try { // error from mapper/parser
             assertTermsHitCount(indexName, "addr.dv", toQuery, expectMatches);
             fail();
-        } catch (OpenSearchException ose) {
-            String exceptionDump = dumpException(ose);
-            assertTrue(exceptionDump.contains("IP masks"));
-            assertTrue(exceptionDump.contains("IpFieldMapper"));
+        } catch (SearchPhaseExecutionException ose) {
+            assertTrue("exceeding on query building", ose.shardFailures()[0].getCause().getCause() instanceof IndexSearcher.TooManyClauses);
         }
     }
 
-    private static String dumpException(OpenSearchException ose) {
-        StringWriter stack = new StringWriter();
-        PrintWriter writer = new PrintWriter(stack);
-        ose.printStackTrace(writer);
-        writer.flush();
-        return stack.toString();
-    }
-
     public static String getFirstThreeOctets(String ipAddress) {
         // Split the IP address by the dot delimiter
         String[] octets = ipAddress.split("\\.");