opendev/gerrit
Code Issues Proposed changes

Allow customizing the set of fields returned by the index

Browse Source 
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
This commit is contained in:
Dave Borowitz 2015-10-28 11:08:31 -04:00
parent 9727353fe0
commit 67758fa8fc
8 changed files with 152 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@ -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<String> FIELDS = ImmutableSet.of(
ADDED_FIELD, APPROVAL_FIELD, CHANGE_FIELD, DELETED_FIELD, ID_FIELD,
MERGEABLE_FIELD, PATCH_SET_FIELD, REVIEWEDBY_FIELD);
private static final Map<String, String> CUSTOM_CHAR_MAPPING = ImmutableMap.of(
"_", " ", ".", " ");
@ -438,10 +433,12 @@ public class LuceneChangeIndex implements ChangeIndex {
List<ChangeData> result =
Lists.newArrayListWithCapacity(docs.scoreDocs.length);
Set<String> 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<ChangeData> r = Collections.unmodifiableList(result);
@ -477,19 +474,62 @@ public class LuceneChangeIndex implements ChangeIndex {
}
}
private ChangeData toChangeData(Document doc) {
@SuppressWarnings("deprecation")
private Set<String> 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<ChangeData, ?> field,
Set<String> 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<String> 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<PatchSet> 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<Account.Id> accounts =
@ -533,8 +577,6 @@ public class LuceneChangeIndex implements ChangeIndex {
}
cd.setReviewedBy(accounts);
}
return cd;
}
private static <T> List<T> decodeProtos(Document doc, String fieldName,

2
gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java
View File

@ -50,7 +50,7 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
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;

15
gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java
View File

@ -61,6 +61,8 @@ public class Schema<T> {
}
private final ImmutableMap<String, FieldDef<T, ?>> fields;
private final ImmutableMap<String, FieldDef<T, ?>> storedFields;
private int version;
protected Schema(Iterable<FieldDef<T, ?>> fields) {
@ -71,10 +73,15 @@ public class Schema<T> {
public Schema(int version, Iterable<FieldDef<T, ?>> fields) {
this.version = version;
ImmutableMap.Builder<String, FieldDef<T, ?>> b = ImmutableMap.builder();
ImmutableMap.Builder<String, FieldDef<T, ?>> sb = ImmutableMap.builder();
for (FieldDef<T, ?> 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<T> {
return fields;
}
/**
* @return all fields in this schema where {@link FieldDef#isStored()} is
* true.
*/
public final ImmutableMap<String, FieldDef<T, ?>> getStoredFields() {
return storedFields;
}
/**
* Look up fields in this schema.
*

17
gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java
View File

@ -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<String> 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.<String> of());
}
public abstract IndexConfig config();
public abstract int start();
public abstract int limit();
public abstract ImmutableSet<String> 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());
}
}

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

@ -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<ReviewDb> db;
private final Provider<CurrentUser> 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<String> requestedFields;
@Inject
QueryProcessor(Provider<ReviewDb> db,
Provider<CurrentUser> 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<String> 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<ChangeData> 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<String> getRequestedFields() {
if (requestedFields != null) {
return requestedFields;
}
ChangeIndex index = indexes.getSearchIndex();
return index != null
? index.getSchema().getStoredFields().keySet()
: ImmutableSet.<String> of();
}
boolean isDisabled() {
return getPermittedLimit() <= 0;
}

4
gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java
View File

@ -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.<String> of());
}
private Set<Change.Status> status(String query) throws QueryParseException {

25
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@ -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> 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<ChangeData> 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> repo,

7
gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java
View File

@ -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);