[GitHub] [lucene] jtibshirani commented on a change in pull request #749: LUCENE-10466: IndexSortSortedNumericDocValuesRangeQuery unconditionally assumes the usage of the LONG-encoded SortField

[GitBox <gi...@apache.org>]

jtibshirani commented on a change in pull request #749:
URL: https://github.com/apache/lucene/pull/749#discussion_r840121029



##########
File path: lucene/sandbox/src/java/org/apache/lucene/sandbox/search/IndexSortSortedNumericDocValuesRangeQuery.java
##########
@@ -70,6 +72,9 @@
  *       field, lowerValue, upperValue, fallbackQuery);
  * </pre>
  *
+ * <b>Notes</b>: only {@code SortField.Type.LONG} and {@code SortField.Type.INT} are supported at

Review comment:
       Instead we can add this as a bullet in the list above, which details what needs to be true for the optimization to apply ("This optimized execution strategy is only used if the following conditions hold..."). 

##########
File path: lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestIndexSortSortedNumericDocValuesRangeQuery.java
##########
@@ -155,15 +156,23 @@ public void testToString() {
   }
 
   public void testIndexSortDocValuesWithEvenLength() throws Exception {
-    testIndexSortDocValuesWithEvenLength(false);
-    testIndexSortDocValuesWithEvenLength(true);
+    final SortField.Type type =
+        RandomPicks.randomFrom(
+            random(),
+            new SortField.Type[] {
+              SortField.Type.INT, SortField.Type.LONG, SortField.Type.DOUBLE, SortField.Type.FLOAT

Review comment:
       I think it'd be best to split out the `DOUBLE` and `FLOAT` checks into its own test that checks the optimization is deactivated (like `testNoIndexSort`). Also I think it's nice to improve coverage by doing
   
   ```
   for (SortField.Type type : new new SortField.Type[] { SortField.Type.INT, SortField.Type.LONG}) {
     ...
   }
   ```
   
   That way if someone breaks the query with integers, they'll know right away, instead of maybe having to run the tests a couple times before they fail!

##########
File path: lucene/sandbox/src/java/org/apache/lucene/sandbox/search/IndexSortSortedNumericDocValuesRangeQuery.java
##########
@@ -250,6 +255,10 @@ private BoundedDocIdSetIterator getDocIdSetIterator(
 
     // Perform a binary search to find the first document with value >= lower.
     ValueComparator comparator = loadComparator(sortField, lower, context);
+    if (comparator == null) {

Review comment:
       I find this harder to follow than if we checked up front in `getDocIdSetIteratorOrNull`. If we did that, we wouldn't have to pipe the `null` return value around here. We could then even simplify the check inside `loadComparator`, because we would know that `SortField.Type` is either `INT` or `LONG`.




-- 
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