You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/09/25 07:03:12 UTC
[GitHub] [lucene] jpountz opened a new pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
jpountz opened a new pull request #320:
URL: https://github.com/apache/lucene/pull/320
This avoids double wrapping of doc values in `Lucene90DocValuesConsumer`.
--
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
[GitHub] [lucene] jpountz merged pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
jpountz merged pull request #320:
URL: https://github.com/apache/lucene/pull/320
--
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
[GitHub] [lucene] jpountz merged pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
jpountz merged pull request #320:
URL: https://github.com/apache/lucene/pull/320
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #320:
URL: https://github.com/apache/lucene/pull/320#discussion_r717012176
##########
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:
All good points, especially that this is used pretty extensively to wrap single-valued fields already.
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by 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
[GitHub] [lucene] jpountz commented on pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
jpountz commented on pull request #320:
URL: https://github.com/apache/lucene/pull/320#issuecomment-928907986
Thanks for the review @gsmiller !
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #320:
URL: https://github.com/apache/lucene/pull/320#discussion_r717012176
##########
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:
All good points, especially that this is used pretty extensively to wrap single-valued fields already.
--
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
[GitHub] [lucene] jpountz commented on pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
jpountz commented on pull request #320:
URL: https://github.com/apache/lucene/pull/320#issuecomment-928907986
Thanks for the review @gsmiller !
--
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
[GitHub] [lucene] jpountz commented on a change in pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #320:
URL: https://github.com/apache/lucene/pull/320#discussion_r716936132
##########
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:
In general we're fine with creating some garbage on a per-segment basis (rather than, say, per doc) so I'm not concerned by the additional garbage. I guess it might have a positive impact when the caller optimizes for the single-valued case already and a negative impact when the caller doesn't, but in any case it is probably trivial? If this was a problem, then it would also be a problem that we use this wrapper when returning Sorted(Set|Numeric)DocValues over a single-valued field?
--
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
[GitHub] [lucene] jpountz commented on pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
jpountz commented on pull request #320:
URL: https://github.com/apache/lucene/pull/320#issuecomment-927023039
There are some indentation changes so the patch is easier to review when ignore space changes: https://github.com/apache/lucene/pull/320/files?w=1.
--
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
[GitHub] [lucene] jpountz commented on a change in pull request #320: LUCENE-10123: Handling of singletons in DocValuesConsumer.
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #320:
URL: https://github.com/apache/lucene/pull/320#discussion_r716936132
##########
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:
In general we're fine with creating some garbage on a per-segment basis (rather than, say, per doc) so I'm not concerned by the additional garbage. I guess it might have a positive impact when the caller optimizes for the single-valued case already and a negative impact when the caller doesn't, but in any case it is probably trivial? If this was a problem, then it would also be a problem that we use this wrapper when returning Sorted(Set|Numeric)DocValues over a single-valued field?
--
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