Rework Index and IndexPredicate to operate over SchemaField abstraction
This change is needed to be able to gradually transition from the
FieldDef to the IndexedField + SearchSpec definition of the field in the
secondary index.
Since changing all indexed fields to the new format in a single change
is infeasible, made Schema to be able to contain both FieldDefs and
SearchSpec via interface SchemaField.
Since each FieldDef is mapped exactly to new SearchSpec, they are
mutually exclusive: either FieldDef is present in Schema, which means
the field was not replaced yet, or SearchSpec (after the replacement).
Same strategy is used for IndexPredicates.
Change-Id: Ia201bb374ed19f94c41ec392a0ac06b84a1e8886
Release-Notes: skip
diff --git a/java/com/google/gerrit/index/Schema.java b/java/com/google/gerrit/index/Schema.java
index ba990bd..a29ed53 100644
--- a/java/com/google/gerrit/index/Schema.java
+++ b/java/com/google/gerrit/index/Schema.java
@@ -14,28 +14,37 @@
package com.google.gerrit.index;
+import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
+import java.util.Set;
+import java.util.function.Function;
/** Specific version of a secondary index schema. */
public class Schema<T> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public static class Builder<T> {
- private final List<FieldDef<T, ?>> fields = new ArrayList<>();
+ private final List<SchemaField<T, ?>> searchFields = new ArrayList<>();
+ private final List<IndexedField<T, ?>> indexedFields = new ArrayList<>();
+
private Optional<Integer> version = Optional.empty();
public Builder<T> version(int version) {
@@ -44,7 +53,8 @@
}
public Builder<T> add(Schema<T> schema) {
- this.fields.addAll(schema.getFields().values());
+ this.indexedFields.addAll(schema.getIndexFields().values());
+ this.searchFields.addAll(schema.getSchemaFields().values());
if (!version.isPresent()) {
version(schema.getVersion() + 1);
}
@@ -57,32 +67,80 @@
}
public final Builder<T> add(ImmutableList<FieldDef<T, ?>> fields) {
- this.fields.addAll(fields);
+ this.searchFields.addAll(fields);
return this;
}
@SafeVarargs
public final Builder<T> remove(FieldDef<T, ?>... fields) {
- this.fields.removeAll(Arrays.asList(fields));
+ this.searchFields.removeAll(Arrays.asList(fields));
+ return this;
+ }
+
+ @SafeVarargs
+ public final Builder<T> addSearchSpecs(IndexedField<T, ?>.SearchSpec... searchSpecs) {
+ return addSearchSpecs(ImmutableList.copyOf(searchSpecs));
+ }
+
+ public Builder<T> addSearchSpecs(ImmutableList<IndexedField<T, ?>.SearchSpec> searchSpecs) {
+ for (IndexedField<T, ?>.SearchSpec searchSpec : searchSpecs) {
+ checkArgument(
+ this.indexedFields.contains(searchSpec.getField()),
+ "%s spec can only be added to the schema that contains %s field",
+ searchSpec.getName(),
+ searchSpec.getField().name());
+ }
+ this.searchFields.addAll(searchSpecs);
+ return this;
+ }
+
+ @SafeVarargs
+ public final Builder<T> addIndexedFields(IndexedField<T, ?>... fields) {
+ return addIndexedFields(ImmutableList.copyOf(fields));
+ }
+
+ public Builder<T> addIndexedFields(ImmutableList<IndexedField<T, ?>> indexedFields) {
+ this.indexedFields.addAll(indexedFields);
+ return this;
+ }
+
+ @SafeVarargs
+ public final Builder<T> remove(IndexedField<T, ?>.SearchSpec... searchSpecs) {
+ this.searchFields.removeAll(Arrays.asList(searchSpecs));
+ return this;
+ }
+
+ @SafeVarargs
+ public final Builder<T> remove(IndexedField<T, ?>... indexedFields) {
+ for (IndexedField<T, ?> field : indexedFields) {
+ ImmutableMap<String, ? extends IndexedField<T, ?>.SearchSpec> searchSpecs =
+ field.getSearchSpecs();
+ checkArgument(
+ !searchSpecs.values().stream().anyMatch(this.searchFields::contains),
+ "Field %s can be only removed from schema after all of it's searches are removed.",
+ field.name());
+ }
+ this.indexedFields.removeAll(Arrays.asList(indexedFields));
return this;
}
public Schema<T> build() {
checkState(version.isPresent());
- return new Schema<>(version.get(), ImmutableList.copyOf(fields));
+ return new Schema<T>(
+ version.get(), ImmutableList.copyOf(indexedFields), ImmutableList.copyOf(searchFields));
}
}
public static class Values<T> {
- private final FieldDef<T, ?> field;
+ private final SchemaField<T, ?> field;
private final Iterable<?> values;
- private Values(FieldDef<T, ?> field, Iterable<?> values) {
+ private Values(SchemaField<T, ?> field, Iterable<?> values) {
this.field = field;
this.values = values;
}
- public FieldDef<T, ?> getField() {
+ public SchemaField<T, ?> getField() {
return field;
}
@@ -91,28 +149,42 @@
}
}
- private static <T> FieldDef<T, ?> checkSame(FieldDef<T, ?> f1, FieldDef<T, ?> f2) {
+ private static <T> SchemaField<T, ?> checkSame(SchemaField<T, ?> f1, SchemaField<T, ?> f2) {
checkState(f1 == f2, "Mismatched %s fields: %s != %s", f1.getName(), f1, f2);
return f1;
}
- private final ImmutableMap<String, FieldDef<T, ?>> fields;
- private final ImmutableMap<String, FieldDef<T, ?>> storedFields;
+ private final ImmutableSet<String> storedFields;
+
+ private final ImmutableMap<String, SchemaField<T, ?>> schemaFields;
+ private final ImmutableMap<String, IndexedField<T, ?>> indexedFields;
private int version;
- private Schema(int version, Iterable<FieldDef<T, ?>> fields) {
+ private Schema(
+ int version,
+ ImmutableList<IndexedField<T, ?>> indexedFields,
+ ImmutableList<SchemaField> schemaFields) {
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();
+
+ this.indexedFields =
+ indexedFields.stream().collect(toImmutableMap(IndexedField::name, Function.identity()));
+ this.schemaFields =
+ schemaFields.stream().collect(toImmutableMap(SchemaField::getName, Function.identity()));
+
+ Set<String> duplicateKeys =
+ Sets.intersection(this.schemaFields.keySet(), this.indexedFields.keySet());
+ checkArgument(
+ duplicateKeys.isEmpty(),
+ "DuplicateKeys found %s, indexFields:%s, schemaFields: %s",
+ duplicateKeys,
+ this.indexedFields.keySet(),
+ this.schemaFields.keySet());
+ this.storedFields =
+ schemaFields.stream()
+ .filter(SchemaField::isStored)
+ .map(SchemaField::getName)
+ .collect(toImmutableSet());
}
public final int getVersion() {
@@ -123,17 +195,21 @@
* Get all fields in this schema.
*
* <p>This is primarily useful for iteration. Most callers should prefer one of the helper methods
- * {@link #getField(FieldDef, FieldDef...)} or {@link #hasField(FieldDef)} to looking up fields by
- * name
+ * {@link #getField(SchemaField, SchemaField...)} or {@link #hasField(SchemaField)} to looking up
+ * fields by name
*
* @return all fields in this schema indexed by name.
*/
- public final ImmutableMap<String, FieldDef<T, ?>> getFields() {
- return fields;
+ public final ImmutableMap<String, SchemaField<T, ?>> getSchemaFields() {
+ return ImmutableMap.copyOf(schemaFields);
+ }
+
+ public final ImmutableMap<String, IndexedField<T, ?>> getIndexFields() {
+ return indexedFields;
}
/** Returns all fields in this schema where {@link FieldDef#isStored()} is true. */
- public final ImmutableMap<String, FieldDef<T, ?>> getStoredFields() {
+ public final ImmutableSet<String> getStoredFields() {
return storedFields;
}
@@ -146,13 +222,14 @@
* absent if no field matches.
*/
@SafeVarargs
- public final Optional<FieldDef<T, ?>> getField(FieldDef<T, ?> first, FieldDef<T, ?>... rest) {
- FieldDef<T, ?> field = fields.get(first.getName());
+ public final Optional<SchemaField<T, ?>> getField(
+ SchemaField<T, ?> first, SchemaField<T, ?>... rest) {
+ SchemaField<T, ?> field = getSchemaField(first);
if (field != null) {
return Optional.of(checkSame(field, first));
}
- for (FieldDef<T, ?> f : rest) {
- field = fields.get(f.getName());
+ for (SchemaField<T, ?> f : rest) {
+ field = getSchemaField(first);
if (field != null) {
return Optional.of(checkSame(field, f));
}
@@ -166,8 +243,8 @@
* @param field field to look up.
* @return whether the field is present.
*/
- public final boolean hasField(FieldDef<T, ?> field) {
- FieldDef<T, ?> f = fields.get(field.getName());
+ public final boolean hasField(SchemaField<T, ?> field) {
+ SchemaField<T, ?> f = getSchemaField(field);
if (f == null) {
return false;
}
@@ -175,7 +252,19 @@
return true;
}
- private Values<T> fieldValues(T obj, FieldDef<T, ?> f, ImmutableSet<String> skipFields) {
+ public final boolean hasField(String fieldName) {
+ return this.getSchemaField(fieldName) != null;
+ }
+
+ private SchemaField<T, ?> getSchemaField(SchemaField<T, ?> field) {
+ return getSchemaField(field.getName());
+ }
+
+ public SchemaField<T, ?> getSchemaField(String fieldName) {
+ return schemaFields.get(fieldName);
+ }
+
+ private Values<T> fieldValues(T obj, SchemaField<T, ?> f, ImmutableSet<String> skipFields) {
if (skipFields.contains(f.getName())) {
return null;
}
@@ -213,10 +302,11 @@
*/
public final Iterable<Values<T>> buildFields(T obj, ImmutableSet<String> skipFields) {
try {
- return fields.values().stream()
+ return schemaFields.values().stream()
.map(f -> fieldValues(obj, f, skipFields))
.filter(Objects::nonNull)
.collect(toImmutableList());
+
} catch (StorageException e) {
return ImmutableList.of();
}
@@ -224,6 +314,9 @@
@Override
public String toString() {
- return MoreObjects.toStringHelper(this).addValue(fields.keySet()).toString();
+ return MoreObjects.toStringHelper(this)
+ .addValue(indexedFields.keySet())
+ .addValue(schemaFields.keySet())
+ .toString();
}
}
diff --git a/java/com/google/gerrit/index/SchemaUtil.java b/java/com/google/gerrit/index/SchemaUtil.java
index 7b0db07..8f47cf5 100644
--- a/java/com/google/gerrit/index/SchemaUtil.java
+++ b/java/com/google/gerrit/index/SchemaUtil.java
@@ -89,6 +89,39 @@
return new Schema.Builder<V>().add(schema).add(moreFields).build();
}
+ public static <V> Schema<V> schema(
+ int version,
+ ImmutableList<FieldDef<V, ?>> fieldDefs,
+ ImmutableList<IndexedField<V, ?>> indexedFields,
+ ImmutableList<IndexedField<V, ?>.SearchSpec> searchSpecs) {
+ return new Schema.Builder<V>()
+ .version(version)
+ .add(fieldDefs)
+ .addIndexedFields(indexedFields)
+ .addSearchSpecs(searchSpecs)
+ .build();
+ }
+
+ public static <V> Schema<V> schema(
+ Schema<V> schema,
+ ImmutableList<FieldDef<V, ?>> fieldDefs,
+ ImmutableList<IndexedField<V, ?>> indexedFields,
+ ImmutableList<IndexedField<V, ?>.SearchSpec> searchSpecs) {
+ return new Schema.Builder<V>()
+ .add(schema)
+ .add(fieldDefs)
+ .addIndexedFields(indexedFields)
+ .addSearchSpecs(searchSpecs)
+ .build();
+ }
+
+ public static <V> Schema<V> schema(
+ ImmutableList<FieldDef<V, ?>> fieldDefs,
+ ImmutableList<IndexedField<V, ?>> indexFields,
+ ImmutableList<IndexedField<V, ?>.SearchSpec> searchSpecs) {
+ return schema(/* version= */ 0, fieldDefs, indexFields, searchSpecs);
+ }
+
public static Set<String> getPersonParts(PersonIdent person) {
if (person == null) {
return ImmutableSet.of();
diff --git a/java/com/google/gerrit/index/query/FieldBundle.java b/java/com/google/gerrit/index/query/FieldBundle.java
index 6ecb6e6..60881df 100644
--- a/java/com/google/gerrit/index/query/FieldBundle.java
+++ b/java/com/google/gerrit/index/query/FieldBundle.java
@@ -19,7 +19,7 @@
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
/** FieldBundle is an abstraction that allows retrieval of raw values from different sources. */
public class FieldBundle {
@@ -34,8 +34,8 @@
/**
* Get a field's value based on the field definition.
*
- * @param fieldDef the definition of the field of which the value should be retrieved. The field
- * must be stored and contained in the result set as specified by {@link
+ * @param schemaField the definition of the field of which the value should be retrieved. The
+ * field must be stored and contained in the result set as specified by {@link
* com.google.gerrit.index.QueryOptions}.
* @param <T> Data type of the returned object based on the field definition
* @return Either a single element or an Iterable based on the field definition. An empty list is
@@ -44,16 +44,16 @@
* check is only enforced on non-repeatable fields.
*/
@SuppressWarnings("unchecked")
- public <T> T getValue(FieldDef<?, T> fieldDef) {
- checkArgument(fieldDef.isStored(), "Field must be stored");
+ public <T> T getValue(SchemaField<?, T> schemaField) {
+ checkArgument(schemaField.isStored(), "Field must be stored");
checkArgument(
- fields.containsKey(fieldDef.getName()) || fieldDef.isRepeatable(),
+ fields.containsKey(schemaField.getName()) || schemaField.isRepeatable(),
"Field %s is not in result set %s",
- fieldDef.getName(),
+ schemaField.getName(),
fields.keySet());
- Iterable<Object> result = fields.get(fieldDef.getName());
- if (fieldDef.isRepeatable()) {
+ Iterable<Object> result = fields.get(schemaField.getName());
+ if (schemaField.isRepeatable()) {
return (T) result;
}
return (T) Iterables.getOnlyElement(result);
diff --git a/java/com/google/gerrit/index/query/IndexPredicate.java b/java/com/google/gerrit/index/query/IndexPredicate.java
index b65fb96..de81c47 100644
--- a/java/com/google/gerrit/index/query/IndexPredicate.java
+++ b/java/com/google/gerrit/index/query/IndexPredicate.java
@@ -21,8 +21,8 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
import com.google.common.primitives.Longs;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.FieldType;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import java.util.Objects;
import java.util.Set;
import java.util.stream.StreamSupport;
@@ -37,19 +37,19 @@
*/
private static final Splitter FULL_TEXT_SPLITTER = Splitter.on(CharMatcher.anyOf(" ,.-:\\/_=\n"));
- private final FieldDef<I, ?> def;
+ private final SchemaField<I, ?> def;
- protected IndexPredicate(FieldDef<I, ?> def, String value) {
+ protected IndexPredicate(SchemaField<I, ?> def, String value) {
super(def.getName(), value);
this.def = def;
}
- protected IndexPredicate(FieldDef<I, ?> def, String name, String value) {
+ protected IndexPredicate(SchemaField<I, ?> def, String name, String value) {
super(name, value);
this.def = def;
}
- public FieldDef<I, ?> getField() {
+ public SchemaField<I, ?> getField() {
return def;
}
diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java
index 7684545..3c716b9 100644
--- a/java/com/google/gerrit/index/query/QueryProcessor.java
+++ b/java/com/google/gerrit/index/query/QueryProcessor.java
@@ -337,7 +337,7 @@
return requestedFields;
}
Index<?, T> index = indexes.getSearchIndex();
- return index != null ? index.getSchema().getStoredFields().keySet() : ImmutableSet.of();
+ return index != null ? index.getSchema().getStoredFields() : ImmutableSet.of();
}
/**
diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
index 36e9e52..a58f2b2 100644
--- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
+++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
@@ -24,10 +24,10 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.InternalGroup;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.Index;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.project.ProjectData;
import com.google.gerrit.index.project.ProjectIndex;
import com.google.gerrit.index.query.DataSource;
@@ -137,7 +137,7 @@
ImmutableList.Builder<FieldBundle> fieldBundles = ImmutableList.builder();
for (V result : results) {
ImmutableListMultimap.Builder<String, Object> fields = ImmutableListMultimap.builder();
- for (FieldDef<V, ?> field : getSchema().getFields().values()) {
+ for (SchemaField<V, ?> field : getSchema().getSchemaFields().values()) {
if (field.get(result) == null) {
continue;
}
@@ -213,7 +213,7 @@
@Override
protected Map<String, Object> docFor(ChangeData value) {
ImmutableMap.Builder<String, Object> doc = ImmutableMap.builder();
- for (FieldDef<ChangeData, ?> field : getSchema().getFields().values()) {
+ for (SchemaField<ChangeData, ?> field : getSchema().getSchemaFields().values()) {
if (ChangeField.MERGEABLE.getName().equals(field.getName()) && skipMergable) {
continue;
}
@@ -231,7 +231,7 @@
changeDataFactory.create(
Project.nameKey((String) doc.get(ChangeField.PROJECT.getName())),
Change.id(Integer.valueOf((String) doc.get(ChangeField.LEGACY_ID_STR.getName()))));
- for (FieldDef<ChangeData, ?> field : getSchema().getFields().values()) {
+ for (SchemaField<ChangeData, ?> field : getSchema().getSchemaFields().values()) {
field.setIfPossible(cd, new FakeStoredValue(doc.get(field.getName())));
}
return cd;
diff --git a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
index f8256bb..05431de 100644
--- a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
+++ b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
@@ -39,6 +39,7 @@
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.Schema.Values;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.DataSource;
import com.google.gerrit.index.query.FieldBundle;
import com.google.gerrit.index.query.ListResultSet;
@@ -50,7 +51,6 @@
import com.google.gerrit.server.logging.LoggingContextAwareScheduledExecutorService;
import java.io.IOException;
import java.sql.Timestamp;
-import java.util.Map;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
@@ -383,11 +383,10 @@
}
protected FieldBundle toFieldBundle(Document doc) {
- Map<String, FieldDef<V, ?>> allFields = getSchema().getFields();
ListMultimap<String, Object> rawFields = ArrayListMultimap.create();
for (IndexableField field : doc.getFields()) {
- checkArgument(allFields.containsKey(field.name()), "Unrecognized field " + field.name());
- FieldType<?> type = allFields.get(field.name()).getType();
+ checkArgument(getSchema().hasField(field.name()), "Unrecognized field " + field.name());
+ FieldType<?> type = getSchema().getSchemaField(field.name()).getType();
if (type == FieldType.EXACT || type == FieldType.FULL_TEXT || type == FieldType.PREFIX) {
rawFields.put(field.name(), field.stringValue());
} else if (type == FieldType.INTEGER || type == FieldType.INTEGER_RANGE) {
@@ -405,7 +404,7 @@
return new FieldBundle(rawFields);
}
- private static Field.Store store(FieldDef<?, ?> f) {
+ private static Field.Store store(SchemaField<?, ?> f) {
return f.isStored() ? Field.Store.YES : Field.Store.NO;
}
diff --git a/java/com/google/gerrit/lucene/ChangeSubIndex.java b/java/com/google/gerrit/lucene/ChangeSubIndex.java
index 661c0b0..ce50473 100644
--- a/java/com/google/gerrit/lucene/ChangeSubIndex.java
+++ b/java/com/google/gerrit/lucene/ChangeSubIndex.java
@@ -22,10 +22,10 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.Schema.Values;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.DataSource;
import com.google.gerrit.index.query.FieldBundle;
import com.google.gerrit.index.query.Predicate;
@@ -118,7 +118,7 @@
@Override
void add(Document doc, Values<ChangeData> values) {
// Add separate DocValues fields for those fields needed for sorting.
- FieldDef<ChangeData, ?> f = values.getField();
+ SchemaField<ChangeData, ?> f = values.getField();
if (f == ChangeField.LEGACY_ID_STR) {
String v = (String) getOnlyElement(values.getValues());
doc.add(new NumericDocValuesField(ID_STR_SORT_FIELD, Integer.valueOf(v)));
diff --git a/java/com/google/gerrit/lucene/LuceneAccountIndex.java b/java/com/google/gerrit/lucene/LuceneAccountIndex.java
index 7ed28f1..30178df 100644
--- a/java/com/google/gerrit/lucene/LuceneAccountIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneAccountIndex.java
@@ -27,6 +27,7 @@
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.Schema.Values;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.DataSource;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -117,7 +118,7 @@
@Override
void add(Document doc, Values<AccountState> values) {
// Add separate DocValues fields for those fields needed for sorting.
- FieldDef<AccountState, ?> f = values.getField();
+ SchemaField<AccountState, ?> f = values.getField();
if (f == ID) {
int v = (Integer) getOnlyElement(values.getValues());
doc.add(new NumericDocValuesField(ID_SORT_FIELD, v));
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index ff2bd53..551f675 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -38,9 +38,9 @@
import com.google.gerrit.entities.converter.ChangeProtoConverter;
import com.google.gerrit.entities.converter.ProtoConverter;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.FieldBundle;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -493,7 +493,7 @@
cd = changeDataFactory.create(Project.nameKey(project.stringValue()), id);
}
- for (FieldDef<ChangeData, ?> field : getSchema().getFields().values()) {
+ for (SchemaField<ChangeData, ?> field : getSchema().getSchemaFields().values()) {
if (fields.contains(field.getName()) && doc.get(field.getName()) != null) {
field.setIfPossible(cd, new LuceneStoredValue(doc.get(field.getName())));
}
diff --git a/java/com/google/gerrit/lucene/LuceneGroupIndex.java b/java/com/google/gerrit/lucene/LuceneGroupIndex.java
index 02d5a8b..223a73b 100644
--- a/java/com/google/gerrit/lucene/LuceneGroupIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneGroupIndex.java
@@ -21,10 +21,10 @@
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.InternalGroup;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.Schema.Values;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.DataSource;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -107,7 +107,7 @@
@Override
void add(Document doc, Values<InternalGroup> values) {
// Add separate DocValues field for the field that is needed for sorting.
- FieldDef<InternalGroup, ?> f = values.getField();
+ SchemaField<InternalGroup, ?> f = values.getField();
if (f == UUID) {
String value = (String) getOnlyElement(values.getValues());
doc.add(new SortedDocValuesField(UUID_SORT_FIELD, new BytesRef(value)));
diff --git a/java/com/google/gerrit/lucene/LuceneProjectIndex.java b/java/com/google/gerrit/lucene/LuceneProjectIndex.java
index 081e259..96b22db 100644
--- a/java/com/google/gerrit/lucene/LuceneProjectIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneProjectIndex.java
@@ -20,10 +20,10 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.Schema.Values;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.project.ProjectData;
import com.google.gerrit.index.project.ProjectIndex;
import com.google.gerrit.index.query.DataSource;
@@ -107,7 +107,7 @@
@Override
void add(Document doc, Values<ProjectData> values) {
// Add separate DocValues field for the field that is needed for sorting.
- FieldDef<ProjectData, ?> f = values.getField();
+ SchemaField<ProjectData, ?> f = values.getField();
if (f == NAME) {
String value = (String) getOnlyElement(values.getValues());
doc.add(new SortedDocValuesField(NAME_SORT_FIELD, new BytesRef(value)));
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
index d57f800..0946adc 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
@@ -21,11 +21,11 @@
import com.google.common.collect.Sets;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Change.Status;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.IndexRewriter;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.AndPredicate;
import com.google.gerrit.index.query.IndexPredicate;
import com.google.gerrit.index.query.LimitPredicate;
@@ -245,9 +245,9 @@
}
IndexPredicate<ChangeData> p = (IndexPredicate<ChangeData>) in;
- FieldDef<ChangeData, ?> def = p.getField();
+ SchemaField<ChangeData, ?> field = p.getField();
Schema<ChangeData> schema = index.getSchema();
- return schema.hasField(def);
+ return schema.hasField(field);
}
private Predicate<ChangeData> partitionChildren(
diff --git a/javatests/com/google/gerrit/index/IndexUpgradeValidator.java b/javatests/com/google/gerrit/index/IndexUpgradeValidator.java
index 85147dde..5bcc6ff 100644
--- a/javatests/com/google/gerrit/index/IndexUpgradeValidator.java
+++ b/javatests/com/google/gerrit/index/IndexUpgradeValidator.java
@@ -18,6 +18,7 @@
import static com.google.common.truth.Truth.assertWithMessage;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
import com.google.common.collect.Sets.SetView;
import org.junit.Ignore;
@@ -28,24 +29,31 @@
*/
@Ignore
public class IndexUpgradeValidator {
+
public static void assertValid(Schema<?> previousSchema, Schema<?> schema) {
+ assertValid(previousSchema.getSchemaFields(), schema.getSchemaFields(), schema.getVersion());
+ assertValid(previousSchema.getIndexFields(), schema.getIndexFields(), schema.getVersion());
+ }
+
+ private static void assertValid(
+ ImmutableMap<String, ?> previousSchemaFields,
+ ImmutableMap<String, ?> schemaFields,
+ int schemaVersion) {
SetView<String> addedFields =
- Sets.difference(schema.getFields().keySet(), previousSchema.getFields().keySet());
+ Sets.difference(schemaFields.keySet(), previousSchemaFields.keySet());
SetView<String> removedFields =
- Sets.difference(previousSchema.getFields().keySet(), schema.getFields().keySet());
+ Sets.difference(previousSchemaFields.keySet(), schemaFields.keySet());
SetView<String> keptFields =
- Sets.intersection(previousSchema.getFields().keySet(), schema.getFields().keySet());
+ Sets.intersection(previousSchemaFields.keySet(), schemaFields.keySet());
assertWithMessage(
"Schema upgrade to version "
- + schema.getVersion()
+ + schemaVersion
+ " may either add or remove fields, but not both")
.that(addedFields.isEmpty() || removedFields.isEmpty())
.isTrue();
ImmutableList<String> modifiedFields =
keptFields.stream()
- .filter(
- fieldName ->
- previousSchema.getFields().get(fieldName) != schema.getFields().get(fieldName))
+ .filter(fieldName -> previousSchemaFields.get(fieldName) != schemaFields.get(fieldName))
.collect(toImmutableList());
assertWithMessage("Fields may not be modified (create a new field instead)")
.that(modifiedFields)
diff --git a/javatests/com/google/gerrit/index/IndexUpgradeValidatorTest.java b/javatests/com/google/gerrit/index/IndexUpgradeValidatorTest.java
index f335a9c..c2caff8 100644
--- a/javatests/com/google/gerrit/index/IndexUpgradeValidatorTest.java
+++ b/javatests/com/google/gerrit/index/IndexUpgradeValidatorTest.java
@@ -29,6 +29,9 @@
/** Tests {@link IndexUpgradeValidator}. */
@RunWith(JUnit4.class)
public class IndexUpgradeValidatorTest {
+
+ // TODO(mariasavtchouk): adopt this test to verity IndexedFields follow the same constraints as
+ // SchemaFields.
@Test
public void valid() {
IndexUpgradeValidator.assertValid(schema(1, ChangeField.ID), schema(2, ChangeField.ID));
diff --git a/javatests/com/google/gerrit/index/SchemaUtilTest.java b/javatests/com/google/gerrit/index/SchemaUtilTest.java
index ed988ff..1eaf0fe 100644
--- a/javatests/com/google/gerrit/index/SchemaUtilTest.java
+++ b/javatests/com/google/gerrit/index/SchemaUtilTest.java
@@ -15,6 +15,7 @@
package com.google.gerrit.index;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.index.FieldDef.exact;
import static com.google.gerrit.index.SchemaUtil.getNameParts;
import static com.google.gerrit.index.SchemaUtil.getPersonParts;
import static com.google.gerrit.index.SchemaUtil.schema;
@@ -25,7 +26,24 @@
import org.junit.Test;
public class SchemaUtilTest {
+
+ private static final FieldDef<String, String> TEST_DEF =
+ exact("test_id").stored().build(id -> id);
+
+ private static final FieldDef<String, String> OTHER_TEST_DEF =
+ exact("other_test_id").stored().build(id -> id);
+
+ private static final IndexedField<String, String> TEST_FIELD =
+ IndexedField.<String>stringBuilder("TestId").build(a -> a);
+
+ private static final IndexedField<String, String> TEST_FIELD_DUPLICATE_NAME =
+ IndexedField.<String>stringBuilder(TEST_DEF.getName()).build(a -> a);
+
+ private static final IndexedField.SearchSpec TEST_FIELD_SPEC =
+ TEST_FIELD.exact(TEST_DEF.getName());
+
static class TestSchemas {
+
static final Schema<String> V1 = schema(/* version= */ 1);
static final Schema<String> V2 = schema(/* version= */ 2);
static Schema<String> V3 = schema(V2); // Not final, ignored.
@@ -85,4 +103,169 @@
assertThat(getNameParts("foO-bAr_Baz a.b@c/d"))
.containsExactly("foo", "bar", "baz", "a", "b", "c", "d");
}
+
+ @Test
+ public void canAddFieldSpecAndFieldDef() {
+ Schema<String> schema0 =
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .add(OTHER_TEST_DEF)
+ .build();
+
+ assertThat(schema0.hasField(TEST_FIELD_SPEC)).isTrue();
+ assertThat(schema0.hasField(OTHER_TEST_DEF)).isTrue();
+ assertThat(schema0.getIndexFields().values()).contains(TEST_FIELD);
+ }
+
+ @Test
+ public void canRemoveIndexedField() {
+ Schema<String> schema0 =
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .build();
+
+ Schema<String> schema1 =
+ new Schema.Builder<String>()
+ .add(schema0)
+ .remove(TEST_FIELD_SPEC)
+ .remove(TEST_FIELD)
+ .build();
+ assertThat(schema1.hasField(TEST_FIELD_SPEC)).isFalse();
+ assertThat(schema1.getIndexFields().values()).doesNotContain(TEST_FIELD);
+ }
+
+ @Test
+ public void canRemoveSearchSpec() {
+ Schema<String> schema0 =
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .build();
+
+ Schema<String> schema1 =
+ new Schema.Builder<String>().add(schema0).remove(TEST_FIELD_SPEC).build();
+ assertThat(schema1.hasField(TEST_FIELD_SPEC)).isFalse();
+ assertThat(schema1.getIndexFields().values()).contains(TEST_FIELD);
+ }
+
+ @Test
+ public void canRemoveFieldDef() {
+ Schema<String> schema0 =
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .add(OTHER_TEST_DEF)
+ .build();
+
+ Schema<String> schema1 =
+ new Schema.Builder<String>().add(schema0).remove(OTHER_TEST_DEF).build();
+ assertThat(schema1.hasField(TEST_FIELD_SPEC)).isTrue();
+ assertThat(schema1.hasField(OTHER_TEST_DEF)).isFalse();
+ assertThat(schema1.getIndexFields().values()).contains(TEST_FIELD);
+ }
+
+ @Test
+ public void addSearchWithoutStoredField_disallowed() {
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new Schema.Builder<String>().version(0).addSearchSpecs(TEST_FIELD_SPEC).build());
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo("test_id spec can only be added to the schema that contains TestId field");
+ }
+
+ @Test
+ public void addDuplicateIndexField_disallowed() {
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addIndexedFields(TEST_FIELD)
+ .build());
+ assertThat(thrown).hasMessageThat().contains("Multiple entries with same key: TestId");
+ }
+
+ @Test
+ public void addDuplicateSearchSpec_disallowed() {
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .build());
+ assertThat(thrown).hasMessageThat().contains("Multiple entries with same key: test_id");
+ }
+
+ @Test
+ public void addFieldDefWithDuplicateSearchName_disallowed() {
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .add(TEST_DEF)
+ .build());
+ assertThat(thrown).hasMessageThat().contains("Multiple entries with same key: test_id");
+ }
+
+ @Test
+ public void addFieldDefWithDuplicateFieldName_disallowed() {
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD_DUPLICATE_NAME)
+ .add(TEST_DEF)
+ .build());
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo("DuplicateKeys found [test_id], indexFields:[test_id], schemaFields: [test_id]");
+ }
+
+ @Test
+ public void removeFieldWithExistingSearchSpec_disallowed() {
+ Schema<String> schema0 =
+ new Schema.Builder<String>()
+ .version(0)
+ .addIndexedFields(TEST_FIELD)
+ .addSearchSpecs(TEST_FIELD_SPEC)
+ .build();
+
+ IllegalArgumentException thrown =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> new Schema.Builder<String>().add(schema0).remove(TEST_FIELD).build());
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo(
+ "Field TestId can be only removed from schema after all of it's searches are removed.");
+
+ Schema<String> schema1 =
+ new Schema.Builder<String>()
+ .add(schema0)
+ .remove(TEST_FIELD_SPEC)
+ .remove(TEST_FIELD)
+ .build();
+ assertThat(schema1.hasField(TEST_FIELD_SPEC)).isFalse();
+ assertThat(schema1.getIndexFields().values()).doesNotContain(TEST_FIELD);
+ }
}
diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
index 9a2499a..986a0bd 100644
--- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
+++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
@@ -624,8 +624,7 @@
.getSearchIndex()
.getRaw(
Account.id(userInfo._accountId),
- QueryOptions.create(
- IndexConfig.createDefault(), 0, 1, schema.getStoredFields().keySet()));
+ QueryOptions.create(IndexConfig.createDefault(), 0, 1, schema.getStoredFields()));
assertThat(rawFields).isPresent();
if (schema.hasField(AccountField.ID)) {
diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
index 568b5a0..094c52e 100644
--- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
+++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
@@ -377,7 +377,7 @@
IndexConfig.createDefault(),
0,
10,
- indexes.getSearchIndex().getSchema().getStoredFields().keySet()));
+ indexes.getSearchIndex().getSchema().getStoredFields()));
assertThat(rawFields).isPresent();
assertThat(rawFields.get().getValue(GroupField.UUID)).isEqualTo(uuid.get());
