Addressing review comments

elastic · Aug 5, 2021 · deab87b · deab87b
1 parent c00e579
commit deab87b
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 15 deletions.
diff --git a/...rc/main/java/org/elasticsearch/xpack/wildcard/mapper/BinaryDvConfirmedAutomatonQuery.java b/...rc/main/java/org/elasticsearch/xpack/wildcard/mapper/BinaryDvConfirmedAutomatonQuery.java
@@ -15,7 +15,6 @@
 import org.apache.lucene.search.ConstantScoreWeight;
 import org.apache.lucene.search.DocIdSetIterator;
 import org.apache.lucene.search.IndexSearcher;
-import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.Query;
 import org.apache.lucene.search.ScoreMode;
 import org.apache.lucene.search.Scorer;
@@ -46,9 +45,6 @@ public BinaryDvConfirmedAutomatonQuery(Query approximation, String field, String
         this.matchPattern = matchPattern;
         bytesMatcher = new ByteRunAutomaton(automaton);
     }
-    public BinaryDvConfirmedAutomatonQuery(String field, String matchPattern, Automaton automaton) {
-        this(new MatchAllDocsQuery(), field, matchPattern, automaton);
-    }
 
     private BinaryDvConfirmedAutomatonQuery(Query approximation, String field, String matchPattern, ByteRunAutomaton bytesMatcher) {
         this.approxQuery = approximation;
@@ -86,7 +82,9 @@ public Scorer scorer(LeafReaderContext context) throws IOException {
                     @Override
                     public boolean matches() throws IOException {
                         if (values.advanceExact(approxDisi.docID()) == false)
-                        {                            
+                        {
+                            // Bug if we have an indexed value but no doc value.
+                            assert false;
                             return false;
                         }
                         BytesRef arrayOfValues = values.binaryValue();

diff --git a/...n/wildcard/src/main/java/org/elasticsearch/xpack/wildcard/mapper/WildcardFieldMapper.java b/...n/wildcard/src/main/java/org/elasticsearch/xpack/wildcard/mapper/WildcardFieldMapper.java
@@ -333,14 +333,12 @@ public Query wildcardQuery(String wildcardPattern, RewriteMethod method, boolean
             if (clauseCount > 0) {
                 // We can accelerate execution with the ngram query
                 BooleanQuery approxQuery = rewritten.build();
-                return new ConstantScoreQuery(
-                    new BinaryDvConfirmedAutomatonQuery(approxQuery, name(), wildcardPattern, automaton));                
+                return new BinaryDvConfirmedAutomatonQuery(approxQuery, name(), wildcardPattern, automaton);                
             } else if (numWildcardChars == 0 || numWildcardStrings > 0) {
                 // We have no concrete characters and we're not a pure length query e.g. ???
                 return new DocValuesFieldExistsQuery(name());
             }
-            return new ConstantScoreQuery(
-                new BinaryDvConfirmedAutomatonQuery(name(), wildcardPattern, automaton));
+            return new BinaryDvConfirmedAutomatonQuery(new MatchAllDocsQuery(), name(), wildcardPattern, automaton);
 
         }
 
@@ -367,7 +365,7 @@ public Query regexpQuery(String value, int syntaxFlags, int matchFlags, int maxD
             // MatchAllButRequireVerificationQuery is a special case meaning the regex is reduced to a single
             // clause which we can't accelerate at all and needs verification. Example would be ".."
             if (approxNgramQuery instanceof MatchAllButRequireVerificationQuery) {
-                return new BinaryDvConfirmedAutomatonQuery(name(), value, automaton);            
+                return new BinaryDvConfirmedAutomatonQuery(new MatchAllDocsQuery(), name(), value, automaton);            
             }
 
             // We can accelerate execution with the ngram query
@@ -741,7 +739,8 @@ public Query rangeQuery(
             Automaton automaton =  TermRangeQuery.toAutomaton(lower, upper, includeLower, includeUpper);
 
             if (accelerationQuery == null) {
-                return new BinaryDvConfirmedAutomatonQuery(name(), lower + "-" + upper, automaton);            
+                return new BinaryDvConfirmedAutomatonQuery(new MatchAllDocsQuery(),
+                    name(), lower + "-" + upper, automaton);            
             }
             return new BinaryDvConfirmedAutomatonQuery(accelerationQuery, name(), lower + "-" + upper, automaton);            
         }
@@ -822,7 +821,8 @@ public Query fuzzyQuery(
                     transpositions
                 );
                 if (ngramQ.clauses().size() == 0) {
-                    return new BinaryDvConfirmedAutomatonQuery(name(), searchTerm, fq.getAutomata().automaton);            
+                    return new BinaryDvConfirmedAutomatonQuery(new MatchAllDocsQuery(),
+                        name(), searchTerm, fq.getAutomata().automaton);            
                 }
 
                 return new BinaryDvConfirmedAutomatonQuery(ngramQ, name(), searchTerm, fq.getAutomata().automaton);            

diff --git a/...dcard/src/test/java/org/elasticsearch/xpack/wildcard/mapper/WildcardFieldMapperTests.java b/...dcard/src/test/java/org/elasticsearch/xpack/wildcard/mapper/WildcardFieldMapperTests.java
@@ -600,14 +600,17 @@ public void testQueryCachingEquality() throws IOException, ParseException {
             new Term("field", pattern),
             Integer.MAX_VALUE
         );
-        BinaryDvConfirmedAutomatonQuery csQ = new BinaryDvConfirmedAutomatonQuery("field", pattern, caseSensitiveAutomaton);
-        BinaryDvConfirmedAutomatonQuery ciQ = new BinaryDvConfirmedAutomatonQuery("field", pattern, caseInSensitiveAutomaton);
+        BinaryDvConfirmedAutomatonQuery csQ = new BinaryDvConfirmedAutomatonQuery(new MatchAllDocsQuery(),
+            "field", pattern, caseSensitiveAutomaton);
+        BinaryDvConfirmedAutomatonQuery ciQ = new BinaryDvConfirmedAutomatonQuery(new MatchAllDocsQuery(),
+            "field", pattern, caseInSensitiveAutomaton);
         assertNotEquals(csQ, ciQ);
         assertNotEquals(csQ.hashCode(), ciQ.hashCode());
 
         // Same query should be equal
         Automaton caseSensitiveAutomaton2 = WildcardQuery.toAutomaton(new Term("field", pattern));
-        BinaryDvConfirmedAutomatonQuery csQ2 = new BinaryDvConfirmedAutomatonQuery("field", pattern, caseSensitiveAutomaton2);
+        BinaryDvConfirmedAutomatonQuery csQ2 = new BinaryDvConfirmedAutomatonQuery(new MatchAllDocsQuery(),
+            "field", pattern, caseSensitiveAutomaton2);
         assertEquals(csQ, csQ2);
         assertEquals(csQ.hashCode(), csQ2.hashCode());
     }