jpountz commented on code in PR #13246:
URL: https://github.com/apache/lucene/pull/13246#discussion_r1544579639
##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -321,42 +307,52 @@ private void updateSkipInterval(boolean success) {
/**
* If {@link NumericComparator#pruning} equals {@link
Pruning#GREATER_THAN_OR_EQUAL_TO}, we
- * could better tune the {@link
NumericLeafComparator#maxValueAsBytes}/{@link
- * NumericLeafComparator#minValueAsBytes}. For instance, if the sort is
ascending and bottom
+ * could better tune the {@link
NumericLeafComparator#maxValueAsLong}/{@link
+ * NumericLeafComparator#minValueAsLong}. For instance, if the sort is
ascending and bottom
* value is 5, we will use a range on [MIN_VALUE, 4].
*/
private void encodeBottom() {
+ if (queueFull == false) {
+ return;
+ }
Review Comment:
This looks a bit hacky to me, let's instead fix the call site to only call
`encodeBottom()` if the queue is full?
##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -85,6 +83,16 @@ public void disableSkipping() {
pruning = Pruning.NONE;
}
+ protected abstract long missingValueAsComparableLong();
+
+ private static long sortableBytesAsLong(byte[] bytes) {
+ return switch (bytes.length) {
+ case 4 -> NumericUtils.sortableBytesToInt(bytes, 0);
+ case 8 -> NumericUtils.sortableBytesToLong(bytes, 0);
+ default -> throw new IllegalStateException("bytes count should be 4 or
8");
+ };
+ }
Review Comment:
Unless I'm mistaken, this part of the change makes stronger assumptions
about how fields are implemented. E.g. before I believe that you could create a
`NumericComparator` for half floats stored on 2 bytes, but now this exception
would fail. Can we somehow make this function work for arbitrary numbers of
bytes?
##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -321,42 +307,52 @@ private void updateSkipInterval(boolean success) {
/**
* If {@link NumericComparator#pruning} equals {@link
Pruning#GREATER_THAN_OR_EQUAL_TO}, we
- * could better tune the {@link
NumericLeafComparator#maxValueAsBytes}/{@link
- * NumericLeafComparator#minValueAsBytes}. For instance, if the sort is
ascending and bottom
+ * could better tune the {@link
NumericLeafComparator#maxValueAsLong}/{@link
+ * NumericLeafComparator#minValueAsLong}. For instance, if the sort is
ascending and bottom
* value is 5, we will use a range on [MIN_VALUE, 4].
*/
private void encodeBottom() {
+ if (queueFull == false) {
+ return;
+ }
if (reverse == false) {
- encodeBottom(maxValueAsBytes);
- if (pruning == Pruning.GREATER_THAN_OR_EQUAL_TO) {
- NumericUtils.nextDown(maxValueAsBytes);
+ maxValueAsLong = bottomAsComparableLong();
+ if (pruning == Pruning.GREATER_THAN_OR_EQUAL_TO && maxValueAsLong !=
Long.MIN_VALUE) {
+ maxValueAsLong--;
}
} else {
- encodeBottom(minValueAsBytes);
- if (pruning == Pruning.GREATER_THAN_OR_EQUAL_TO) {
- NumericUtils.nextUp(minValueAsBytes);
+ minValueAsLong = bottomAsComparableLong();
+ if (pruning == Pruning.GREATER_THAN_OR_EQUAL_TO && minValueAsLong !=
Long.MAX_VALUE) {
+ minValueAsLong++;
}
}
}
/**
* If {@link NumericComparator#pruning} equals {@link
Pruning#GREATER_THAN_OR_EQUAL_TO}, we
- * could better tune the {@link
NumericLeafComparator#maxValueAsBytes}/{@link
- * NumericLeafComparator#minValueAsBytes}. For instance, if the sort is
ascending and top value
+ * could better tune the {@link
NumericLeafComparator#minValueAsLong}/{@link
+ * NumericLeafComparator#minValueAsLong}. For instance, if the sort is
ascending and top value
* is 3, we will use a range on [4, MAX_VALUE].
*/
private void encodeTop() {
+ if (leafTopSet == false) {
Review Comment:
and likewise here, let's only call this method if `leafTopSet` is true?
##########
lucene/core/src/java/org/apache/lucene/search/comparators/NumericComparator.java:
##########
@@ -94,8 +102,12 @@ public abstract class NumericLeafComparator implements
LeafFieldComparator {
// if skipping functionality should be enabled on this segment
private final boolean enableSkipping;
private final int maxDoc;
- private byte[] minValueAsBytes;
- private byte[] maxValueAsBytes;
+
+ /** According to {@link FieldComparator#setTopValue}, topValueSet is final
in leafComparator */
Review Comment:
:+1:
--
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
