jpountz commented on a change in pull request #736:
URL: https://github.com/apache/lucene/pull/736#discussion_r827711332
##########
File path:
lucene/sandbox/src/java/org/apache/lucene/sandbox/search/IndexSortSortedNumericDocValuesRangeQuery.java
##########
@@ -307,9 +292,16 @@ private BoundedDocIdSetIterator getDocIdSetIterator(
}
int lastDocIdExclusive = high + 1;
- BoundedDocIdSetIterator disi =
- new BoundedDocIdSetIterator(firstDocIdInclusive, lastDocIdExclusive,
delegate);
- return boundedDisiFunction.apply(sortField.getMissingValue(), disi);
+ Object missingValue = sortField.getMissingValue();
+ BoundedDocIdSetIterator disi;
+ if (missingValue == null
+ || (long) missingValue < lowerValue
+ || (long) missingValue > upperValue) {
Review comment:
I don't think it's correct to exclude the `missingValue == null` case
since it's assumed to be zero by default? We should resolve the missing value
that is used in practice first, e.g. as below?
```
final long missingLongValue = missingValue == null ? 0L : (long)
missingValue;
if (missingLongValue < lowerValue || missingValue > upperValue) { ... }
```
##########
File path:
lucene/sandbox/src/java/org/apache/lucene/sandbox/search/IndexSortSortedNumericDocValuesRangeQuery.java
##########
@@ -380,4 +372,25 @@ public long cost() {
return lastDoc - firstDoc;
}
}
+
+ private static class BoundedAllMatchDocIdSetIterator extends
BoundedDocIdSetIterator {
+
+ BoundedAllMatchDocIdSetIterator(int firstDoc, int lastDoc) {
+ super(firstDoc, lastDoc, null);
+ }
+
+ @Override
+ public int advance(int target) throws IOException {
+ if (target < firstDoc) {
+ target = firstDoc;
+ }
+
+ if (target < lastDoc) {
+ docID = target;
+ } else {
+ docID = NO_MORE_DOCS;
+ }
+ return docID;
+ }
+ }
Review comment:
nit: I'd rather fold this logic into `BoundedDocIdSetIterator` with a
`null` check on `delegate` unless we have evidence that the sub class buys us
much?
##########
File path:
lucene/sandbox/src/java/org/apache/lucene/sandbox/search/IndexSortSortedNumericDocValuesRangeQuery.java
##########
@@ -201,27 +200,18 @@ public boolean isCacheable(LeafReaderContext ctx) {
@Override
public int count(LeafReaderContext context) throws IOException {
- BoundedDocIdSetIterator disi;
- PointValues pointValues = context.reader().getPointValues(field);
- if (pointValues != null && pointValues.getDocCount() ==
context.reader().maxDoc()) {
- disi = getDocIdSetIteratorOrNull(context, (missingValue,
boundedDisi) -> boundedDisi);
- } else {
- disi =
- getDocIdSetIteratorOrNull(
- context,
- (missingValue, boundedDisi) -> {
- if (missingValue == null
- || (long) missingValue < lowerValue
- || (long) missingValue > upperValue) {
- return boundedDisi;
- }
- return null;
- });
- }
-
+ LeafReader reader = context.reader();
+ PointValues pointValues = reader.getPointValues(field);
+ BoundedDocIdSetIterator disi = getDocIdSetIteratorOrNull(context);
if (disi != null) {
- return disi.lastDoc - disi.firstDoc;
+ if (reader.hasDeletions() == false
+ && pointValues != null
+ && pointValues.getDocCount() == context.reader().maxDoc()
Review comment:
this `docCount == maxDoc` check could move to `getDocIdSetIterator` to
set the `delegate` to `null` when the field is dense?
--
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-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
