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 2022/04/27 17:58:48 UTC
[GitHub] [lucene] risdenk commented on a diff in pull request #847: LUCENE-10542: FieldSource exists implementation can avoid value retrieval
risdenk commented on code in PR #847:
URL: https://github.com/apache/lucene/pull/847#discussion_r860101008
##########
lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/BytesRefFieldSource.java:
##########
@@ -45,35 +45,33 @@ public FunctionValues getValues(Map<Object, Object> context, LeafReaderContext r
// To be sorted or not to be sorted, that is the question
// TODO: do it cleaner?
if (fieldInfo != null && fieldInfo.getDocValuesType() == DocValuesType.BINARY) {
- final BinaryDocValues binaryValues = DocValues.getBinary(readerContext.reader(), field);
+ final BinaryDocValues arr = DocValues.getBinary(readerContext.reader(), field);
return new FunctionValues() {
int lastDocID = -1;
- private BytesRef getValueForDoc(int doc) throws IOException {
+ @Override
+ public boolean exists(int doc) throws IOException {
if (doc < lastDocID) {
throw new IllegalArgumentException(
"docs were sent out-of-order: lastDocID=" + lastDocID + " vs docID=" + doc);
}
lastDocID = doc;
- int curDocID = binaryValues.docID();
+ int curDocID = arr.docID();
if (doc > curDocID) {
- curDocID = binaryValues.advance(doc);
- }
- if (doc == curDocID) {
- return binaryValues.binaryValue();
- } else {
- return null;
+ curDocID = arr.advance(doc);
}
- }
-
- @Override
- public boolean exists(int doc) throws IOException {
- return getValueForDoc(doc) != null;
+ return doc == curDocID;
}
Review Comment:
`exists` is the same for all of these *FieldSource implementations now, but I didn't know a good way to remove the duplication. So I left the duplication for now.
##########
lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/BytesRefFieldSource.java:
##########
@@ -45,35 +45,33 @@ public FunctionValues getValues(Map<Object, Object> context, LeafReaderContext r
// To be sorted or not to be sorted, that is the question
// TODO: do it cleaner?
if (fieldInfo != null && fieldInfo.getDocValuesType() == DocValuesType.BINARY) {
- final BinaryDocValues binaryValues = DocValues.getBinary(readerContext.reader(), field);
+ final BinaryDocValues arr = DocValues.getBinary(readerContext.reader(), field);
return new FunctionValues() {
int lastDocID = -1;
- private BytesRef getValueForDoc(int doc) throws IOException {
+ @Override
+ public boolean exists(int doc) throws IOException {
if (doc < lastDocID) {
throw new IllegalArgumentException(
"docs were sent out-of-order: lastDocID=" + lastDocID + " vs docID=" + doc);
}
lastDocID = doc;
- int curDocID = binaryValues.docID();
+ int curDocID = arr.docID();
if (doc > curDocID) {
- curDocID = binaryValues.advance(doc);
- }
- if (doc == curDocID) {
- return binaryValues.binaryValue();
- } else {
- return null;
+ curDocID = arr.advance(doc);
}
- }
-
- @Override
- public boolean exists(int doc) throws IOException {
- return getValueForDoc(doc) != null;
+ return doc == curDocID;
}
@Override
public boolean bytesVal(int doc, BytesRefBuilder target) throws IOException {
- BytesRef value = getValueForDoc(doc);
+ BytesRef value;
+ if (exists(doc)) {
+ value = arr.binaryValue();
+ } else {
+ value = null;
+ }
Review Comment:
A nice side effect of removing getValueForDoc and using exists is the logic is MUCH cleaner now. If value exists then go convert it - otherwise don't. It avoids the indirection from before. So I think this is using the API better than before.
##########
lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/LongFieldSource.java:
##########
@@ -147,10 +122,6 @@ protected NumericDocValues getNumericDocValues(
return DocValues.getNumeric(readerContext.reader(), field);
}
- protected MutableValueLong newMutableValueLong() {
- return new MutableValueLong();
- }
-
Review Comment:
This wasn't used anywhere besides getValueFiller and this was the only difference in getValueFiller compared to the base class.
##########
lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/BytesRefFieldSource.java:
##########
@@ -45,35 +45,33 @@ public FunctionValues getValues(Map<Object, Object> context, LeafReaderContext r
// To be sorted or not to be sorted, that is the question
// TODO: do it cleaner?
if (fieldInfo != null && fieldInfo.getDocValuesType() == DocValuesType.BINARY) {
- final BinaryDocValues binaryValues = DocValues.getBinary(readerContext.reader(), field);
+ final BinaryDocValues arr = DocValues.getBinary(readerContext.reader(), field);
Review Comment:
This is only a cosmetic change to make `BytesRefFieldSource` and `DoubleFieldSource` match the other *FieldSource implementations using arr.
--
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