opendev/gerrit
Code Issues Proposed changes

Fix sorting of results from Lucene for account, group and project index

Browse Source 
The fields that are used for sorting must be added to the document. This
means the documents in the index must be all recomputed, hence we need
new index schema versions for the affected indexes.

The sorting that is done by Lucene for the account index must match the
sorting of accounts that is already done in QueryAccounts before
returning query results to clients. QueryAccounts sorts results by
fullname, preferred email and account ID. If Lucene would do a different
sorting limited queries may return wrong results. E.g. if there are 3
accounts, Foo (ID=1), Bar (ID=2) and Baz (ID=3), which are all matched
by a query for which the results are limited to 2, callers of
QueryAccounts expect Bar and Baz to be returned since QueryAccounts
promises sorting by fullname. If now Lucene would sort by account ID,
Lucene would find Foo and Bar, Baz would be skipped since it's over the
limit. These results are then handed over to QueryAccounts which does
sorting by name so that Bar and Foo are returned to the client. This
would be wrong since the caller expects Bar and Baz. It's a bit unclear
how this worked when Lucene was not doing proper sorting at all, maybe
it was just luck that the withLimit test succeeded before. As Lucene
does the sorting properly now, the sorting in QueryAccounts could be
removed, but since Lucene is not the only index implementation and other
index implementation may still have sorting issues we keep that sorting
in QueryAccounts for now.

Bug: Issue 10210
Change-Id: Ic59e4d330fe8c7198023ddbc2fa946cf5db80b63
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-12-24 10:37:20 +01:00
committed by David Pursehouse
parent d8e89a879f
commit 714f7d3c3f
9 changed files with 172 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
java/com/google/gerrit/index/project/ProjectSchemaDefinitions.java
View File

