Fix index rewriter to rewrite all Or/AndPredicates
ChangeIndexRewriter processes predicate tree to group all index
predicates into one at each level. Rewriter starts from root node
and traverses to all the child nodes. Rewriter recursively marks each
child predicate as indexed or nonindexed. Then it partitions all the
indexed predicates together and also rewrites And/OrPredicates to
either a source or a cardinal predicate. Before this change, rewriter
short-circuits the partitioning & skips rewrite logic when there is
only one indexed child. Due to this behavior, query [2] was not
rewritten to OrCardinal predicate since (status:open OR status:merged)
is one indexed child at its level (as in [3]), whereas query [1] was
rewritten to AndCardinal.
[1] status:open age:1d issue:123
[2] (status:open OR status:merged) issue:123
[3] root
/ \
Or issue:123
/ \
open merged
Fix index rewriter to run the rewrite logic to rewrite Or/AndPredicate
to respective cardinal predicates when it short-circuits. This helps
AndSource to choose the right source more often. Also fix few tests
to assert on OrCardinalPredicate rather than OrPredicate.
Unfortunately there are no operators which are datasources other
than Index in the Core as of now. We at Qualcomm have a datasource
operator in a plugin which takes a bug number and queries the bug
tracker's DB for changes related to that bug number. Suppose this
operator (say issue:123) returns around 15 changes. On a 3.4 test site
against LUCENE which contains ~20K open changes, ~2.8M merged changes,
the performance of,
query "(status:open OR status:merged) issue:123"
before: 6m52s, 6m40s, 6m36s
after: 0.07s, 0.07s, 0.07s
Release-Notes: AndSource chooses right source more often
Change-Id: I87311359e28795d5f461b233cecd82ecaf53ee57
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
index 1f270b0..437c1c4 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
@@ -261,7 +261,8 @@
throws QueryParseException {
if (isIndexed.cardinality() == 1) {
int i = isIndexed.nextSetBit(0);
- newChildren.add(0, new IndexedChangeQuery(index, newChildren.remove(i), opts));
+ Predicate<ChangeData> indexed = newChildren.remove(i);
+ newChildren.add(0, new IndexedChangeQuery(index, copy(indexed, indexed.getChildren()), opts));
return copy(in, newChildren);
}
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
index 96fc0d1..0a4f31c 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.common.data.GlobalCapability.DEFAULT_MAX_QUERY_LIMIT;
+import static com.google.gerrit.entities.Change.Status.ABANDONED;
import static com.google.gerrit.entities.Change.Status.MERGED;
import static com.google.gerrit.entities.Change.Status.NEW;
import static com.google.gerrit.index.query.Predicate.and;
@@ -29,6 +30,7 @@
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.query.AndCardinalPredicate;
import com.google.gerrit.index.query.AndPredicate;
+import com.google.gerrit.index.query.OrCardinalPredicate;
import com.google.gerrit.index.query.OrPredicate;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -72,7 +74,12 @@
assertThat(AndChangeSource.class).isSameInstanceAs(out.getClass());
assertThat(out.getChildren())
.containsExactly(
- query(Predicate.or(ChangeStatusPredicate.open(), ChangeStatusPredicate.closed())), in)
+ query(
+ orCardinal(
+ ChangeStatusPredicate.forStatus(NEW),
+ ChangeStatusPredicate.forStatus(MERGED),
+ ChangeStatusPredicate.forStatus(ABANDONED))),
+ in)
.inOrder();
}
@@ -89,7 +96,12 @@
assertThat(AndChangeSource.class).isSameInstanceAs(out.getClass());
assertThat(out.getChildren())
.containsExactly(
- query(Predicate.or(ChangeStatusPredicate.open(), ChangeStatusPredicate.closed())), in)
+ query(
+ orCardinal(
+ ChangeStatusPredicate.forStatus(NEW),
+ ChangeStatusPredicate.forStatus(MERGED),
+ ChangeStatusPredicate.forStatus(ABANDONED))),
+ in)
.inOrder();
}
@@ -268,6 +280,11 @@
return new AndCardinalPredicate<>(Arrays.asList(preds));
}
+ @SafeVarargs
+ private static OrCardinalPredicate<ChangeData> orCardinal(Predicate<ChangeData>... preds) {
+ return new OrCardinalPredicate<>(Arrays.asList(preds));
+ }
+
private Predicate<ChangeData> rewrite(Predicate<ChangeData> in) throws QueryParseException {
return rewrite.rewrite(in, options(0, DEFAULT_MAX_QUERY_LIMIT));
}
