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/26 19:16:51 UTC
[GitHub] [lucene] risdenk opened a new pull request, #840: LUCENE-10534: FieldSource exists improvements
risdenk opened a new pull request, #840:
URL: https://github.com/apache/lucene/pull/840
# Description
Please provide a short description of the changes you're making with this pull request.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/lucene/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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] risdenk commented on a diff in pull request #840: LUCENE-10534: FieldSource exists improvements
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #840:
URL: https://github.com/apache/lucene/pull/840#discussion_r859074624
##########
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.
--
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] risdenk commented on a diff in pull request #840: LUCENE-10534: FieldSource exists improvements
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #840:
URL: https://github.com/apache/lucene/pull/840#discussion_r859065937
##########
lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/DoubleFieldSource.java:
##########
@@ -52,55 +50,32 @@ public SortField getSortField(boolean reverse) {
public FunctionValues getValues(Map<Object, Object> context, LeafReaderContext readerContext)
throws IOException {
- final NumericDocValues values = getNumericDocValues(context, readerContext);
+ final NumericDocValues arr = getNumericDocValues(context, readerContext);
Review Comment:
This is only a cosmetic change to make `DoubleFieldSource` match the other `*FieldSource` implementations using `arr`.
##########
lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/EnumFieldSource.java:
##########
@@ -136,8 +121,16 @@ public String strVal(int doc) throws IOException {
@Override
public boolean exists(int doc) throws IOException {
- getValueForDoc(doc);
- return arr.docID() == doc;
+ if (doc < lastDocID) {
+ throw new IllegalArgumentException(
+ "docs were sent out-of-order: lastDocID=" + lastDocID + " vs docID=" + doc);
+ }
+ lastDocID = doc;
+ int curDocID = arr.docID();
+ if (doc > curDocID) {
+ curDocID = arr.advance(doc);
+ }
+ 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.
--
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] risdenk commented on a diff in pull request #840: LUCENE-10534: FieldSource exists improvements
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #840:
URL: https://github.com/apache/lucene/pull/840#discussion_r859076455
##########
lucene/queries/src/java/org/apache/lucene/queries/function/valuesource/DoubleFieldSource.java:
##########
@@ -52,55 +50,32 @@ public SortField getSortField(boolean reverse) {
public FunctionValues getValues(Map<Object, Object> context, LeafReaderContext readerContext)
throws IOException {
- final NumericDocValues values = getNumericDocValues(context, readerContext);
+ final NumericDocValues arr = getNumericDocValues(context, readerContext);
return new DoubleDocValues(this) {
int lastDocID;
- private double getValueForDoc(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 = values.docID();
- if (doc > curDocID) {
- curDocID = values.advance(doc);
- }
- if (doc == curDocID) {
- return Double.longBitsToDouble(values.longValue());
+ @Override
+ public double doubleVal(int doc) throws IOException {
+ if (exists(doc)) {
+ return Double.longBitsToDouble(arr.longValue());
} else {
return 0.0;
}
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.
--
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] risdenk closed pull request #840: LUCENE-10534: FieldSource exists improvements
Posted by GitBox <gi...@apache.org>.
risdenk closed pull request #840: LUCENE-10534: FieldSource exists improvements
URL: https://github.com/apache/lucene/pull/840
--
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] risdenk commented on pull request #840: LUCENE-10534: FieldSource exists improvements
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #840:
URL: https://github.com/apache/lucene/pull/840#issuecomment-1111315776
Superseded by https://github.com/apache/lucene/pull/847
--
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