Support pagination using offsets instead of sortkey

We cannot guarantee that secondary index implementations (particularly
the one used by googlesource.com) can efficiently paginate based on
the sortkey, and in particular can both sort and reverse sort on the
same field. This particular performance issue notwithstanding,
searching is now generally fast enough that it is feasible just to
skip the first N results when doing pagination.

Add an option S= to QueryChanges to support starting at a nonzero
offset. Note that we still have to fetch n+S results from the index in
order to do visibility filtering, since if we skipped at the index
layer we wouldn't know how many of the skipped elements would have
matched later filtering.

Drop the sortkey token suffix from the legacy anchor parser; there is
no reliable way to convert it to an offset, and it's unlikely that
users have permalinks to specific sortkey values.

On the server side, remove the sortkey field from the current index
version, and use pagination by offset instead of sortkey in the new
version only.

Continue to support sortkey queries against old index versions, to
support online reindexing while clients have an older JS version.

Change-Id: I6a82965db02c4d534e2107ca6ec91217085124d6

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

@@ -14,7 +14,7 @@
 
 package com.google.gerrit.server.query.change;
 
-import com.google.gerrit.server.index.ChangeField;
+import com.google.gerrit.server.index.Schema;
 import com.google.gerrit.server.index.TimestampRangePredicate;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gwtorm.server.OrmException;
