Reimplement is:watched to inline filter query trees
Instead of recursively executing the user's watched notifications
inline the query parse tree as part of the current query. This allows
an index such as Lucene to execute the entire set of notification
filters, avoiding unnecessary joins through the database.
Change-Id: Id55ec1b84aff8bed99db748de7c5f7332c62fbda
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/AndPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/AndPredicate.java
index 096e12e..5966dde 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/AndPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/AndPredicate.java
@@ -45,9 +45,6 @@
c += p.getCost();
}
}
- if (t.size() < 2) {
- throw new IllegalArgumentException("Need at least two predicates");
- }
children = t;
cost = c;
}
@@ -101,7 +98,7 @@
}
@Override
- public final String toString() {
+ public String toString() {
final StringBuilder r = new StringBuilder();
r.append("(");
for (int i = 0; i < getChildCount(); i++) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 7f5e91e..430ae22 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -235,7 +235,7 @@
}
if ("watched".equalsIgnoreCase(value)) {
- return new IsWatchedByPredicate(args, currentUser);
+ return new IsWatchedByPredicate(args, currentUser, false);
}
if ("visible".equalsIgnoreCase(value)) {
@@ -354,10 +354,10 @@
for (Account.Id id : m) {
if (currentUser instanceof IdentifiedUser
&& id.equals(((IdentifiedUser) currentUser).getAccountId())) {
- p.add(new IsWatchedByPredicate(args, currentUser));
+ p.add(new IsWatchedByPredicate(args, currentUser, false));
} else {
p.add(new IsWatchedByPredicate(args,
- args.userFactory.create(args.dbProvider, id)));
+ args.userFactory.create(args.dbProvider, id), true));
}
}
return Predicate.or(p);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
index 25cfae7..a7005d7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
@@ -14,22 +14,18 @@
package com.google.gerrit.server.query.change;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.query.OperatorPredicate;
+import com.google.gerrit.server.query.AndPredicate;
import com.google.gerrit.server.query.Predicate;
import com.google.gerrit.server.query.QueryParseException;
-import com.google.gwtorm.server.OrmException;
-import java.util.ArrayList;
-import java.util.HashMap;
import java.util.List;
-import java.util.Map;
-class IsWatchedByPredicate extends OperatorPredicate<ChangeData> {
+class IsWatchedByPredicate extends AndPredicate<ChangeData> {
private static String describe(CurrentUser user) {
if (user instanceof IdentifiedUser) {
return ((IdentifiedUser) user).getAccountId().toString();
@@ -37,85 +33,81 @@
return user.toString();
}
- private final ChangeQueryBuilder.Arguments args;
private final CurrentUser user;
- private Map<Project.NameKey, List<Predicate<ChangeData>>> rules;
-
- IsWatchedByPredicate(ChangeQueryBuilder.Arguments args, CurrentUser user) {
- super(ChangeQueryBuilder.FIELD_WATCHEDBY, describe(user));
- this.args = args;
+ IsWatchedByPredicate(ChangeQueryBuilder.Arguments args,
+ CurrentUser user,
+ boolean checkIsVisible) {
+ super(filters(args, user, checkIsVisible));
this.user = user;
}
- @Override
- public boolean match(final ChangeData cd) throws OrmException {
- if (rules == null) {
- ChangeQueryBuilder builder = new ChangeQueryBuilder(args, user);
- rules = new HashMap<Project.NameKey, List<Predicate<ChangeData>>>();
- for (AccountProjectWatch w : user.getNotificationFilters()) {
- List<Predicate<ChangeData>> list = rules.get(w.getProjectNameKey());
- if (list == null) {
- list = new ArrayList<Predicate<ChangeData>>(4);
- rules.put(w.getProjectNameKey(), list);
- }
-
- Predicate<ChangeData> p = compile(builder, w);
- if (p != null) {
- list.add(p);
+ private static List<Predicate<ChangeData>> filters(
+ ChangeQueryBuilder.Arguments args,
+ CurrentUser user,
+ boolean checkIsVisible) {
+ List<Predicate<ChangeData>> r = Lists.newArrayList();
+ ChangeQueryBuilder builder = new ChangeQueryBuilder(args, user);
+ for (AccountProjectWatch w : user.getNotificationFilters()) {
+ Predicate<ChangeData> f = null;
+ if (w.getFilter() != null) {
+ try {
+ f = builder.parse(w.getFilter());
+ if (builder.find(f, IsWatchedByPredicate.class) != null) {
+ // If the query is going to infinite loop, assume it
+ // will never match and return null. Yes this test
+ // prevents you from having a filter that matches what
+ // another user is filtering on. :-)
+ continue;
+ }
+ } catch (QueryParseException e) {
+ continue;
}
}
- }
- if (rules.isEmpty()) {
- return false;
- }
+ Predicate<ChangeData> p;
+ if (w.getProjectNameKey().equals(args.allProjectsName)) {
+ p = null;
+ } else {
+ p = builder.project(w.getProjectNameKey().get());
+ }
- Change change = cd.change(args.dbProvider);
- if (change == null) {
- return false;
- }
-
- Project.NameKey project = change.getDest().getParentKey();
- List<Predicate<ChangeData>> list = rules.get(project);
- if (list == null) {
- list = rules.get(args.allProjectsName);
- }
- if (list != null) {
- for (Predicate<ChangeData> p : list) {
- if (p.match(cd)) {
- return true;
- }
+ if (p != null && f != null) {
+ r.add(and(p, f));
+ } else if (p != null) {
+ r.add(p);
+ } else if (f != null) {
+ r.add(f);
+ } else {
+ r.add(builder.status_open());
}
}
- return false;
+ if (r.isEmpty()) {
+ return none();
+ } else if (checkIsVisible) {
+ return ImmutableList.of(or(r), builder.is_visible());
+ } else {
+ return r;
+ }
}
- @SuppressWarnings("unchecked")
- private Predicate<ChangeData> compile(ChangeQueryBuilder builder,
- AccountProjectWatch w) {
- Predicate<ChangeData> p = builder.is_visible();
- if (w.getFilter() != null) {
- try {
- p = Predicate.and(builder.parse(w.getFilter()), p);
- if (builder.find(p, IsWatchedByPredicate.class) != null) {
- // If the query is going to infinite loop, assume it
- // will never match and return null. Yes this test
- // prevents you from having a filter that matches what
- // another user is filtering on. :-)
- //
- return null;
- }
- p = args.rewriter.get().rewrite(p);
- } catch (QueryParseException e) {
- return null;
- }
- }
- return p;
+ private static List<Predicate<ChangeData>> none() {
+ Predicate<ChangeData> any = any();
+ return ImmutableList.of(not(any));
}
@Override
public int getCost() {
return 1;
}
+
+ @Override
+ public String toString() {
+ String val = describe(user);
+ if (val.indexOf(' ') < 0) {
+ return ChangeQueryBuilder.FIELD_WATCHEDBY + ":" + val;
+ } else {
+ return ChangeQueryBuilder.FIELD_WATCHEDBY + ":\"" + val + "\"";
+ }
+ }
}
