From acdc155f13dae65a43d223766fdd06cdd5b59c59 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 23 Sep 2013 14:44:35 -0700 Subject: [PATCH] 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/index/IndexRewriteImpl.java | 6 +- .../server/index/IndexedChangeQuery.java | 89 ++++++++++++++++--- .../gerrit/server/query/change/Paginated.java | 2 +- .../server/query/change/SortKeyPredicate.java | 11 +++ .../gerrit/server/index/IndexRewriteTest.java | 2 +- .../server/index/IndexedChangeQueryTest.java | 67 ++++++++++++++ 6 files changed, 158 insertions(+), 19 deletions(-) create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/index/IndexedChangeQueryTest.java diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexRewriteImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexRewriteImpl.java index aab6c64f38..bbc4773db1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexRewriteImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexRewriteImpl.java @@ -154,7 +154,7 @@ public class IndexRewriteImpl implements ChangeQueryRewriter { Predicate 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); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java index 974e29112d..2185af3917 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java +++ b/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 - implements ChangeDataSource { - private final ChangeIndex index; - private final Predicate pred; - private final int limit; - private final ChangeDataSource source; + implements ChangeDataSource, Paginated { - public IndexedChangeQuery(ChangeIndex index, Predicate pred, int limit) - throws QueryParseException { + /** + * Replace all {@link SortKeyPredicate}s in a tree. + *

+ * 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 replaceSortKeyPredicates( + Predicate p, String newValue) { + if (p instanceof SortKeyPredicate) { + return ((SortKeyPredicate) p).copy(newValue); + } else if (p.getChildCount() > 0) { + List> newChildren = + Lists.newArrayListWithCapacity(p.getChildCount()); + boolean replaced = false; + for (Predicate c : p.getChildren()) { + Predicate nc = replaceSortKeyPredicates(c, newValue); + newChildren.add(nc); + if (nc != c) { + replaced = true; + } + } + return replaced ? p.copy(newChildren) : p; + } else { + return p; + } + } + + private final Provider db; + private final ChangeIndex index; + private final int limit; + + private Predicate pred; + private ChangeDataSource source; + + public IndexedChangeQuery(Provider db, ChangeIndex index, + Predicate 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 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 @Override public ResultSet read() throws OrmException { + final ChangeDataSource currSource; + try { + currSource = index.getSource(pred, limit); + } catch (QueryParseException e) { + throw new OrmException(e); + } + source = currSource; final ResultSet rs = source.read(); + return new ResultSet() { @Override public Iterator iterator() { @@ -94,7 +149,7 @@ public class IndexedChangeQuery extends Predicate @Override public ChangeData apply(ChangeData input) { - input.cacheFromSource(source); + input.cacheFromSource(currSource); return input; } }).iterator(); @@ -104,7 +159,7 @@ public class IndexedChangeQuery extends Predicate public List toList() { List r = rs.toList(); for (ChangeData cd : r) { - cd.cacheFromSource(source); + cd.cacheFromSource(currSource); } return r; } @@ -116,6 +171,12 @@ public class IndexedChangeQuery extends Predicate }; } + @Override + public ResultSet restart(ChangeData last) throws OrmException { + pred = replaceSortKeyPredicates(pred, last.change(db).getSortKey()); + return read(); + } + @Override public Predicate copy( Collection> children) { @@ -124,7 +185,7 @@ public class IndexedChangeQuery extends Predicate @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 @Override public String toString() { return Objects.toStringHelper("index") - .add("p", source) + .add("p", pred) .add("limit", limit) .toString(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/Paginated.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/Paginated.java index 2b3edf7a2f..d9ff80ca23 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/Paginated.java +++ b/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 restart(ChangeData last) throws OrmException; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SortKeyPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SortKeyPredicate.java index 35a42f6e0b..5b4881d721 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SortKeyPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SortKeyPredicate.java @@ -38,6 +38,7 @@ public abstract class SortKeyPredicate extends IndexPredicate { public abstract long getMinValue(); public abstract long getMaxValue(); + public abstract SortKeyPredicate copy(String newValue); public static class Before extends SortKeyPredicate { Before(Provider dbProvider, String value) { @@ -59,6 +60,11 @@ public abstract class SortKeyPredicate extends IndexPredicate { 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 { Change change = cd.change(dbProvider); return change != null && change.getSortKey().compareTo(getValue()) > 0; } + + @Override + public After copy(String newValue) { + return new After(dbProvider, newValue); + } } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java index fb3f9bad2d..08617b6850 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java +++ b/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 p, int limit) throws QueryParseException { - return new IndexedChangeQuery(index, p, limit); + return new IndexedChangeQuery(null, index, p, limit); } private Set status(String query) throws QueryParseException { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexedChangeQueryTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexedChangeQueryTest.java new file mode 100644 index 0000000000..ba1f53d6e8 --- /dev/null +++ b/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 p = parse("foo:a bar:b OR (foo:b bar:a)"); + assertSame(p, replaceSortKeyPredicates(p, "1234")); + } + + public void testReplaceSortKeyPredicate_TopLevelSortKey() throws Exception { + Predicate 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 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 parse(String query) throws QueryParseException { + return queryBuilder.parse(query); + } +}