[GitHub] [lucene] gsmiller commented on a change in pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.

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

gsmiller commented on a change in pull request #320:
URL: https://github.com/apache/lucene/pull/320#discussion_r716849310



##########
File path: lucene/core/src/java/org/apache/lucene/codecs/DocValuesConsumer.java
##########
@@ -410,9 +417,26 @@ public SortedNumericDocValues getSortedNumeric(FieldInfo fieldInfo) throws IOExc
                 values = DocValues.emptySortedNumeric();
               }
               cost += values.cost();
+              if (DocValues.unwrapSingleton(values) == null) {

Review comment:
       nit: You could check `allSingletons` here first to short-circuit the attempted unwrapping. I probably doesn't really matter from a performance standpoint, so feel free to ignore the suggestion, but it could become:
   `if (allSingletons && DocValues.unwrapSingleton(values) == null)`

##########
File path: lucene/core/src/java/org/apache/lucene/index/DocValues.java
##########
@@ -157,97 +157,12 @@ public int getValueCount() {
 
   /** An empty SortedNumericDocValues which returns zero values for every document */
   public static final SortedNumericDocValues emptySortedNumeric() {
-    return new SortedNumericDocValues() {
-
-      private int doc = -1;
-
-      @Override
-      public int advance(int target) {
-        return doc = NO_MORE_DOCS;
-      }
-
-      @Override
-      public boolean advanceExact(int target) throws IOException {
-        doc = target;
-        return false;
-      }
-
-      @Override
-      public int docID() {
-        return doc;
-      }
-
-      @Override
-      public int nextDoc() {
-        return doc = NO_MORE_DOCS;
-      }
-
-      @Override
-      public long cost() {
-        return 0;
-      }
-
-      @Override
-      public int docValueCount() {
-        throw new IllegalStateException();
-      }
-
-      @Override
-      public long nextValue() {
-        throw new IllegalStateException();
-      }
-    };
+    return singleton(emptyNumeric());

Review comment:
       Are you concerned about any (minor) performance impact of this? It's probably trivial but it appears to be adding one more level of indirection and a little additional garbage creation.

##########
File path: lucene/core/src/java/org/apache/lucene/codecs/DocValuesConsumer.java
##########
@@ -849,10 +880,28 @@ public SortedSetDocValues getSortedSet(FieldInfo fieldInfo) throws IOException {
                 values = DocValues.emptySortedSet();
               }
               cost += values.cost();
+              if (DocValues.unwrapSingleton(values) == null) {

Review comment:
       nit: similar comment about checking `allSingletons` in the condition (as above, feel free to ignore :) )




-- 
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