Implement Paginated interface for index searches

To support pagination, we need to be able to restart a search from a
particular sort key. In the SQL index implementation, this is done
using a combination of rewrite rules and ChangeAccess methods for
scanning around the sort key. This approach does not quite work for
the secondary index, which is much less rewrite-heavy.

Teach IndexedChangeQuery to replace SortKeyPredicate leaves in a
predicate tree with a new predicate having a different cut point. This
is a simple recursive algorithm since we explicitly do not try to deal
with the case of multiple sort key predicates in different subtrees.
Such queries are already documented to have unpredictable behavior
(since in the SQL implementation it is non-obvious which of a number
of such predicates is chosen as the source and which are used for
filtering).

Change-Id: Ia839eb21086b64be2bebdcc71aa579b8c99a2fd8

gerrit-server/src/main/java/com/google/gerrit/server/index/IndexRewriteImpl.java

@@ -154,7 +154,7 @@ public class IndexRewriteImpl implements ChangeQueryRewriter {
 
     Predicate<ChangeData> out = rewriteImpl(in, index, limit);
     if (in == out || out instanceof IndexPredicate) {
-      return new IndexedChangeQuery(index, out, limit);
+      return new IndexedChangeQuery(db, index, out, limit);
     } else if (out == null /* cannot rewrite */) {
       return in;
     } else {
@@ -231,7 +231,7 @@ public class IndexRewriteImpl implements ChangeQueryRewriter {
     if (isIndexed.cardinality() == 1) {
       int i = isIndexed.nextSetBit(0);
       newChildren.add(
-          0, new IndexedChangeQuery(index, newChildren.remove(i), limit));
+          0, new IndexedChangeQuery(db, index, newChildren.remove(i), limit));
       return copy(in, newChildren);
     }
 
@@ -251,7 +251,7 @@ public class IndexRewriteImpl implements ChangeQueryRewriter {
         all.add(c);
       }
     }
-    all.add(0, new IndexedChangeQuery(index, in.copy(indexed), limit));
+    all.add(0, new IndexedChangeQuery(db, index, in.copy(indexed), limit));
     return copy(in, all);
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java

@@ -14,17 +14,22 @@
 
 package com.google.gerrit.server.index;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
-
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeDataSource;
+import com.google.gerrit.server.query.change.Paginated;
+import com.google.gerrit.server.query.change.SortKeyPredicate;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.ResultSet;
+import com.google.inject.Provider;
 
 import java.util.Collection;
 import java.util.Iterator;
@@ -39,18 +44,55 @@ import java.util.List;
  * {@link ChangeDataSource} to be chosen by the query processor.
  */
 public class IndexedChangeQuery extends Predicate<ChangeData>
-    implements ChangeDataSource {
+    implements ChangeDataSource, Paginated {
-  private final ChangeIndex index;
-  private final Predicate<ChangeData> pred;
-  private final int limit;
-  private final ChangeDataSource source;
 
-  public IndexedChangeQuery(ChangeIndex index, Predicate<ChangeData> pred, int limit)
+  /**
-      throws QueryParseException {
+   * Replace all {@link SortKeyPredicate}s in a tree.
+   * <p>
+   * Strictly speaking this should replace only the {@link SortKeyPredicate} at
+   * the top-level AND node, but this implementation is simpler, and the
+   * behavior of having multiple sortkey operators is undefined anyway.
+   *
+   * @param p predicate to replace in.
+   * @param newValue new cut value to replace all sortkey operators with.
+   * @return a copy of {@code p} with all sortkey predicates replaced; or p
+   *     itself.
+   */
+  @VisibleForTesting
+  static Predicate<ChangeData> replaceSortKeyPredicates(
+      Predicate<ChangeData> p, String newValue) {
+    if (p instanceof SortKeyPredicate) {
+      return ((SortKeyPredicate) p).copy(newValue);
+    } else if (p.getChildCount() > 0) {
+      List<Predicate<ChangeData>> newChildren =
+          Lists.newArrayListWithCapacity(p.getChildCount());
+      boolean replaced = false;
+      for (Predicate<ChangeData> c : p.getChildren()) {
+        Predicate<ChangeData> nc = replaceSortKeyPredicates(c, newValue);
+        newChildren.add(nc);
+        if (nc != c) {
+          replaced = true;
+        }
+      }
+      return replaced ? p.copy(newChildren) : p;
+    } else {
+      return p;
+    }
+  }
+
+  private final Provider<ReviewDb> db;
+  private final ChangeIndex index;
+  private final int limit;
+
+  private Predicate<ChangeData> pred;
+  private ChangeDataSource source;
+
+  public IndexedChangeQuery(Provider<ReviewDb> db, ChangeIndex index,
+      Predicate<ChangeData> pred, int limit) throws QueryParseException {
+    this.db = db;
     this.index = index;
     this.pred = pred;
     this.limit = limit;
-    this.source = index.getSource(pred, limit);
   }
 
   @Override
@@ -71,9 +113,14 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
     return ImmutableList.of(pred);
   }
 
+  @Override
+  public int limit() {
+    return limit;
+  }
+
   @Override
   public int getCardinality() {
-    return source.getCardinality();
+    return source != null ? source.getCardinality() : limit();
   }
 
   @Override
@@ -84,7 +131,15 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
 
   @Override
   public ResultSet<ChangeData> read() throws OrmException {
+    final ChangeDataSource currSource;
+    try {
+      currSource = index.getSource(pred, limit);
+    } catch (QueryParseException e) {
+      throw new OrmException(e);
+    }
+    source = currSource;
     final ResultSet<ChangeData> rs = source.read();
+
     return new ResultSet<ChangeData>() {
       @Override
       public Iterator<ChangeData> iterator() {
@@ -94,7 +149,7 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
               @Override
               public
               ChangeData apply(ChangeData input) {
-                input.cacheFromSource(source);
+                input.cacheFromSource(currSource);
                 return input;
               }
             }).iterator();
@@ -104,7 +159,7 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
       public List<ChangeData> toList() {
         List<ChangeData> r = rs.toList();
         for (ChangeData cd : r) {
-          cd.cacheFromSource(source);
+          cd.cacheFromSource(currSource);
         }
         return r;
       }
@@ -116,6 +171,12 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
     };
   }
 
+  @Override
+  public ResultSet<ChangeData> restart(ChangeData last) throws OrmException {
+    pred = replaceSortKeyPredicates(pred, last.change(db).getSortKey());
+    return read();
+  }
+
   @Override
   public Predicate<ChangeData> copy(
       Collection<? extends Predicate<ChangeData>> children) {
@@ -124,7 +185,7 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
 
   @Override
   public boolean match(ChangeData cd) throws OrmException {
-    return cd.isFromSource(source) || pred.match(cd);
+    return (source != null && cd.isFromSource(source)) || pred.match(cd);
   }
 
   @Override
@@ -153,7 +214,7 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
   @Override
   public String toString() {
     return Objects.toStringHelper("index")
-        .add("p", source)
+        .add("p", pred)
         .add("limit", limit)
         .toString();
   }

gerrit-server/src/main/java/com/google/gerrit/server/query/change/Paginated.java

@@ -17,7 +17,7 @@ package com.google.gerrit.server.query.change;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.ResultSet;
 
-interface Paginated {
+public interface Paginated {
   int limit();
 
   ResultSet<ChangeData> restart(ChangeData last) throws OrmException;

gerrit-server/src/main/java/com/google/gerrit/server/query/change/SortKeyPredicate.java

@@ -38,6 +38,7 @@ public abstract class SortKeyPredicate extends IndexPredicate<ChangeData> {
 
   public abstract long getMinValue();
   public abstract long getMaxValue();
+  public abstract SortKeyPredicate copy(String newValue);
 
   public static class Before extends SortKeyPredicate {
     Before(Provider<ReviewDb> dbProvider, String value) {
@@ -59,6 +60,11 @@ public abstract class SortKeyPredicate extends IndexPredicate<ChangeData> {
       Change change = cd.change(dbProvider);
       return change != null && change.getSortKey().compareTo(getValue()) < 0;
     }
+
+    @Override
+    public Before copy(String newValue) {
+      return new Before(dbProvider, newValue);
+    }
   }
 
   public static class After extends SortKeyPredicate {
@@ -81,5 +87,10 @@ public abstract class SortKeyPredicate extends IndexPredicate<ChangeData> {
       Change change = cd.change(dbProvider);
       return change != null && change.getSortKey().compareTo(getValue()) > 0;
     }
+
+    @Override
+    public After copy(String newValue) {
+      return new After(dbProvider, newValue);
+    }
   }
 }

gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java

@@ -214,7 +214,7 @@ public class IndexRewriteTest extends TestCase {
 
   private IndexedChangeQuery query(Predicate<ChangeData> p, int limit)
       throws QueryParseException {
-    return new IndexedChangeQuery(index, p, limit);
+    return new IndexedChangeQuery(null, index, p, limit);
   }
 
   private Set<Change.Status> status(String query) throws QueryParseException {

gerrit-server/src/test/java/com/google/gerrit/server/index/IndexedChangeQueryTest.java

@@ -0,0 +1,67 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.package com.google.gerrit.server.git;
+
+package com.google.gerrit.server.index;
+
+import static com.google.gerrit.server.index.IndexedChangeQuery.replaceSortKeyPredicates;
+
+import com.google.gerrit.server.query.Predicate;
+import com.google.gerrit.server.query.QueryParseException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+
+import junit.framework.TestCase;
+
+public class IndexedChangeQueryTest extends TestCase {
+  private FakeIndex index;
+  private ChangeQueryBuilder queryBuilder;
+
+  @Override
+  public void setUp() throws Exception {
+    super.setUp();
+    index = new FakeIndex(FakeIndex.V2);
+    IndexCollection indexes = new IndexCollection();
+    indexes.setSearchIndex(index);
+    queryBuilder = new FakeQueryBuilder(indexes);
+  }
+
+  public void testReplaceSortKeyPredicate_NoSortKey() throws Exception {
+    Predicate<ChangeData> p = parse("foo:a bar:b OR (foo:b bar:a)");
+    assertSame(p, replaceSortKeyPredicates(p, "1234"));
+  }
+
+  public void testReplaceSortKeyPredicate_TopLevelSortKey() throws Exception {
+    Predicate<ChangeData> p;
+    p = parse("foo:a bar:b sortkey_before:1234 OR (foo:b bar:a)");
+    assertEquals(parse("foo:a bar:b sortkey_before:5678 OR (foo:b bar:a)"),
+        replaceSortKeyPredicates(p, "5678"));
+    p = parse("foo:a bar:b sortkey_after:1234 OR (foo:b bar:a)");
+    assertEquals(parse("foo:a bar:b sortkey_after:5678 OR (foo:b bar:a)"),
+        replaceSortKeyPredicates(p, "5678"));
+  }
+
+  public void testReplaceSortKeyPredicate_NestedSortKey() throws Exception {
+    Predicate<ChangeData> p;
+    p = parse("foo:a bar:b OR (foo:b bar:a AND sortkey_before:1234)");
+    assertEquals(parse("foo:a bar:b OR (foo:b bar:a sortkey_before:5678)"),
+        replaceSortKeyPredicates(p, "5678"));
+    p = parse("foo:a bar:b OR (foo:b bar:a AND sortkey_after:1234)");
+    assertEquals(parse("foo:a bar:b OR (foo:b bar:a sortkey_after:5678)"),
+        replaceSortKeyPredicates(p, "5678"));
+  }
+
+  private Predicate<ChangeData> parse(String query) throws QueryParseException {
+    return queryBuilder.parse(query);
+  }
+}