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
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 2bfa39b..13efa6c 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 @@ 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 @@ @Override public ResultSet<ChangeData> 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 f491bc2..f99cce8 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.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 @@ return intTerm(ID_FIELD, cd.getId().get()); } - public static Query toQuery(Predicate<ChangeData> p) + public static Query toQuery(Schema<ChangeData> schema, Predicate<ChangeData> 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<ChangeData>) p); + return fieldQuery(schema, (IndexPredicate<ChangeData>) p); } else { throw new QueryParseException("cannot create query for index: " + p); } } - private static Query or(Predicate<ChangeData> p) throws QueryParseException { + private static Query or(Schema<ChangeData> schema, Predicate<ChangeData> 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 @@ } } - private static Query and(Predicate<ChangeData> p) throws QueryParseException { + private static Query and(Schema<ChangeData> schema, Predicate<ChangeData> p) + throws QueryParseException { try { BooleanQuery b = new BooleanQuery(); List<Query> not = Lists.newArrayListWithCapacity(p.getChildCount()); @@ -92,10 +95,10 @@ if (n instanceof TimestampRangePredicate) { b.add(notTimestamp((TimestampRangePredicate<ChangeData>) 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 @@ } } - private static Query not(Predicate<ChangeData> p) throws QueryParseException { + private static Query not(Schema<ChangeData> schema, Predicate<ChangeData> p) + throws QueryParseException { Predicate<ChangeData> n = p.getChild(0); if (n instanceof TimestampRangePredicate) { return notTimestamp((TimestampRangePredicate<ChangeData>) n); @@ -116,12 +120,12 @@ // 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<ChangeData> p) - throws QueryParseException { + private static Query fieldQuery(Schema<ChangeData> schema, + IndexPredicate<ChangeData> p) throws QueryParseException { if (p.getType() == FieldType.INTEGER) { return intQuery(p); } else if (p.getType() == FieldType.TIMESTAMP) { @@ -133,7 +137,7 @@ } 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 @@ return new TermQuery(intTerm(p.getField().getName(), value)); } - private static Query sortKeyQuery(SortKeyPredicate p) { + private static Query sortKeyQuery(Schema<ChangeData> 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<ChangeData> 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 5f70d36..2f1cfa0dc 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 @@ 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 20b61e7..cfa4644 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 @@ } }; - /** 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<ChangeData, Long> LEGACY_SORTKEY = + new FieldDef.Single<ChangeData, Long>( + "sortkey", FieldType.LONG, true) { + @Override + public Long get(ChangeData input, FillArgs args) + throws OrmException { + return legacyParseSortKey(input.change(args.db).getSortKey()); + } + }; + + /** + * Sort key field. + * <p> + * 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<ChangeData, Long> SORTKEY = new FieldDef.Single<ChangeData, Long>( "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 1ed6c47..39a29fb 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 @@ ChangeField.REF, ChangeField.TOPIC, ChangeField.UPDATED, - ChangeField.SORTKEY, + ChangeField.LEGACY_SORTKEY, ChangeField.FILE, ChangeField.OWNER, ChangeField.REVIEWER, @@ -58,6 +58,28 @@ 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<ChangeData> V3 = release( + ChangeField.LEGACY_ID, + ChangeField.ID, + ChangeField.STATUS, + ChangeField.PROJECT, + ChangeField.REF, + ChangeField.TOPIC, + ChangeField.UPDATED, ChangeField.SORTKEY, ChangeField.FILE, ChangeField.OWNER,
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 d440a6c..39abb59 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.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 @@ return ((IntPredicate<?>) find(p, IntPredicate.class, FIELD_LIMIT)).intValue(); } - public static boolean hasNonTrivialSortKeyAfter(Predicate<ChangeData> p) { + public static boolean hasNonTrivialSortKeyAfter(Schema<ChangeData> schema, + Predicate<ChangeData> 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<ChangeData> 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 5b4881d..a502746 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.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<ChangeData> { + @SuppressWarnings("deprecation") + private static long parseSortKey(Schema<ChangeData> schema, String value) { + FieldDef<ChangeData, ?> field = schema.getFields().get(ChangeField.SORTKEY.getName()); + if (field == ChangeField.SORTKEY) { + return ChangeUtil.parseSortKey(value); + } else { + return ChangeField.legacyParseSortKey(value); + } + } + protected final Provider<ReviewDb> dbProvider; - @SuppressWarnings("deprecation") SortKeyPredicate(Provider<ReviewDb> dbProvider, String name, String value) { super(ChangeField.SORTKEY, name, value); this.dbProvider = dbProvider; @@ -36,8 +47,8 @@ return 1; } - public abstract long getMinValue(); - public abstract long getMaxValue(); + public abstract long getMinValue(Schema<ChangeData> schema); + public abstract long getMaxValue(Schema<ChangeData> schema); public abstract SortKeyPredicate copy(String newValue); public static class Before extends SortKeyPredicate { @@ -46,13 +57,13 @@ } @Override - public long getMinValue() { + public long getMinValue(Schema<ChangeData> schema) { return 0; } @Override - public long getMaxValue() { - return ChangeUtil.parseSortKey(getValue()); + public long getMaxValue(Schema<ChangeData> schema) { + return parseSortKey(schema, getValue()); } @Override @@ -73,12 +84,12 @@ } @Override - public long getMinValue() { - return ChangeUtil.parseSortKey(getValue()); + public long getMinValue(Schema<ChangeData> schema) { + return parseSortKey(schema, getValue()); } @Override - public long getMaxValue() { + public long getMaxValue(Schema<ChangeData> 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 2a2c22f..2ddbecb 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 @@ 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 {