[GitHub] [lucene] zacharymorn commented on a change in pull request #767: LUCENE-10436: Deprecate DocValuesFieldExistsQuery, NormsFieldExistsQuery and KnnVectorFieldExistsQuery with FieldExistsQuery

[GitBox <gi...@apache.org>]

zacharymorn commented on a change in pull request #767:
URL: https://github.com/apache/lucene/pull/767#discussion_r839158832



##########
File path: lucene/core/src/java/org/apache/lucene/search/DocValuesFieldExistsQuery.java
##########
@@ -31,42 +28,21 @@
 /**
  * A {@link Query} that matches documents that have a value for a given field as reported by doc
  * values iterators.
+ *
+ * @deprecated Use {@link org.apache.lucene.search.FieldExistsQuery} instead.
  */
-public final class DocValuesFieldExistsQuery extends Query {
-
-  private final String field;
+@Deprecated
+public final class DocValuesFieldExistsQuery extends FieldExistsQuery {
+  private String field;
 
   /** Create a query that will match documents which have a value for the given {@code field}. */
   public DocValuesFieldExistsQuery(String field) {
+    super(field);
     this.field = Objects.requireNonNull(field);
   }
 
-  public String getField() {
-    return field;
-  }
-
-  @Override
-  public boolean equals(Object other) {
-    return sameClassAs(other) && field.equals(((DocValuesFieldExistsQuery) other).field);
-  }
-
-  @Override
-  public int hashCode() {
-    return 31 * classHash() + field.hashCode();
-  }
-
-  @Override
-  public String toString(String field) {
-    return "DocValuesFieldExistsQuery [field=" + this.field + "]";
-  }
-
-  @Override
-  public void visit(QueryVisitor visitor) {
-    if (visitor.acceptField(field)) {
-      visitor.visitLeaf(this);
-    }
-  }
-
+  // nocommit this seems to be generalizable to norms and knn as well given LUCENE-9334, and thus
+  // could be moved to the new FieldExistsQuery?

Review comment:
       Thanks for the confirmation! I do have one follow-up question though. When I looked into this further, I noticed `PointValues` was used in the current implementation (pasted below for ease of reference) for determining if `DocValuesFieldExistsQuery` could be re-written to `MatchAllDocsQuery`: 
   
   ```
   @Override
     public Query rewrite(IndexReader reader) throws IOException {
       boolean allReadersRewritable = true;
       for (LeafReaderContext context : reader.leaves()) {
         LeafReader leaf = context.reader();
         Terms terms = leaf.terms(field);
         PointValues pointValues = leaf.getPointValues(field);
         if ((terms == null || terms.getDocCount() != leaf.maxDoc())
             && (pointValues == null || pointValues.getDocCount() != leaf.maxDoc())) {
           allReadersRewritable = false;
           break;
         }
       }
       if (allReadersRewritable) {
         return new MatchAllDocsQuery();
       }
       return super.rewrite(reader);
     }
   ```
   
   I thought `PointValues` and `DocValues` are separate indexed values and hence we would use one of the `leaf.getXXXDocValues` method here instead? On the other hand, the returned values of those methods such as `NumericDocValues` and `BinaryDocValues` don't seems to have a method to retrieve the associated doc count. I felt I might be missing something here but will look further into it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org