@@ -24,8 +24,9 @@ import java.util.Date;
 public class AfterPredicate extends TimestampRangePredicate<ChangeData> {
   private final Date cut;
 
-  AfterPredicate(String value) throws QueryParseException {
-    super(ChangeField.UPDATED, ChangeQueryBuilder.FIELD_AFTER, value);
+  AfterPredicate(Schema<ChangeData> schema, String value)
+      throws QueryParseException {
+    super(updatedField(schema), ChangeQueryBuilder.FIELD_BEFORE, value);
     cut = parse(value);
   }
 

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

@@ -19,7 +19,7 @@ import static java.util.concurrent.TimeUnit.SECONDS;
 
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.config.ConfigUtil;
-import com.google.gerrit.server.index.ChangeField;
+import com.google.gerrit.server.index.Schema;
 import com.google.gerrit.server.index.TimestampRangePredicate;
 import com.google.gerrit.server.util.TimeUtil;
 import com.google.gwtorm.server.OrmException;
@@ -29,8 +29,8 @@ import java.sql.Timestamp;
 public class AgePredicate extends TimestampRangePredicate<ChangeData> {
   private final long cut;
 
-  AgePredicate(String value) {
-    super(ChangeField.UPDATED, ChangeQueryBuilder.FIELD_AGE, value);
+  AgePredicate(Schema<ChangeData> schema, String value) {
+    super(updatedField(schema), ChangeQueryBuilder.FIELD_AGE, value);
 
     long s = ConfigUtil.getTimeUnit(getValue(), 0, SECONDS);
     long ms = MILLISECONDS.convert(s, SECONDS);
@@ -47,10 +47,6 @@ public class AgePredicate extends TimestampRangePredicate<ChangeData> {
     return new Timestamp(cut);
   }
 
-  long getCut() {
-    return cut + 1;
-  }
-
   @Override
   public boolean match(final ChangeData object) throws OrmException {
     Change change = object.change();

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

@@ -14,9 +14,12 @@
 
 package com.google.gerrit.server.query.change;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.google.common.base.Function;
 import com.google.common.base.Throwables;
 import com.google.common.collect.FluentIterable;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.gerrit.server.query.AndPredicate;
@@ -71,10 +74,18 @@ public class AndSource extends AndPredicate<ChangeData>
     return r;
   }
 
+  private final int start;
   private int cardinality = -1;
 
   public AndSource(Collection<? extends Predicate<ChangeData>> that) {
+    this(that, 0);
+  }
+
+  public AndSource(Collection<? extends Predicate<ChangeData>> that,
+      int start) {
     super(sort(that));
+    checkArgument(start >= 0, "negative start: %s", start);
+    this.start = start;
   }
 
   @Override
@@ -98,9 +109,13 @@ public class AndSource extends AndPredicate<ChangeData>
     if (source == null) {
       throw new OrmException("No ChangeDataSource: " + this);
     }
+    @SuppressWarnings("unchecked")
+    Predicate<ChangeData> pred = (Predicate<ChangeData>) source;
+    boolean useSortKey = ChangeQueryBuilder.hasSortKey(pred);
 
     List<ChangeData> r = Lists.newArrayList();
     ChangeData last = null;
+    int nextStart = 0;
     boolean skipped = false;
     for (ChangeData data : buffer(source, source.read())) {
       if (match(data)) {
@@ -109,6 +124,7 @@ public class AndSource extends AndPredicate<ChangeData>
         skipped = true;
       }
       last = data;
+      nextStart++;
     }
 
     if (skipped && last != null && source instanceof Paginated) {
@@ -117,21 +133,31 @@ public class AndSource extends AndPredicate<ChangeData>
       // limit the caller wants.  Restart the source and continue.
       //
       Paginated p = (Paginated) source;
-      while (skipped && r.size() < p.limit()) {
+      while (skipped && r.size() < p.limit() + start) {
         ChangeData lastBeforeRestart = last;
         skipped = false;
         last = null;
-        for (ChangeData data : buffer(source, p.restart(lastBeforeRestart))) {
+        ResultSet<ChangeData> next = useSortKey
+            ? p.restart(lastBeforeRestart)
+            : p.restart(nextStart);
+
+        for (ChangeData data : buffer(source, next)) {
           if (match(data)) {
             r.add(data);
           } else {
             skipped = true;
           }
           last = data;
+          nextStart++;
         }
       }
     }
 
+    if (start >= r.size()) {
+      r = ImmutableList.of();
+    } else if (start > 0) {
+      r = ImmutableList.copyOf(r.subList(start, r.size()));
+    }
     return new ListResultSet<ChangeData>(r);
   }
 

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

@@ -14,13 +14,8 @@
 
 package com.google.gerrit.server.query.change;
 
-import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.index.ChangeIndex;
-import com.google.gerrit.server.index.IndexCollection;
-import com.google.gerrit.server.index.Schema;
 import com.google.gerrit.server.query.IntPredicate;
 import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryRewriter;
@@ -41,19 +36,12 @@ public class BasicChangeRewrites extends QueryRewriter<ChangeData> {
       new QueryRewriter.Definition<ChangeData, BasicChangeRewrites>(
           BasicChangeRewrites.class, BUILDER);
 
-  static Schema<ChangeData> schema(@Nullable IndexCollection indexes) {
-    ChangeIndex index = indexes != null ? indexes.getSearchIndex() : null;
-    return index != null ? index.getSchema() : null;
-  }
-
   protected final Provider<ReviewDb> dbProvider;
-  private final IndexCollection indexes;
 
   @Inject
-  public BasicChangeRewrites(Provider<ReviewDb> dbProvider, IndexCollection indexes) {
+  public BasicChangeRewrites(Provider<ReviewDb> dbProvider) {
     super(mydef);
     this.dbProvider = dbProvider;
-    this.indexes = indexes;
   }
 
   @Rewrite("-status:open")
@@ -84,14 +72,6 @@ public class BasicChangeRewrites extends QueryRewriter<ChangeData> {
         new ChangeStatusPredicate(Change.Status.MERGED));
   }
 
-  @SuppressWarnings("unchecked")
-  @NoCostComputation
-  @Rewrite("sortkey_before:z A=(age:*)")
-  public Predicate<ChangeData> r00_ageToSortKey(@Named("A") AgePredicate a) {
-    String cut = ChangeUtil.sortKey(a.getCut(), Integer.MAX_VALUE);
-    return and(new SortKeyPredicate.Before(schema(indexes), dbProvider, cut), a);
-  }
-
   @NoCostComputation
   @Rewrite("A=(limit:*) B=(limit:*)")
   public Predicate<ChangeData> r00_smallestLimit(
@@ -100,21 +80,6 @@ public class BasicChangeRewrites extends QueryRewriter<ChangeData> {
     return a.intValue() <= b.intValue() ? a : b;
   }
 
-  @NoCostComputation
-  @Rewrite("A=(sortkey_before:*) B=(sortkey_before:*)")
-  public Predicate<ChangeData> r00_oldestSortKey(
-      @Named("A") SortKeyPredicate.Before a,
-      @Named("B") SortKeyPredicate.Before b) {
-    return a.getValue().compareTo(b.getValue()) <= 0 ? a : b;
-  }
-
-  @NoCostComputation
-  @Rewrite("A=(sortkey_after:*) B=(sortkey_after:*)")
-  public Predicate<ChangeData> r00_newestSortKey(
-      @Named("A") SortKeyPredicate.After a, @Named("B") SortKeyPredicate.After b) {
-    return a.getValue().compareTo(b.getValue()) >= 0 ? a : b;
-  }
-
   private static final class InvalidProvider<T> implements Provider<T> {
     @Override
     public T get() {

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

@@ -14,7 +14,7 @@
 
 package com.google.gerrit.server.query.change;
 
-import com.google.gerrit.server.index.ChangeField;
+import com.google.gerrit.server.index.Schema;
 import com.google.gerrit.server.index.TimestampRangePredicate;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gwtorm.server.OrmException;
@@ -24,8 +24,9 @@ import java.util.Date;
 public class BeforePredicate extends TimestampRangePredicate<ChangeData> {
   private final Date cut;
 
-  BeforePredicate(String value) throws QueryParseException {
-    super(ChangeField.UPDATED, ChangeQueryBuilder.FIELD_BEFORE, value);
+  BeforePredicate(Schema<ChangeData> schema, String value)
+      throws QueryParseException {
+    super(updatedField(schema), ChangeQueryBuilder.FIELD_BEFORE, value);
     cut = parse(value);
   }
 

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.query.change;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.Lists;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -124,13 +125,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
           ChangeQueryBuilder.class);
 
   @SuppressWarnings("unchecked")
-  public static boolean hasLimit(Predicate<ChangeData> p) {
-    return find(p, IntPredicate.class, FIELD_LIMIT) != null;
-  }
-
-  @SuppressWarnings("unchecked")
-  public static int getLimit(Predicate<ChangeData> p) {
-    return ((IntPredicate<?>) find(p, IntPredicate.class, FIELD_LIMIT)).intValue();
+  public static Integer getLimit(Predicate<ChangeData> p) {
+    IntPredicate<?> ip =
+        (IntPredicate<?>) find(p, IntPredicate.class, FIELD_LIMIT);
+    return ip != null ? ip.intValue() : null;
   }
 
   public static boolean hasNonTrivialSortKeyAfter(Schema<ChangeData> schema,
@@ -237,12 +235,12 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
 
   @Operator
   public Predicate<ChangeData> age(String value) {
-    return new AgePredicate(value);
+    return new AgePredicate(schema(args.indexes), value);
   }
 
   @Operator
   public Predicate<ChangeData> before(String value) throws QueryParseException {
-    return new BeforePredicate(value);
+    return new BeforePredicate(schema(args.indexes), value);
   }
 
   @Operator
@@ -252,7 +250,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
 
   @Operator
   public Predicate<ChangeData> after(String value) throws QueryParseException {
-    return new AfterPredicate(value);
+    return new AfterPredicate(schema(args.indexes), value);
   }
 
   @Operator
@@ -660,16 +658,18 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     return new LimitPredicate(limit);
   }
 
+  boolean supportsSortKey() {
+    return SortKeyPredicate.hasSortKeyField(schema(args.indexes));
+  }
+
   @Operator
   public Predicate<ChangeData> sortkey_after(String sortKey) {
-    return new SortKeyPredicate.After(
-        BasicChangeRewrites.schema(args.indexes), args.db, sortKey);
+    return new SortKeyPredicate.After(schema(args.indexes), args.db, sortKey);
   }
 
   @Operator
   public Predicate<ChangeData> sortkey_before(String sortKey) {
-    return new SortKeyPredicate.Before(
-        BasicChangeRewrites.schema(args.indexes), args.db, sortKey);
+    return new SortKeyPredicate.Before(schema(args.indexes), args.db, sortKey);
   }
 
   @Operator
@@ -778,4 +778,9 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     }
     throw new IllegalArgumentException();
   }
+
+  private static Schema<ChangeData> schema(@Nullable IndexCollection indexes) {
+    ChangeIndex index = indexes != null ? indexes.getSearchIndex() : null;
+    return index != null ? index.getSchema() : null;
+  }
 }

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

@@ -18,6 +18,6 @@ import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryParseException;
 
 public interface ChangeQueryRewriter {
-  Predicate<ChangeData> rewrite(Predicate<ChangeData> in)
+  Predicate<ChangeData> rewrite(Predicate<ChangeData> in, int start)
       throws QueryParseException;
 }

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

@@ -36,7 +36,7 @@ class CommentPredicate extends IndexPredicate<ChangeData> {
   public boolean match(ChangeData object) throws OrmException {
     try {
       for (ChangeData cData : index.getSource(
-          Predicate.and(new LegacyChangeIdPredicate(args, object.getId()), this), 1)
+          Predicate.and(new LegacyChangeIdPredicate(args, object.getId()), this), 0, 1)
           .read()) {
         if (cData.getId().equals(object.getId())) {
           return true;

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

@@ -40,7 +40,7 @@ class MessagePredicate extends IndexPredicate<ChangeData> {
   public boolean match(ChangeData object) throws OrmException {
     try {
       for (ChangeData cData : index.getSource(
-          Predicate.and(new LegacyChangeIdPredicate(args, object.getId()), this), 1)
+          Predicate.and(new LegacyChangeIdPredicate(args, object.getId()), this), 0, 1)
           .read()) {
         if (cData.getId().equals(object.getId())) {
           return true;

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

@@ -21,4 +21,6 @@ public interface Paginated {
   int limit();
 
   ResultSet<ChangeData> restart(ChangeData last) throws OrmException;
+
+  ResultSet<ChangeData> restart(int start) throws OrmException;
 }

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

@@ -80,6 +80,11 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
     imp.setSortkeyBefore(key);
   }
 
+  @Option(name = "-S", metaVar = "CNT", usage = "Number of changes to skip")
+  public void setStart(int start) {
+    imp.setStart(start);
+  }
+
   @Inject
   QueryChanges(ChangeJson json, QueryProcessor qp, Provider<CurrentUser> user) {
     this.json = json;

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.query.change;
 
+import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.data.GlobalCapability;
@@ -103,6 +104,7 @@ public class QueryProcessor {
 
   private OutputFormat outputFormat = OutputFormat.TEXT;
   private int limit;
+  private int start;
   private String sortkeyAfter;
   private String sortkeyBefore;
   private boolean includePatchSets;
@@ -144,6 +146,10 @@ public class QueryProcessor {
     limit = n;
   }
 
+  void setStart(int n) {
+    start = n;
+  }
+
   void setSortkeyAfter(String sortkey) {
     sortkeyAfter = sortkey;
   }
@@ -240,17 +246,17 @@ public class QueryProcessor {
     List<ChangeDataSource> sources = Lists.newArrayListWithCapacity(cnt);
     for (String query : queries) {
       Predicate<ChangeData> q = parseQuery(query, visibleToMe);
-      Predicate<ChangeData> s = queryRewriter.rewrite(q);
+      Predicate<ChangeData> s = queryRewriter.rewrite(q, start);
       if (!(s instanceof ChangeDataSource)) {
         q = Predicate.and(queryBuilder.status_open(), q);
-        s = queryRewriter.rewrite(q);
+        s = queryRewriter.rewrite(q, start);
       }
       if (!(s instanceof ChangeDataSource)) {
         throw new QueryParseException("invalid query: " + s);
       }
 
       // Don't trust QueryRewriter to have left the visible predicate.
-      AndSource a = new AndSource(ImmutableList.of(s, visibleToMe));
+      AndSource a = new AndSource(ImmutableList.of(s, visibleToMe), start);
       limits.add(limit(q));
       sources.add(a);
     }
@@ -264,7 +270,11 @@ public class QueryProcessor {
     List<List<ChangeData>> out = Lists.newArrayListWithCapacity(cnt);
     for (int i = 0; i < cnt; i++) {
       List<ChangeData> results = matches.get(i).toList();
-      Collections.sort(results, sortkeyAfter != null ? cmpAfter : cmpBefore);
+      if (sortkeyAfter != null) {
+        Collections.sort(results, cmpAfter);
+      } else if (sortkeyBefore != null) {
+        Collections.sort(results, cmpBefore);
+      }
       if (results.size() > maxLimit) {
         moreResults = true;
       }
@@ -406,16 +416,14 @@ public class QueryProcessor {
   }
 
   private int limit(Predicate<ChangeData> s) {
-    int n = ChangeQueryBuilder.hasLimit(s)
-        ? ChangeQueryBuilder.getLimit(s)
-        : maxLimit;
+    int n = Objects.firstNonNull(ChangeQueryBuilder.getLimit(s), maxLimit);
     return limit > 0 ? Math.min(n, limit) + 1 : n + 1;
   }
 
   private Predicate<ChangeData> parseQuery(String queryString,
       final Predicate<ChangeData> visibleToMe) throws QueryParseException {
     Predicate<ChangeData> q = queryBuilder.parse(queryString);
-    if (!ChangeQueryBuilder.hasSortKey(q)) {
+    if (queryBuilder.supportsSortKey() && !ChangeQueryBuilder.hasSortKey(q)) {
       if (sortkeyBefore != null) {
         q = Predicate.and(q, queryBuilder.sortkey_before(sortkeyBefore));
       } else if (sortkeyAfter != null) {

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

@@ -29,6 +29,10 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Provider;
 
 public abstract class SortKeyPredicate extends IndexPredicate<ChangeData> {
+  public static boolean hasSortKeyField(Schema<ChangeData> schema) {
+    return sortkeyFieldOrNull(schema) != null;
+  }
+
   @SuppressWarnings("deprecation")
   private static long parseSortKey(Schema<ChangeData> schema, String value) {
     FieldDef<ChangeData, ?> field = schema.getFields().get(SORTKEY.getName());
@@ -40,7 +44,8 @@ public abstract class SortKeyPredicate extends IndexPredicate<ChangeData> {
   }
 
   @SuppressWarnings("deprecation")
-  private static FieldDef<ChangeData, ?> sortkeyField(Schema<ChangeData> schema) {
+  private static FieldDef<ChangeData, ?> sortkeyFieldOrNull(
+      Schema<ChangeData> schema) {
     if (schema == null) {
       return ChangeField.LEGACY_SORTKEY;
     }
@@ -48,9 +53,13 @@ public abstract class SortKeyPredicate extends IndexPredicate<ChangeData> {
     if (f != null) {
       return f;
     }
+    return schema.getFields().get(ChangeField.LEGACY_SORTKEY.getName());
+  }
+
+  private static FieldDef<ChangeData, ?> sortkeyField(Schema<ChangeData> schema) {
     return checkNotNull(
-        schema.getFields().get(ChangeField.LEGACY_SORTKEY.getName()),
-        "schema missing sortkey field, found: %s", schema.getFields().keySet());
+        sortkeyFieldOrNull(schema),
+        "schema missing sortkey field, found: %s", schema);
   }
 
   protected final Schema<ChangeData> schema;