From 2e0f89d81448bf4b863a6e6acb5cb51ee0fef94d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 20 Sep 2013 14:55:15 -0700 Subject: [PATCH] Index full sortkey field in secondary index Using the sort key of the last element for pagination only works as long as every sort key is unique. This is true for the general definition of this field in the Change object, which includes the unique change ID at the end of the hex string. Previously, we were incorrectly truncating the change ID off, resulting in many changes in the same sort key bucket and thus broken pagination. Having two different definitions of sort key in the same running server makes it a bit ugly to handle SortKeyPredicates, since the definition of min/max value is now schema dependent, but at least we can keep the same field name. Don't @Deprecate the new SORTKEY field. We were originally hoping to remove this field and depend only on the UPDATED field (with the change ID as tiebreaker), but as long as this field is used for pagination, we have to keep it around. This is because we can't be sure a secondary index will be able to express a query like "(field X, field Y) > (N, M)" to allow us to restart a query in the middle of an UPDATED bucket. We may still decide to scrap the current pagination system but that will probably not happen until we kill the SQL index code. Change-Id: Icb760dbacd01939e5e4936ef87165b6dddcacdc0 --- .../gerrit/lucene/LuceneChangeIndex.java | 5 +-- .../google/gerrit/lucene/QueryBuilder.java | 44 +++++++++++-------- .../com/google/gerrit/server/ChangeUtil.java | 2 +- .../gerrit/server/index/ChangeField.java | 26 ++++++++++- .../gerrit/server/index/ChangeSchemas.java | 24 +++++++++- .../query/change/ChangeQueryBuilder.java | 6 ++- .../server/query/change/SortKeyPredicate.java | 29 ++++++++---- .../google/gerrit/solr/SolrChangeIndex.java | 4 +- 8 files changed, 102 insertions(+), 38 deletions(-) diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 2bfa39bcdc..13efa6cff2 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -250,8 +250,8 @@ public class LuceneChangeIndex implements ChangeIndex { if (!Sets.intersection(statuses, CLOSED_STATUSES).isEmpty()) { indexes.add(closedIndex); } - return new QuerySource(indexes, QueryBuilder.toQuery(p), limit, - ChangeQueryBuilder.hasNonTrivialSortKeyAfter(p)); + return new QuerySource(indexes, QueryBuilder.toQuery(schema, p), limit, + ChangeQueryBuilder.hasNonTrivialSortKeyAfter(schema, p)); } @Override @@ -300,7 +300,6 @@ public class LuceneChangeIndex implements ChangeIndex { @Override public ResultSet read() throws OrmException { IndexSearcher[] searchers = new IndexSearcher[indexes.size()]; - @SuppressWarnings("deprecation") Sort sort = new Sort( new SortField( ChangeField.SORTKEY.getName(), diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java index f491bc23c0..f99cce8aa3 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/QueryBuilder.java @@ -23,6 +23,7 @@ import com.google.gerrit.server.index.ChangeField; import com.google.gerrit.server.index.FieldType; import com.google.gerrit.server.index.IndexPredicate; import com.google.gerrit.server.index.RegexPredicate; +import com.google.gerrit.server.index.Schema; import com.google.gerrit.server.index.TimestampRangePredicate; import com.google.gerrit.server.query.AndPredicate; import com.google.gerrit.server.query.NotPredicate; @@ -54,26 +55,27 @@ public class QueryBuilder { return intTerm(ID_FIELD, cd.getId().get()); } - public static Query toQuery(Predicate p) + public static Query toQuery(Schema schema, Predicate p) throws QueryParseException { if (p instanceof AndPredicate) { - return and(p); + return and(schema, p); } else if (p instanceof OrPredicate) { - return or(p); + return or(schema, p); } else if (p instanceof NotPredicate) { - return not(p); + return not(schema, p); } else if (p instanceof IndexPredicate) { - return fieldQuery((IndexPredicate) p); + return fieldQuery(schema, (IndexPredicate) p); } else { throw new QueryParseException("cannot create query for index: " + p); } } - private static Query or(Predicate p) throws QueryParseException { + private static Query or(Schema schema, Predicate p) + throws QueryParseException { try { BooleanQuery q = new BooleanQuery(); for (int i = 0; i < p.getChildCount(); i++) { - q.add(toQuery(p.getChild(i)), SHOULD); + q.add(toQuery(schema, p.getChild(i)), SHOULD); } return q; } catch (BooleanQuery.TooManyClauses e) { @@ -81,7 +83,8 @@ public class QueryBuilder { } } - private static Query and(Predicate p) throws QueryParseException { + private static Query and(Schema schema, Predicate p) + throws QueryParseException { try { BooleanQuery b = new BooleanQuery(); List not = Lists.newArrayListWithCapacity(p.getChildCount()); @@ -92,10 +95,10 @@ public class QueryBuilder { if (n instanceof TimestampRangePredicate) { b.add(notTimestamp((TimestampRangePredicate) n), MUST); } else { - not.add(toQuery(n)); + not.add(toQuery(schema, n)); } } else { - b.add(toQuery(c), MUST); + b.add(toQuery(schema, c), MUST); } } for (Query q : not) { @@ -107,7 +110,8 @@ public class QueryBuilder { } } - private static Query not(Predicate p) throws QueryParseException { + private static Query not(Schema schema, Predicate p) + throws QueryParseException { Predicate n = p.getChild(0); if (n instanceof TimestampRangePredicate) { return notTimestamp((TimestampRangePredicate) n); @@ -116,12 +120,12 @@ public class QueryBuilder { // Lucene does not support negation, start with all and subtract. BooleanQuery q = new BooleanQuery(); q.add(new MatchAllDocsQuery(), MUST); - q.add(toQuery(n), MUST_NOT); + q.add(toQuery(schema, n), MUST_NOT); return q; } - private static Query fieldQuery(IndexPredicate p) - throws QueryParseException { + private static Query fieldQuery(Schema schema, + IndexPredicate p) throws QueryParseException { if (p.getType() == FieldType.INTEGER) { return intQuery(p); } else if (p.getType() == FieldType.TIMESTAMP) { @@ -133,7 +137,7 @@ public class QueryBuilder { } else if (p.getType() == FieldType.FULL_TEXT) { return fullTextQuery(p); } else if (p instanceof SortKeyPredicate) { - return sortKeyQuery((SortKeyPredicate) p); + return sortKeyQuery(schema, (SortKeyPredicate) p); } else { throw badFieldType(p.getType()); } @@ -158,12 +162,14 @@ public class QueryBuilder { return new TermQuery(intTerm(p.getField().getName(), value)); } - private static Query sortKeyQuery(SortKeyPredicate p) { + private static Query sortKeyQuery(Schema schema, SortKeyPredicate p) { + long min = p.getMinValue(schema); + long max = p.getMaxValue(schema); return NumericRangeQuery.newLongRange( p.getField().getName(), - p.getMinValue() != Long.MIN_VALUE ? p.getMinValue() : null, - p.getMaxValue() != Long.MAX_VALUE ? p.getMaxValue() : null, - true, true); + min != Long.MIN_VALUE ? min : null, + max != Long.MAX_VALUE ? max : null, + false, false); } private static Query timestampQuery(IndexPredicate p) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 5f70d361c5..2f1cfa0dcc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -478,7 +478,7 @@ public class ChangeUtil { if ("z".equals(sortKey)) { return Long.MAX_VALUE; } - return Long.parseLong(sortKey.substring(0, 8), 16); + return Long.parseLong(sortKey, 16); } public static void computeSortKey(final Change c) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java index 20b61e7900..cfa4644a64 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java @@ -125,8 +125,32 @@ public class ChangeField { } }; - /** Sort key field, duplicates {@link #UPDATED}. */ @Deprecated + public static long legacyParseSortKey(String sortKey) { + if ("z".equals(sortKey)) { + return Long.MAX_VALUE; + } + return Long.parseLong(sortKey.substring(0, 8), 16); + } + + /** Legacy sort key field. */ + @Deprecated + public static final FieldDef LEGACY_SORTKEY = + new FieldDef.Single( + "sortkey", FieldType.LONG, true) { + @Override + public Long get(ChangeData input, FillArgs args) + throws OrmException { + return legacyParseSortKey(input.change(args.db).getSortKey()); + } + }; + + /** + * Sort key field. + *

+ * Redundant with {@link #UPDATED} and {@link #LEGACY_ID}, but secondary index + * implementations may not be able to search over tuples of values. + */ public static final FieldDef SORTKEY = new FieldDef.Single( "sortkey", FieldType.LONG, true) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java index 1ed6c470a0..39a29fbd4c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeSchemas.java @@ -38,7 +38,7 @@ public class ChangeSchemas { ChangeField.REF, ChangeField.TOPIC, ChangeField.UPDATED, - ChangeField.SORTKEY, + ChangeField.LEGACY_SORTKEY, ChangeField.FILE, ChangeField.OWNER, ChangeField.REVIEWER, @@ -51,6 +51,28 @@ public class ChangeSchemas { @SuppressWarnings({"unchecked", "deprecation"}) static final Schema V2 = release( + ChangeField.LEGACY_ID, + ChangeField.ID, + ChangeField.STATUS, + ChangeField.PROJECT, + ChangeField.REF, + ChangeField.TOPIC, + ChangeField.UPDATED, + ChangeField.LEGACY_SORTKEY, + ChangeField.FILE, + ChangeField.OWNER, + ChangeField.REVIEWER, + ChangeField.COMMIT, + ChangeField.TR, + ChangeField.LABEL, + ChangeField.REVIEWED, + ChangeField.COMMIT_MESSAGE, + ChangeField.COMMENT, + ChangeField.CHANGE, + ChangeField.APPROVAL); + + @SuppressWarnings("unchecked") + static final Schema V3 = release( ChangeField.LEGACY_ID, ChangeField.ID, ChangeField.STATUS, 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 d440a6c6e4..39abb5947f 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 @@ -34,6 +34,7 @@ import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.git.GitRepositoryManager; 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.patch.PatchListCache; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; @@ -121,10 +122,11 @@ public class ChangeQueryBuilder extends QueryBuilder { return ((IntPredicate) find(p, IntPredicate.class, FIELD_LIMIT)).intValue(); } - public static boolean hasNonTrivialSortKeyAfter(Predicate p) { + public static boolean hasNonTrivialSortKeyAfter(Schema schema, + Predicate p) { SortKeyPredicate after = (SortKeyPredicate) find(p, SortKeyPredicate.class, "sortkey_after"); - return after != null && after.getMaxValue() > 0; + return after != null && after.getMaxValue(schema) > 0; } public static boolean hasSortKey(Predicate p) { 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 5b4881d721..a502746ca3 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 @@ -18,14 +18,25 @@ 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.ChangeField; +import com.google.gerrit.server.index.FieldDef; import com.google.gerrit.server.index.IndexPredicate; +import com.google.gerrit.server.index.Schema; import com.google.gwtorm.server.OrmException; import com.google.inject.Provider; public abstract class SortKeyPredicate extends IndexPredicate { + @SuppressWarnings("deprecation") + private static long parseSortKey(Schema schema, String value) { + FieldDef field = schema.getFields().get(ChangeField.SORTKEY.getName()); + if (field == ChangeField.SORTKEY) { + return ChangeUtil.parseSortKey(value); + } else { + return ChangeField.legacyParseSortKey(value); + } + } + protected final Provider dbProvider; - @SuppressWarnings("deprecation") SortKeyPredicate(Provider dbProvider, String name, String value) { super(ChangeField.SORTKEY, name, value); this.dbProvider = dbProvider; @@ -36,8 +47,8 @@ public abstract class SortKeyPredicate extends IndexPredicate { return 1; } - public abstract long getMinValue(); - public abstract long getMaxValue(); + public abstract long getMinValue(Schema schema); + public abstract long getMaxValue(Schema schema); public abstract SortKeyPredicate copy(String newValue); public static class Before extends SortKeyPredicate { @@ -46,13 +57,13 @@ public abstract class SortKeyPredicate extends IndexPredicate { } @Override - public long getMinValue() { + public long getMinValue(Schema schema) { return 0; } @Override - public long getMaxValue() { - return ChangeUtil.parseSortKey(getValue()); + public long getMaxValue(Schema schema) { + return parseSortKey(schema, getValue()); } @Override @@ -73,12 +84,12 @@ public abstract class SortKeyPredicate extends IndexPredicate { } @Override - public long getMinValue() { - return ChangeUtil.parseSortKey(getValue()); + public long getMinValue(Schema schema) { + return parseSortKey(schema, getValue()); } @Override - public long getMaxValue() { + public long getMaxValue(Schema schema) { return Long.MAX_VALUE; } diff --git a/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java b/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java index 2a2c22f4f5..2ddbecbd70 100644 --- a/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java +++ b/gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java @@ -205,8 +205,8 @@ class SolrChangeIndex implements ChangeIndex, LifecycleListener { if (!Sets.intersection(statuses, CLOSED_STATUSES).isEmpty()) { indexes.add(closedIndex); } - return new QuerySource(indexes, QueryBuilder.toQuery(p), limit, - ChangeQueryBuilder.hasNonTrivialSortKeyAfter(p)); + return new QuerySource(indexes, QueryBuilder.toQuery(schema, p), limit, + ChangeQueryBuilder.hasNonTrivialSortKeyAfter(schema, p)); } private void commit(SolrServer server) throws IOException {