gsmiller commented on code in PR #14268:
URL: https://github.com/apache/lucene/pull/14268#discussion_r2021381006
##########
lucene/CHANGES.txt:
##########
@@ -186,6 +186,8 @@ Optimizations
* GITHUB#14272: Use DocIdSetIterator#range for continuous-id BKD leaves. (Guo
Feng)
+* GITHUB#14268: PointInSetQuery clips segments by lower and upper (hanbj)
Review Comment:
Let's move the changes entry to 10.3 since the 10.2 branch has already been
cut and I don't think we need to squeeze this in with that release?
Also, can we make this entry a little more descriptive? Maybe something like
`PointInSetQuery optimization for the case when no segment docs can intersect
with the query values`?
##########
lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java:
##########
@@ -248,6 +255,33 @@ public long cost() {
}
}
+ private boolean checkValidPointValues(PointValues values) throws
IOException {
Review Comment:
I'd prefer going back to having this logic inlined as it was. I don't think
checking `values == null` is really part of validating the `PointValues`. And
there's nothing else calling this, so I think it's a bit easier to read when
inlined.
##########
lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java:
##########
@@ -248,6 +255,33 @@ public long cost() {
}
}
+ private boolean checkValidPointValues(PointValues values) throws
IOException {
Review Comment:
(But I do agree we should do the validation before the optimization you
introduced)
##########
lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java:
##########
@@ -153,6 +162,21 @@ public ScorerSupplier scorerSupplier(LeafReaderContext
context) throws IOExcepti
return null;
}
+ if (values.getDocCount() == 0) {
Review Comment:
Looking at the git annotations in `PointRangeQuery`, these checks were added
in two different changes. I think it's likely this was just overlooked. I do
not believe it's possible to have a non-null `PointValue` at this point that
returns a zero doc count. (I also played around with this using some unit tests
and a debugger and can confirm that behavior). All that said, I'm not strongly
opposed to leaving the check in there.
--
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
