gsmiller commented on code in PR #14268:
URL: https://github.com/apache/lucene/pull/14268#discussion_r2018814824
##########
lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java:
##########
@@ -108,6 +110,8 @@ protected PointInSetQuery(String field, int numDims, int
bytesPerDim, Stream pac
}
if (previous == null) {
previous = new BytesRefBuilder();
+ lowerPoint = new byte[bytesPerDim * numDims];
+ System.arraycopy(current.bytes, current.offset, lowerPoint, 0,
lowerPoint.length);
Review Comment:
minor: I think it'd be slightly more readable if you used `current.length`
here instead of ` lowerPoint.length` (and I might also throw an `assert
lowerPoint.length == current.length` immediately before this line to make it
clear they should be equal).
##########
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) {
+ return null;
+ } else if (lowerPoint != null && upperPoint != null) {
Review Comment:
Should it be true that either 1) both of these are null, or 2) both are
non-null? I think so right? If that's right, I would check `lowerPoint != null`
then put an `assert upperPoint != null` in the condition branch.
##########
lucene/core/src/java/org/apache/lucene/search/PointInSetQuery.java:
##########
@@ -122,6 +126,11 @@ protected PointInSetQuery(String field, int numDims, int
bytesPerDim, Stream pac
}
sortedPackedPoints = builder.finish();
sortedPackedPointsHashCode = sortedPackedPoints.hashCode();
+ if (previous != null) {
+ BytesRef max = previous.get();
+ upperPoint = new byte[bytesPerDim * numDims];
+ System.arraycopy(max.bytes, max.offset, upperPoint, 0,
upperPoint.length);
Review Comment:
minor: same comment here about using `previous.length` in place of
`upperPoint.length`. I don't feel strongly about this so please feel free to
disagree if you have a different perspective.
##########
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:
I'm not 100% sure of this but I don't think it's possible to get back a
non-null instance from `reader#getPointValues` that has a zero doc count. I
believe you'll always get back a null instance if the points field has no docs
in a segment. Can you confirm with a test and/or some debugging?
--
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