@@ -34,7 +34,10 @@ public class ProjectSchemaDefinitions extends SchemaDefinitions<ProjectData> {
static final Schema<ProjectData> V2 = schema(V1, ProjectField.STATE, ProjectField.REF_STATE);
// Bump Lucene version requires reindexing
static final Schema<ProjectData> V3 = schema(V2);
@Deprecated static final Schema<ProjectData> V3 = schema(V2);
// Lucene index was changed to add an additional field for sorting.
static final Schema<ProjectData> V4 = schema(V3);
public static final ProjectSchemaDefinitions INSTANCE = new ProjectSchemaDefinitions();

38
java/com/google/gerrit/lucene/LuceneAccountIndex.java
View File

@@ -14,10 +14,15 @@
package com.google.gerrit.lucene;
import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.gerrit.server.index.account.AccountField.FULL_NAME;
import static com.google.gerrit.server.index.account.AccountField.ID;
import static com.google.gerrit.server.index.account.AccountField.PREFERRED_EMAIL_EXACT;
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.query.DataSource;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -35,6 +40,8 @@ import java.io.IOException;
import java.nio.file.Path;
import java.util.concurrent.ExecutionException;
import org.apache.lucene.document.Document;
import org.apache.lucene.document.NumericDocValuesField;
import org.apache.lucene.document.SortedDocValuesField;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.Sort;
@@ -42,12 +49,15 @@ import org.apache.lucene.search.SortField;
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.FSDirectory;
import org.apache.lucene.store.RAMDirectory;
import org.apache.lucene.util.BytesRef;
import org.eclipse.jgit.lib.Config;
public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountState>
implements AccountIndex {
private static final String ACCOUNTS = "accounts";
private static final String FULL_NAME_SORT_FIELD = sortFieldName(FULL_NAME);
private static final String EMAIL_SORT_FIELD = sortFieldName(PREFERRED_EMAIL_EXACT);
private static final String ID_SORT_FIELD = sortFieldName(ID);
private static Term idTerm(AccountState as) {
@@ -92,6 +102,23 @@ public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountS
queryBuilder = new QueryBuilder<>(schema, indexWriterConfig.getAnalyzer());
}
@Override
void add(Document doc, Values<AccountState> values) {
// Add separate DocValues fields for those fields needed for sorting.
FieldDef<AccountState, ?> f = values.getField();
if (f == ID) {
int v = (Integer) getOnlyElement(values.getValues());
doc.add(new NumericDocValuesField(ID_SORT_FIELD, v));
} else if (f == FULL_NAME) {
String value = (String) getOnlyElement(values.getValues());
doc.add(new SortedDocValuesField(FULL_NAME_SORT_FIELD, new BytesRef(value)));
} else if (f == PREFERRED_EMAIL_EXACT) {
String value = (String) getOnlyElement(values.getValues());
doc.add(new SortedDocValuesField(EMAIL_SORT_FIELD, new BytesRef(value)));
}
super.add(doc, values);
}
@Override
public void replace(AccountState as) throws IOException {
try {
@@ -114,9 +141,14 @@ public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountS
public DataSource<AccountState> getSource(Predicate<AccountState> p, QueryOptions opts)
throws QueryParseException {
return new LuceneQuerySource(
opts.filterFields(IndexUtils::accountFields),
queryBuilder.toQuery(p),
new Sort(new SortField(ID_SORT_FIELD, SortField.Type.LONG, true)));
opts.filterFields(IndexUtils::accountFields), queryBuilder.toQuery(p), getSort());
}
private Sort getSort() {
return new Sort(
new SortField(FULL_NAME_SORT_FIELD, SortField.Type.STRING, false),
new SortField(EMAIL_SORT_FIELD, SortField.Type.STRING, false),
new SortField(ID_SORT_FIELD, SortField.Type.LONG, false));
}
@Override

16
java/com/google/gerrit/lucene/LuceneGroupIndex.java
View File

@@ -14,10 +14,13 @@
package com.google.gerrit.lucene;
import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.gerrit.server.index.group.GroupField.UUID;
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.query.DataSource;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
@@ -35,6 +38,7 @@ import java.io.IOException;
import java.nio.file.Path;
import java.util.concurrent.ExecutionException;
import org.apache.lucene.document.Document;
import org.apache.lucene.document.SortedDocValuesField;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.Sort;
@@ -42,6 +46,7 @@ import org.apache.lucene.search.SortField;
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.FSDirectory;
import org.apache.lucene.store.RAMDirectory;
import org.apache.lucene.util.BytesRef;
import org.eclipse.jgit.lib.Config;
public class LuceneGroupIndex extends AbstractLuceneIndex<AccountGroup.UUID, InternalGroup>
@@ -93,6 +98,17 @@ public class LuceneGroupIndex extends AbstractLuceneIndex<AccountGroup.UUID, Int
queryBuilder = new QueryBuilder<>(schema, indexWriterConfig.getAnalyzer());
}
@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();
if (f == UUID) {
String value = (String) getOnlyElement(values.getValues());
doc.add(new SortedDocValuesField(UUID_SORT_FIELD, new BytesRef(value)));
}
super.add(doc, values);
}
@Override
public void replace(InternalGroup group) throws IOException {
try {

16
java/com/google/gerrit/lucene/LuceneProjectIndex.java
View File

@@ -14,10 +14,13 @@
package com.google.gerrit.lucene;
import static com.google.common.collect.Iterables.getOnlyElement;
import static com.google.gerrit.index.project.ProjectField.NAME;
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.project.ProjectData;
import com.google.gerrit.index.project.ProjectIndex;
import com.google.gerrit.index.query.DataSource;
@@ -35,6 +38,7 @@ import java.io.IOException;
import java.nio.file.Path;
import java.util.concurrent.ExecutionException;
import org.apache.lucene.document.Document;
import org.apache.lucene.document.SortedDocValuesField;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.SearcherFactory;
import org.apache.lucene.search.Sort;
@@ -42,6 +46,7 @@ import org.apache.lucene.search.SortField;
import org.apache.lucene.store.Directory;
import org.apache.lucene.store.FSDirectory;
import org.apache.lucene.store.RAMDirectory;
import org.apache.lucene.util.BytesRef;
import org.eclipse.jgit.lib.Config;
public class LuceneProjectIndex extends AbstractLuceneIndex<Project.NameKey, ProjectData>
@@ -92,6 +97,17 @@ public class LuceneProjectIndex extends AbstractLuceneIndex<Project.NameKey, Pro
queryBuilder = new QueryBuilder<>(schema, indexWriterConfig.getAnalyzer());
}
@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();
if (f == NAME) {
String value = (String) getOnlyElement(values.getValues());
doc.add(new SortedDocValuesField(NAME_SORT_FIELD, new BytesRef(value)));
}
super.add(doc, values);
}
@Override
public void replace(ProjectData projectState) throws IOException {
try {

5
java/com/google/gerrit/server/index/account/AccountSchemaDefinitions.java
View File

@@ -46,7 +46,10 @@ public class AccountSchemaDefinitions extends SchemaDefinitions<AccountState> {
static final Schema<AccountState> V8 = schema(V7, AccountField.NAME_PART_NO_SECONDARY_EMAIL);
// Bump Lucene version requires reindexing
static final Schema<AccountState> V9 = schema(V8);
@Deprecated static final Schema<AccountState> V9 = schema(V8);
// Lucene index was changed to add additional fields for sorting.
static final Schema<AccountState> V10 = schema(V9);
public static final String NAME = "accounts";
public static final AccountSchemaDefinitions INSTANCE = new AccountSchemaDefinitions();

5
java/com/google/gerrit/server/index/group/GroupSchemaDefinitions.java
View File

@@ -40,7 +40,10 @@ public class GroupSchemaDefinitions extends SchemaDefinitions<InternalGroup> {
@Deprecated static final Schema<InternalGroup> V5 = schema(V4, GroupField.REF_STATE);
// Bump Lucene version requires reindexing
static final Schema<InternalGroup> V6 = schema(V5);
@Deprecated static final Schema<InternalGroup> V6 = schema(V5);
// Lucene index was changed to add an additional field for sorting.
static final Schema<InternalGroup> V7 = schema(V6);
public static final GroupSchemaDefinitions INSTANCE = new GroupSchemaDefinitions();

61
javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
View File

@@ -453,6 +453,67 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
assertQuery(newQuery(domain).withStart(1), result.subList(1, 3));
}
@Test
public void sortedByFullname() throws Exception {
String appendix = name("name");
// Use an account creation order that ensures that sorting by fullname differs from sorting by
// account ID.
AccountInfo userFoo = newAccountWithFullName("user1", "foo-" + appendix);
AccountInfo userBar = newAccountWithFullName("user2", "bar-" + appendix);
AccountInfo userBaz = newAccountWithFullName("user3", "baz-" + appendix);
assertThat(userFoo._accountId).isLessThan(userBar._accountId);
assertThat(userBar._accountId).isLessThan(userBaz._accountId);
String query = "name:" + userFoo.name + " OR name:" + userBar.name + " OR name:" + userBaz.name;
// Must request details to populate fullname in the results. If fullname is not set sorting by
// fullname is not possible.
assertQuery(newQuery(query).withOption(ListAccountsOption.DETAILS), userBar, userBaz, userFoo);
}
@Test
public void sortedByPreferredEmail() throws Exception {
String appendix = name("name");
// Use an account creation order that ensures that sorting by preferred email differs from
// sorting by account ID. Use the same fullname for all accounts so that sorting must be done by
// preferred email.
AccountInfo userFoo3 =
newAccount("user3", "foo-" + appendix, "foo3-" + appendix + "@test.com", true);
AccountInfo userFoo1 =
newAccount("user1", "foo-" + appendix, "foo1-" + appendix + "@test.com", true);
AccountInfo userFoo2 =
newAccount("user2", "foo-" + appendix, "foo2-" + appendix + "@test.com", true);
assertThat(userFoo3._accountId).isLessThan(userFoo1._accountId);
assertThat(userFoo1._accountId).isLessThan(userFoo2._accountId);
String query =
"name:" + userFoo1.name + " OR name:" + userFoo2.name + " OR name:" + userFoo3.name;
// Must request details to populate fullname and preferred email in the results. If fullname and
// preferred email are not set sorting by fullname and preferred email is not possible. Since
// all 3 accounts have the same fullname we expect sorting by preferred email.
assertQuery(
newQuery(query).withOption(ListAccountsOption.DETAILS), userFoo1, userFoo2, userFoo3);
}
@Test
public void sortedById() throws Exception {
String appendix = name("name");
// Each new account gets a higher account ID. Create the accounts in an order that sorting by
// fullname differs from sorting by accout ID.
AccountInfo userFoo = newAccountWithFullName("user1", "foo-" + appendix);
AccountInfo userBar = newAccountWithFullName("user2", "bar-" + appendix);
AccountInfo userBaz = newAccountWithFullName("user3", "baz-" + appendix);
assertThat(userFoo._accountId).isLessThan(userBar._accountId);
assertThat(userBar._accountId).isLessThan(userBaz._accountId);
String query = "name:" + userFoo.name + " OR name:" + userBar.name + " OR name:" + userBaz.name;
// Normally sorting is done by fullname and preferred email, but if no details are requested
// fullname and preferred email are not set and then sorting is done by account ID.
assertQuery(newQuery(query), userFoo, userBar, userBaz);
}
@Test
public void withDetails() throws Exception {
AccountInfo user1 = newAccount("myuser", "My User", "my.user@example.com", true);

18
javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
View File

@@ -314,6 +314,17 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
assertQuery(newQuery(query).withStart(1), result.subList(1, 3));
}
@Test
public void sortedByUuid() throws Exception {
GroupInfo group1 = createGroup(name("group1"));
GroupInfo group2 = createGroup(name("group2"));
GroupInfo group3 = createGroup(name("group3"));
String query = "uuid:" + group1.id + " OR uuid:" + group2.id + " OR uuid:" + group3.id;
// assertQuery sorts the expected groups by UUID
assertQuery(newQuery(query), group1, group2, group3);
}
@Test
public void asAnonymous() throws Exception {
GroupInfo group = createGroup(name("group"));
@@ -448,7 +459,10 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
throws Exception {
List<GroupInfo> result = query.get();
Iterable<String> uuids = uuids(result);
assertThat(uuids).named(format(query, result, groups)).containsExactlyElementsIn(uuids(groups));
assertThat(uuids)
.named(format(query, result, groups))
.containsExactlyElementsIn(uuids(groups))
.inOrder();
return result;
}
@@ -513,7 +527,7 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
}
protected static Iterable<String> uuids(List<GroupInfo> groups) {
return groups.stream().map(g -> g.id).collect(toList());
return groups.stream().map(g -> g.id).sorted().collect(toList());
}
protected String name(String name) {

20
javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java
View File

@@ -164,9 +164,9 @@ public abstract class AbstractQueryProjectsTest extends GerritServerTests {
String namePart = getSanitizedMethodName();
namePart = CharMatcher.is('_').removeFrom(namePart);
ProjectInfo project1 = createProject(name("project-" + namePart));
ProjectInfo project2 = createProject(name("project-" + namePart + "-2"));
ProjectInfo project3 = createProject(name("project-" + namePart + "3"));
ProjectInfo project1 = createProject(name("project1-" + namePart));
ProjectInfo project2 = createProject(name("project2-" + namePart + "-foo"));
ProjectInfo project3 = createProject(name("project3-" + namePart + "foo"));
assertQuery("inname:" + namePart, project1, project2, project3);
assertQuery("inname:" + namePart.toUpperCase(Locale.US), project1, project2, project3);
@@ -252,6 +252,17 @@ public abstract class AbstractQueryProjectsTest extends GerritServerTests {
assertQuery(newQuery(query).withStart(1), result.subList(1, 3));
}
@Test
public void sortedByName() throws Exception {
ProjectInfo projectFoo = createProject("foo-" + name("project1"));
ProjectInfo projectBar = createProject("bar-" + name("project2"));
ProjectInfo projectBaz = createProject("baz-" + name("project3"));
String query =
"name:" + projectFoo.name + " OR name:" + projectBar.name + " OR name:" + projectBaz.name;
assertQuery(newQuery(query), projectBar, projectBaz, projectFoo);
}
@Test
public void asAnonymous() throws Exception {
ProjectInfo project = createProjectRestrictedToRegisteredUsers(name("project"));
@@ -335,7 +346,8 @@ public abstract class AbstractQueryProjectsTest extends GerritServerTests {
Iterable<String> names = names(result);
assertThat(names)
.named(format(query, result, projects))
.containsExactlyElementsIn(names(projects));
.containsExactlyElementsIn(names(projects))
.inOrder();
return result;
}