From 67758fa8fc39d553b81444728698d5b939c22cba Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 28 Oct 2015 11:08:31 -0400 Subject: [PATCH] Allow customizing the set of fields returned by the index Callers may want to fine-tune the set of fields returned to avoid deserializing fields they know in advance aren't going to be used. Alternatively, they may want to force rereading data from the database instead of returning possibly-stale data from the index. (Although this does not prevent stale documents from showing up in the search results in the first place.) Add a "fields" field to QueryOptions to customize this behavior. For the Lucene case, we need to fudge the fields a bit so we always return at least one field containing the Change.Id, otherwise we wouldn't be able to even construct a ChangeData for the result. Change-Id: Ia365f71e1e380d87988e5d277729f0d339d6d3d1 --- .../gerrit/lucene/LuceneChangeIndex.java | 86 ++++++++++++++----- .../server/index/IndexedChangeQuery.java | 2 +- .../google/gerrit/server/index/Schema.java | 15 ++++ .../server/query/change/QueryOptions.java | 17 ++-- .../server/query/change/QueryProcessor.java | 26 +++++- .../server/index/IndexRewriterTest.java | 4 +- .../change/AbstractQueryChangesTest.java | 25 ++++++ .../change/LuceneQueryChangesV14Test.java | 7 ++ 8 files changed, 152 insertions(+), 30 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 5c3b6294f9..c05c8f0459 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 @@ -131,7 +131,6 @@ public class LuceneChangeIndex implements ChangeIndex { private static final String APPROVAL_FIELD = ChangeField.APPROVAL.getName(); private static final String CHANGE_FIELD = ChangeField.CHANGE.getName(); private static final String DELETED_FIELD = ChangeField.DELETED.getName(); - private static final String ID_FIELD = ChangeField.LEGACY_ID2.getName(); private static final String MERGEABLE_FIELD = ChangeField.MERGEABLE.getName(); private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName(); private static final String REVIEWEDBY_FIELD = @@ -139,10 +138,6 @@ public class LuceneChangeIndex implements ChangeIndex { private static final String UPDATED_SORT_FIELD = sortFieldName(ChangeField.UPDATED); - private static final ImmutableSet FIELDS = ImmutableSet.of( - ADDED_FIELD, APPROVAL_FIELD, CHANGE_FIELD, DELETED_FIELD, ID_FIELD, - MERGEABLE_FIELD, PATCH_SET_FIELD, REVIEWEDBY_FIELD); - private static final Map CUSTOM_CHAR_MAPPING = ImmutableMap.of( "_", " ", ".", " "); @@ -438,10 +433,12 @@ public class LuceneChangeIndex implements ChangeIndex { List result = Lists.newArrayListWithCapacity(docs.scoreDocs.length); + Set fields = fields(opts); + String idFieldName = idFieldName(); for (int i = opts.start(); i < docs.scoreDocs.length; i++) { ScoreDoc sd = docs.scoreDocs[i]; - Document doc = searchers[sd.shardIndex].doc(sd.doc, FIELDS); - result.add(toChangeData(doc)); + Document doc = searchers[sd.shardIndex].doc(sd.doc, fields); + result.add(toChangeData(doc, fields, idFieldName)); } final List r = Collections.unmodifiableList(result); @@ -477,19 +474,62 @@ public class LuceneChangeIndex implements ChangeIndex { } } - private ChangeData toChangeData(Document doc) { + @SuppressWarnings("deprecation") + private Set fields(QueryOptions opts) { + if (schemaHasRequestedField(ChangeField.LEGACY_ID2, opts.fields()) + || schemaHasRequestedField(ChangeField.CHANGE, opts.fields()) + || schemaHasRequestedField(ChangeField.LEGACY_ID, opts.fields())) { + return opts.fields(); + } + // Request the numeric ID field even if the caller did not request it, + // otherwise we can't actually construct a ChangeData. + return Sets.union(opts.fields(), ImmutableSet.of(idFieldName())); + } + + private boolean schemaHasRequestedField(FieldDef field, + Set requested) { + return schema.hasField(field) && requested.contains(field.getName()); + } + + @SuppressWarnings("deprecation") + private String idFieldName() { + return schema.getField(ChangeField.LEGACY_ID2, ChangeField.LEGACY_ID).get() + .getName(); + } + + private ChangeData toChangeData(Document doc, Set fields, + String idFieldName) { + ChangeData cd; + // Either change or the ID field was guaranteed to be included in the call + // to fields() above. BytesRef cb = doc.getBinaryValue(CHANGE_FIELD); - if (cb == null) { - int id = doc.getField(ID_FIELD).numericValue().intValue(); - return changeDataFactory.create(db.get(), new Change.Id(id)); + if (cb != null) { + cd = changeDataFactory.create(db.get(), + ChangeProtoField.CODEC.decode(cb.bytes, cb.offset, cb.length)); + } else { + int id = doc.getField(idFieldName).numericValue().intValue(); + cd = changeDataFactory.create(db.get(), new Change.Id(id)); } - // Change proto. - Change change = ChangeProtoField.CODEC.decode( - cb.bytes, cb.offset, cb.length); - ChangeData cd = changeDataFactory.create(db.get(), change); + if (fields.contains(PATCH_SET_FIELD)) { + decodePatchSets(doc, cd); + } + if (fields.contains(APPROVAL_FIELD)) { + decodeApprovals(doc, cd); + } + if (fields.contains(ADDED_FIELD) && fields.contains(DELETED_FIELD)) { + decodeChangedLines(doc, cd); + } + if (fields.contains(MERGEABLE_FIELD)) { + decodeMergeable(doc, cd); + } + if (fields.contains(REVIEWEDBY_FIELD)) { + decodeReviewedBy(doc, cd); + } + return cd; + } - // Patch sets. + private void decodePatchSets(Document doc, ChangeData cd) { List patchSets = decodeProtos(doc, PATCH_SET_FIELD, PatchSetProtoField.CODEC); if (!patchSets.isEmpty()) { @@ -497,12 +537,14 @@ public class LuceneChangeIndex implements ChangeIndex { // this cannot be valid since a change needs at least one patch set. cd.setPatchSets(patchSets); } + } - // Approvals. + private void decodeApprovals(Document doc, ChangeData cd) { cd.setCurrentApprovals( decodeProtos(doc, APPROVAL_FIELD, PatchSetApprovalProtoField.CODEC)); + } - // Changed lines. + private void decodeChangedLines(Document doc, ChangeData cd) { IndexableField added = doc.getField(ADDED_FIELD); IndexableField deleted = doc.getField(DELETED_FIELD); if (added != null && deleted != null) { @@ -510,16 +552,18 @@ public class LuceneChangeIndex implements ChangeIndex { added.numericValue().intValue(), deleted.numericValue().intValue()); } + } - // Mergeable. + private void decodeMergeable(Document doc, ChangeData cd) { String mergeable = doc.get(MERGEABLE_FIELD); if ("1".equals(mergeable)) { cd.setMergeable(true); } else if ("0".equals(mergeable)) { cd.setMergeable(false); } + } - // Reviewed-by. + private void decodeReviewedBy(Document doc, ChangeData cd) { IndexableField[] reviewedBy = doc.getFields(REVIEWEDBY_FIELD); if (reviewedBy.length > 0) { Set accounts = @@ -533,8 +577,6 @@ public class LuceneChangeIndex implements ChangeIndex { } cd.setReviewedBy(accounts); } - - return cd; } private static List decodeProtos(Document doc, String fieldName, 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 683f8cfe6d..28fa9f80fd 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 @@ -50,7 +50,7 @@ public class IndexedChangeQuery extends Predicate int backendLimit = opts.config().maxLimit(); int limit = Ints.saturatedCast((long) opts.limit() + opts.start()); limit = Math.min(limit, backendLimit); - return QueryOptions.create(opts.config(), 0, limit); + return QueryOptions.create(opts.config(), 0, limit, opts.fields()); } private final ChangeIndex index; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java index df70292bf9..a605461d44 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java @@ -61,6 +61,8 @@ public class Schema { } private final ImmutableMap> fields; + private final ImmutableMap> storedFields; + private int version; protected Schema(Iterable> fields) { @@ -71,10 +73,15 @@ public class Schema { public Schema(int version, Iterable> fields) { this.version = version; ImmutableMap.Builder> b = ImmutableMap.builder(); + ImmutableMap.Builder> sb = ImmutableMap.builder(); for (FieldDef f : fields) { b.put(f.getName(), f); + if (f.isStored()) { + sb.put(f.getName(), f); + } } this.fields = b.build(); + this.storedFields = sb.build(); } public final int getVersion() { @@ -94,6 +101,14 @@ public class Schema { return fields; } + /** + * @return all fields in this schema where {@link FieldDef#isStored()} is + * true. + */ + public final ImmutableMap> getStoredFields() { + return storedFields; + } + /** * Look up fields in this schema. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java index 1964fa56b4..a70f892b02 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java @@ -17,29 +17,36 @@ package com.google.gerrit.server.query.change; import static com.google.common.base.Preconditions.checkArgument; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.server.index.IndexConfig; +import java.util.Set; + @AutoValue public abstract class QueryOptions { - public static QueryOptions create(IndexConfig config, int start, int limit) { + public static QueryOptions create(IndexConfig config, int start, int limit, + Set fields) { checkArgument(start >= 0, "start must be nonnegative: %s", start); checkArgument(limit > 0, "limit must be positive: %s", limit); - return new AutoValue_QueryOptions(config, start, limit); + return new AutoValue_QueryOptions(config, start, limit, + ImmutableSet.copyOf(fields)); } public static QueryOptions oneResult() { - return create(IndexConfig.createDefault(), 0, 1); + return create(IndexConfig.createDefault(), 0, 1, + ImmutableSet. of()); } public abstract IndexConfig config(); public abstract int start(); public abstract int limit(); + public abstract ImmutableSet fields(); public QueryOptions withLimit(int newLimit) { - return create(config(), start(), newLimit); + return create(config(), start(), newLimit, fields()); } public QueryOptions withStart(int newStart) { - return create(config(), newStart, limit()); + return create(config(), newStart, limit(), fields()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java index c3c70b379d..1a6ae02a0f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Ordering; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.metrics.Description; @@ -25,6 +26,8 @@ import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.metrics.Timer0; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.index.ChangeIndex; +import com.google.gerrit.server.index.IndexCollection; import com.google.gerrit.server.index.IndexConfig; import com.google.gerrit.server.index.IndexPredicate; import com.google.gerrit.server.index.IndexRewriter; @@ -39,11 +42,13 @@ import com.google.inject.Singleton; import java.util.ArrayList; import java.util.List; +import java.util.Set; public class QueryProcessor { private final Provider db; private final Provider userProvider; private final ChangeControl.GenericFactory changeControlFactory; + private final IndexCollection indexes; private final IndexRewriter rewriter; private final IndexConfig indexConfig; private final Metrics metrics; @@ -51,17 +56,20 @@ public class QueryProcessor { private int limitFromCaller; private int start; private boolean enforceVisibility = true; + private Set requestedFields; @Inject QueryProcessor(Provider db, Provider userProvider, ChangeControl.GenericFactory changeControlFactory, + IndexCollection indexes, IndexRewriter rewriter, IndexConfig indexConfig, Metrics metrics) { this.db = db; this.userProvider = userProvider; this.changeControlFactory = changeControlFactory; + this.indexes = indexes; this.rewriter = rewriter; this.indexConfig = indexConfig; this.metrics = metrics; @@ -82,6 +90,11 @@ public class QueryProcessor { return this; } + public QueryProcessor setRequestedFields(Set fields) { + requestedFields = fields; + return this; + } + /** * Query for changes that match a structured query. * @@ -150,7 +163,8 @@ public class QueryProcessor { "Cannot go beyond page " + indexConfig.maxPages() + "of results"); } - QueryOptions opts = QueryOptions.create(indexConfig, start, limit + 1); + QueryOptions opts = QueryOptions.create( + indexConfig, start, limit + 1, getRequestedFields()); Predicate s = rewriter.rewrite(q, opts); if (!(s instanceof ChangeDataSource)) { q = Predicate.and(open(), q); @@ -184,6 +198,16 @@ public class QueryProcessor { return out; } + private Set getRequestedFields() { + if (requestedFields != null) { + return requestedFields; + } + ChangeIndex index = indexes.getSearchIndex(); + return index != null + ? index.getSchema().getStoredFields().keySet() + : ImmutableSet. of(); + } + boolean isDisabled() { return getPermittedLimit() <= 0; } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java index 7b1185e09f..f44d1723a8 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java @@ -25,6 +25,7 @@ import static com.google.gerrit.server.query.Predicate.and; import static com.google.gerrit.server.query.Predicate.or; import static org.junit.Assert.assertEquals; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.query.AndPredicate; import com.google.gerrit.server.query.Predicate; @@ -285,7 +286,8 @@ public class IndexRewriterTest extends GerritBaseTests { } private static QueryOptions options(int start, int limit) { - return QueryOptions.create(CONFIG, start, limit); + return QueryOptions.create(CONFIG, start, limit, + ImmutableSet. of()); } private Set status(String query) throws QueryParseException { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index dd2667f5ce..49e1796c80 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -56,6 +56,7 @@ import com.google.gerrit.server.change.ChangeTriplet; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.validators.CommitValidators; +import com.google.gerrit.server.index.ChangeField; import com.google.gerrit.server.index.IndexCollection; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; @@ -1263,6 +1264,30 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { cd.messages(); } + @Test + public void prepopulateOnlyRequestedFields() throws Exception { + assume().that(notesMigration.enabled()).isFalse(); + TestRepository repo = createProject("repo"); + Change change = insert(newChange(repo, null, null, null, null)); + + db = new DisabledReviewDb(); + requestContext.setContext(newRequestContext(userId)); + // Use QueryProcessor directly instead of API so we get ChangeDatas back. + List cds = queryProcessor + .setRequestedFields(ImmutableSet.of( + ChangeField.PATCH_SET.getName(), + ChangeField.CHANGE.getName())) + .queryChanges(queryBuilder.parse(change.getId().toString())) + .changes(); + assertThat(cds).hasSize(1); + + ChangeData cd = cds.get(0); + cd.change(); + cd.patchSets(); + + exception.expect(DisabledReviewDb.Disabled.class); + cd.currentApprovals(); + } protected ChangeInserter newChange( TestRepository repo, diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java index 7e7899b50c..53a9805bf0 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java @@ -78,6 +78,13 @@ public class LuceneQueryChangesV14Test extends LuceneQueryChangesTest { // Ignore. } + @Override + @Ignore + @Test + public void prepopulateOnlyRequestedFields() throws Exception { + // Ignore. + } + @Test public void isReviewed() throws Exception { clockStepMs = MILLISECONDS.convert(2, MINUTES);