gautamworah96 commented on a change in pull request #731:
URL: https://github.com/apache/lucene/pull/731#discussion_r836963073
##########
File path:
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestMultiRangeQueries.java
##########
@@ -761,4 +764,103 @@ public void testEqualsAndHashCode() {
assertNotEquals(query1.hashCode(), query3.hashCode());
}
}
+
+ public void testRewrite() throws IOException {
+ LongPointMultiRangeBuilder builder = new LongPointMultiRangeBuilder("ff",
1);
+ builder.add(new long[] {2}, new long[] {3});
+ builder.add(new long[] {3}, new long[] {4});
+ builder.add(new long[] {1}, new long[] {5});
+ builder.add(new long[] {8}, new long[] {10});
+ builder.add(new long[] {3}, new long[] {3});
+ builder.add(new long[] {1}, new long[] {6});
+ MultiRangeQuery multiRangeQuery = builder.build();
+ Query query = multiRangeQuery.rewrite(null);
+ assertNotEquals(query, multiRangeQuery);
+ Query query1 = query.rewrite(null);
+ Assert.assertEquals(query1, query);
+ }
+
+ public void testRandomRewrite() throws IOException {
Review comment:
Lets write a small comment here explaining what behavior this function
is testing? Same for the `testOneDimensionCount` function
##########
File path:
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestMultiRangeQueries.java
##########
@@ -761,4 +764,103 @@ public void testEqualsAndHashCode() {
assertNotEquals(query1.hashCode(), query3.hashCode());
}
}
+
+ public void testRewrite() throws IOException {
+ LongPointMultiRangeBuilder builder = new LongPointMultiRangeBuilder("ff",
1);
+ builder.add(new long[] {2}, new long[] {3});
+ builder.add(new long[] {3}, new long[] {4});
+ builder.add(new long[] {1}, new long[] {5});
+ builder.add(new long[] {8}, new long[] {10});
+ builder.add(new long[] {3}, new long[] {3});
+ builder.add(new long[] {1}, new long[] {6});
+ MultiRangeQuery multiRangeQuery = builder.build();
+ Query query = multiRangeQuery.rewrite(null);
+ assertNotEquals(query, multiRangeQuery);
+ Query query1 = query.rewrite(null);
+ Assert.assertEquals(query1, query);
+ }
+
+ public void testRandomRewrite() throws IOException {
+ Directory dir = newDirectory();
+ RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+ int dims = 1;
+ addRandomDoc(w);
+
+ IndexReader reader = w.getReader();
+ IndexSearcher searcher = newSearcher(reader);
+
+ for (int n = 0; n < 100; n++) {
Review comment:
Maybe change it to `n < numIters` and randomly generate `numIters`. This
makes it a bit clearer that the outermost loop is for the num of iterations.
Same for the `testOneDimensionCount` function
##########
File path:
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestMultiRangeQueries.java
##########
@@ -761,4 +764,103 @@ public void testEqualsAndHashCode() {
assertNotEquals(query1.hashCode(), query3.hashCode());
}
}
+
+ public void testRewrite() throws IOException {
+ LongPointMultiRangeBuilder builder = new LongPointMultiRangeBuilder("ff",
1);
+ builder.add(new long[] {2}, new long[] {3});
+ builder.add(new long[] {3}, new long[] {4});
+ builder.add(new long[] {1}, new long[] {5});
+ builder.add(new long[] {8}, new long[] {10});
+ builder.add(new long[] {3}, new long[] {3});
+ builder.add(new long[] {1}, new long[] {6});
+ MultiRangeQuery multiRangeQuery = builder.build();
+ Query query = multiRangeQuery.rewrite(null);
+ assertNotEquals(query, multiRangeQuery);
+ Query query1 = query.rewrite(null);
+ Assert.assertEquals(query1, query);
+ }
+
+ public void testRandomRewrite() throws IOException {
+ Directory dir = newDirectory();
+ RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+ int dims = 1;
+ addRandomDoc(w);
+
+ IndexReader reader = w.getReader();
+ IndexSearcher searcher = newSearcher(reader);
+
+ for (int n = 0; n < 100; n++) {
+ int numRanges = RandomNumbers.randomIntBetween(random(), 1, 20);
+ LongPointMultiRangeBuilder builder1 = new
LongPointMultiRangeBuilder("point", dims);
+ BooleanQuery.Builder builder2 = new BooleanQuery.Builder();
+ for (int i = 0; i < numRanges; i++) {
+ long[] lower = new long[dims];
+ long[] upper = new long[dims];
+ for (int j = 0; j < dims; j++) {
+ lower[j] = RandomNumbers.randomLongBetween(random(), 0, 2000);
+ upper[j] = lower[j] + RandomNumbers.randomLongBetween(random(), 0,
2000);
+ }
+ builder1.add(lower, upper);
+ builder2.add(LongPoint.newRangeQuery("point", lower, upper),
BooleanClause.Occur.SHOULD);
+ }
+
+ MultiRangeQuery multiRangeQuery = builder1.build();
+ MultiRangeQuery rewriteMultiRangeQuery = (MultiRangeQuery)
multiRangeQuery.rewrite(reader);
+ int count = searcher.count(multiRangeQuery);
+ int rewriteCount = searcher.count(rewriteMultiRangeQuery);
+ assertEquals(rewriteCount, count);
+ }
+ IOUtils.close(reader, w, dir);
+ }
+
+ private void addRandomDoc(RandomIndexWriter w) throws IOException {
Review comment:
minor: addRandomDoc -> addRandomDocs.
Also, I would put the function before both the `testRandomRewrite` and the
`testOneDimensionCount` functions.
##########
File path:
lucene/sandbox/src/java/org/apache/lucene/sandbox/search/MultiRangeQuery.java
##########
@@ -163,6 +165,60 @@ public void visit(QueryVisitor visitor) {
}
}
+ @Override
+ public Query rewrite(IndexReader reader) throws IOException {
+ List<RangeClause> mergedRanges = mergeOverlappingRanges();
+ if (mergedRanges != rangeClauses) {
+ return new MultiRangeQuery(field, numDims, bytesPerDim, mergedRanges) {
+ @Override
+ protected String toString(int dimension, byte[] value) {
+ return MultiRangeQuery.this.toString(dimension, value);
+ }
+ };
+ } else {
+ return this;
+ }
+ }
+
+ private List<RangeClause> mergeOverlappingRanges() {
+ if (numDims != 1 || rangeClauses.size() <= 1) {
+ return rangeClauses;
+ }
+ List<RangeClause> originRangeClause = new ArrayList<>(rangeClauses);
+ final ArrayUtil.ByteArrayComparator comparator =
ArrayUtil.getUnsignedComparator(bytesPerDim);
+ originRangeClause.sort(
+ new Comparator<RangeClause>() {
+ @Override
+ public int compare(RangeClause o1, RangeClause o2) {
+ int result = comparator.compare(o1.lowerValue, 0, o2.lowerValue,
0);
+ if (result == 0) {
+ return comparator.compare(o1.upperValue, 0, o2.upperValue, 0);
+ } else {
+ return result;
+ }
+ }
+ });
+ List<RangeClause> finalRangeClause = new ArrayList<>();
+ RangeClause current = originRangeClause.get(0);
+ for (int i = 1; i < originRangeClause.size(); i++) {
+ RangeClause nextClause = originRangeClause.get(i);
+ if (comparator.compare(nextClause.lowerValue, 0, current.upperValue, 0)
> 0) {
+ finalRangeClause.add(current);
+ current = nextClause;
+ } else {
+ if (comparator.compare(nextClause.upperValue, 0, current.upperValue,
0) > 0) {
+ current = new RangeClause(current.lowerValue, nextClause.upperValue);
+ }
+ }
+ }
+ finalRangeClause.add(current);
+ if (finalRangeClause.size() != rangeClauses.size()) {
Review comment:
Ah. Makes sense. Maybe add a comment here explaining that this saves us
from creating an extra `MultiRangeQuery` object in `rewrite`?
##########
File path:
lucene/sandbox/src/java/org/apache/lucene/sandbox/search/MultiRangeQuery.java
##########
@@ -163,6 +165,69 @@ public void visit(QueryVisitor visitor) {
}
}
+ /**
+ * merge its overlapping ranges and return a simpler but slightly different
form by calling {@link
+ * #mergeOverlappingRanges}
+ */
+ @Override
+ public Query rewrite(IndexReader reader) throws IOException {
+ if (numDims != 1) {
+ return this;
+ }
+ List<RangeClause> mergedRanges = mergeOverlappingRanges(rangeClauses,
bytesPerDim);
+ if (mergedRanges != rangeClauses) {
+ return new MultiRangeQuery(field, numDims, bytesPerDim, mergedRanges) {
+ @Override
+ protected String toString(int dimension, byte[] value) {
+ return MultiRangeQuery.this.toString(dimension, value);
+ }
+ };
+ } else {
+ return this;
+ }
+ }
+
+ /** merge overlapping ranges to some unconnected ranges */
+ public static List<RangeClause> mergeOverlappingRanges(
Review comment:
Lets write better javadoc here with some parameter descriptions for the
input parameters since this is a static function that other classes could also
use.
##########
File path:
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestMultiRangeQueries.java
##########
@@ -761,4 +764,103 @@ public void testEqualsAndHashCode() {
assertNotEquals(query1.hashCode(), query3.hashCode());
}
}
+
+ public void testRewrite() throws IOException {
+ LongPointMultiRangeBuilder builder = new LongPointMultiRangeBuilder("ff",
1);
+ builder.add(new long[] {2}, new long[] {3});
+ builder.add(new long[] {3}, new long[] {4});
+ builder.add(new long[] {1}, new long[] {5});
+ builder.add(new long[] {8}, new long[] {10});
+ builder.add(new long[] {3}, new long[] {3});
+ builder.add(new long[] {1}, new long[] {6});
+ MultiRangeQuery multiRangeQuery = builder.build();
+ Query query = multiRangeQuery.rewrite(null);
+ assertNotEquals(query, multiRangeQuery);
+ Query query1 = query.rewrite(null);
+ Assert.assertEquals(query1, query);
+ }
+
+ public void testRandomRewrite() throws IOException {
+ Directory dir = newDirectory();
+ RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+ int dims = 1;
+ addRandomDoc(w);
+
+ IndexReader reader = w.getReader();
+ IndexSearcher searcher = newSearcher(reader);
+
+ for (int n = 0; n < 100; n++) {
+ int numRanges = RandomNumbers.randomIntBetween(random(), 1, 20);
+ LongPointMultiRangeBuilder builder1 = new
LongPointMultiRangeBuilder("point", dims);
+ BooleanQuery.Builder builder2 = new BooleanQuery.Builder();
+ for (int i = 0; i < numRanges; i++) {
+ long[] lower = new long[dims];
+ long[] upper = new long[dims];
+ for (int j = 0; j < dims; j++) {
+ lower[j] = RandomNumbers.randomLongBetween(random(), 0, 2000);
+ upper[j] = lower[j] + RandomNumbers.randomLongBetween(random(), 0,
2000);
+ }
+ builder1.add(lower, upper);
+ builder2.add(LongPoint.newRangeQuery("point", lower, upper),
BooleanClause.Occur.SHOULD);
Review comment:
We don't need `builder2` in this function right? We are rewriting the
`builder1` query...
##########
File path:
lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestMultiRangeQueries.java
##########
@@ -761,4 +764,103 @@ public void testEqualsAndHashCode() {
assertNotEquals(query1.hashCode(), query3.hashCode());
}
}
+
+ public void testRewrite() throws IOException {
+ LongPointMultiRangeBuilder builder = new LongPointMultiRangeBuilder("ff",
1);
+ builder.add(new long[] {2}, new long[] {3});
+ builder.add(new long[] {3}, new long[] {4});
+ builder.add(new long[] {1}, new long[] {5});
+ builder.add(new long[] {8}, new long[] {10});
+ builder.add(new long[] {3}, new long[] {3});
+ builder.add(new long[] {1}, new long[] {6});
+ MultiRangeQuery multiRangeQuery = builder.build();
+ Query query = multiRangeQuery.rewrite(null);
+ assertNotEquals(query, multiRangeQuery);
+ Query query1 = query.rewrite(null);
+ Assert.assertEquals(query1, query);
+ }
Review comment:
Lets remove this `testRewrite` function now that we are adding a
randomized test for it.
##########
File path:
lucene/sandbox/src/java/org/apache/lucene/sandbox/search/MultiRangeQuery.java
##########
@@ -163,6 +165,60 @@ public void visit(QueryVisitor visitor) {
}
}
+ @Override
+ public Query rewrite(IndexReader reader) throws IOException {
Review comment:
Hmm. I should've been clearer. I meant something like
```
Merges the overlapping ranges and returns unconnected ranges by calling
{@link #mergeOverlappingRanges}
```
--
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
