opendev/gerrit
Code Issues Proposed changes

Adopt last updated time predicates to reuse for merged time predicates

Browse Source 
The After/Before Operators will be reused in the follow-up change to
implement the merge operators.

I had to split it into a separate change, because the existing tests
for change queries do not cover the code path that invokes the match
method of IndexedPredicates.

The match method is only required for change index queries, see
https://gerrit-review.googlesource.com/c/gerrit/+/79515.

The query tests cover the cases when the IndexedChangeQuery is used to
filter the results returned from other IndexedChangeQuery (as rewritten
by ChangeIndexRewriter, see the test).

Also, ideally, the TimeRangePredicate should look more like
IntegerRangePredicate.
I would prefer to do another round of refactoring for this to work on
any index-specific edge cases.

Change-Id: I983d157809821fa7ac05d6f89802bd1734017239
This commit is contained in:
Marija Savtchouk
2020-12-08 09:03:39 +00:00
parent 7735223a9a
commit 5be75151db
8 changed files with 162 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java
View File

@@ -29,7 +29,6 @@ import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.index.query.RegexPredicate;
import com.google.gerrit.index.query.TimestampRangePredicate;
import com.google.gerrit.server.query.change.AfterPredicate;
import java.time.Instant;
public class ElasticQueryBuilder {
@@ -130,7 +129,9 @@ public class ElasticQueryBuilder {
private <T> QueryBuilder timestampQuery(IndexPredicate<T> p) throws QueryParseException {
if (p instanceof TimestampRangePredicate) {
TimestampRangePredicate<T> r = (TimestampRangePredicate<T>) p;
if (p instanceof AfterPredicate) {
if (r.getMaxTimestamp().getTime() == Long.MAX_VALUE) {
// The time range only has the start value, search from the start to the max supported value
// Long.MAX_VALUE
return QueryBuilders.rangeQuery(r.getField().getName())
.gte(Instant.ofEpochMilli(r.getMinTimestamp().getTime()));
}

4
java/com/google/gerrit/index/query/TimestampRangePredicate.java
View File

@@ -34,6 +34,10 @@ public abstract class TimestampRangePredicate<I> extends IndexPredicate<I> {
super(def, name, value);
}
protected Timestamp getValueTimestamp(I object) {
return (Timestamp) this.getField().get(object);
}
public abstract Date getMinTimestamp();
public abstract Date getMaxTimestamp();

18
java/com/google/gerrit/server/query/change/AfterPredicate.java
View File

@@ -14,15 +14,21 @@
package com.google.gerrit.server.query.change;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.index.change.ChangeField;
import java.sql.Timestamp;
import java.util.Date;
/**
* Predicate that matches a {@link Timestamp} field from the index in a range from the passed {@code
* String} representation of the Timestamp value to the maximum supported time.
*/
public class AfterPredicate extends TimestampRangeChangePredicate {
protected final Date cut;
public AfterPredicate(String value) throws QueryParseException {
super(ChangeField.UPDATED, ChangeQueryBuilder.FIELD_BEFORE, value);
public AfterPredicate(FieldDef<ChangeData, Timestamp> def, String name, String value)
throws QueryParseException {
super(def, name, value);
cut = parse(value);
}
@@ -38,6 +44,10 @@ public class AfterPredicate extends TimestampRangeChangePredicate {
@Override
public boolean match(ChangeData cd) {
return cd.change().getLastUpdatedOn().getTime() >= cut.getTime();
Timestamp valueTimestamp = this.getValueTimestamp(cd);
if (valueTimestamp == null) {
return false;
}
return valueTimestamp.getTime() >= cut.getTime();
}
}

8
java/com/google/gerrit/server/query/change/AgePredicate.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.query.change;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import com.google.gerrit.entities.Change;
import com.google.gerrit.server.config.ConfigUtil;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.util.time.TimeUtil;
@@ -46,7 +45,10 @@ public class AgePredicate extends TimestampRangeChangePredicate {
@Override
public boolean match(ChangeData object) {
Change change = object.change();
return change != null && change.getLastUpdatedOn().getTime() <= cut;
Timestamp valueTimestamp = this.getValueTimestamp(object);
if (valueTimestamp == null) {
return false;
}
return valueTimestamp.getTime() <= cut;
}
}

18
java/com/google/gerrit/server/query/change/BeforePredicate.java
View File

@@ -14,15 +14,21 @@
package com.google.gerrit.server.query.change;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.index.change.ChangeField;
import java.sql.Timestamp;
import java.util.Date;
/**
* Predicate that matches a {@link Timestamp} field from the index in a range from the the epoch to
* the passed {@code String} representation of the Timestamp value.
*/
public class BeforePredicate extends TimestampRangeChangePredicate {
protected final Date cut;
public BeforePredicate(String value) throws QueryParseException {
super(ChangeField.UPDATED, ChangeQueryBuilder.FIELD_BEFORE, value);
public BeforePredicate(FieldDef<ChangeData, Timestamp> def, String name, String value)
throws QueryParseException {
super(def, name, value);
cut = parse(value);
}
@@ -38,6 +44,10 @@ public class BeforePredicate extends TimestampRangeChangePredicate {
@Override
public boolean match(ChangeData cd) {
return cd.change().getLastUpdatedOn().getTime() <= cut.getTime();
Timestamp valueTimestamp = this.getValueTimestamp(cd);
if (valueTimestamp == null) {
return false;
}
return valueTimestamp.getTime() <= cut.getTime();
}
}

10
java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@@ -142,7 +142,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
public static final String FIELD_ASSIGNEE = "assignee";
public static final String FIELD_AUTHOR = "author";
public static final String FIELD_EXACTAUTHOR = "exactauthor";
public static final String FIELD_BEFORE = "before";
public static final String FIELD_CHANGE = "change";
public static final String FIELD_CHANGE_ID = "change_id";
public static final String FIELD_COMMENT = "comment";
@@ -205,6 +205,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
public static final String ARG_ID_OWNER = "owner";
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
// Operators to match on the last time the change was updated. Naming for legacy reasons.
public static final String OPERATOR_BEFORE = "before";
public static final String OPERATOR_AFTER = "after";
private static final QueryBuilder.Definition<ChangeData, ChangeQueryBuilder> mydef =
new QueryBuilder.Definition<>(ChangeQueryBuilder.class);
@@ -471,7 +475,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
@Operator
public Predicate<ChangeData> before(String value) throws QueryParseException {
return new BeforePredicate(value);
return new BeforePredicate(ChangeField.UPDATED, ChangeQueryBuilder.OPERATOR_BEFORE, value);
}
@Operator
@@ -481,7 +485,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
@Operator
public Predicate<ChangeData> after(String value) throws QueryParseException {
return new AfterPredicate(value);
return new AfterPredicate(ChangeField.UPDATED, ChangeQueryBuilder.OPERATOR_AFTER, value);
}
@Operator

26
javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
View File

@@ -106,6 +106,32 @@ public class ChangeIndexRewriterTest {
assertThat(rewrite.rewrite(in, options(0, DEFAULT_MAX_QUERY_LIMIT))).isEqualTo(query(in));
}
@Test
public void threeLevelTreeWithMultipleSources() throws Exception {
Predicate<ChangeData> in = parse("-status:abandoned (foo:a OR file:b)");
Predicate<ChangeData> out = rewrite(in);
assertThat(out.getClass()).isSameInstanceAs(AndChangeSource.class);
Predicate<ChangeData> firstIndexedSubQuery = parse("-status:abandoned");
assertThat(out.getChild(0)).isEqualTo(query(firstIndexedSubQuery));
assertThat(out.getChild(1).getClass()).isSameInstanceAs(OrSource.class);
OrSource indexedSubTree = (OrSource) out.getChild(1);
Predicate<ChangeData> secondIndexedSubQuery = parse("foo:a OR file:b");
assertThat(indexedSubTree.getChildren())
.containsExactly(
query(secondIndexedSubQuery.getChild(1)), secondIndexedSubQuery.getChild(0))
.inOrder();
// Same at the assertions above, that were added for readability
assertThat(out.getChild(0)).isEqualTo(query(in.getChild(0)));
assertThat(indexedSubTree.getChildren())
.containsExactly(query(in.getChild(1).getChild(1)), in.getChild(1).getChild(0))
.inOrder();
}
@Test
public void threeLevelTreeWithSomeIndexPredicates() throws Exception {
Predicate<ChangeData> in = parse("-foo:a (file:b OR file:c)");

89
javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -85,6 +85,7 @@ import com.google.gerrit.httpd.raw.IndexPreloadingUtil;
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.query.IndexPredicate;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.lifecycle.LifecycleManager;
@@ -110,6 +111,7 @@ import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.index.change.IndexedChangeQuery;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.Sequences;
import com.google.gerrit.server.project.ProjectCache;
@@ -1573,6 +1575,10 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
assertThat(nowMs - lastUpdatedMs(change2)).isEqualTo(thirtyHoursInMs);
assertThat(TimeUtil.nowMs()).isEqualTo(nowMs);
// Change1 was last updated on 2009-09-30 21:00:00 -0000
// Change2 was last updated on 2009-10-02 03:00:00 -0000
// The endpoint is 2009-10-03 09:00:00 -0000
assertQuery("-age:1d");
assertQuery("-age:" + (30 * 60 - 1) + "m");
assertQuery("-age:2d", change2);
@@ -1580,6 +1586,15 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
assertQuery("age:3d");
assertQuery("age:2d", change1);
assertQuery("age:1d", change2, change1);
// Same test as above, but using filter code path.
assertQuery(makeIndexedPredicateFilterQuery("-age:1d"));
assertQuery(makeIndexedPredicateFilterQuery("-age:" + (30 * 60 - 1) + "m"));
assertQuery(makeIndexedPredicateFilterQuery("-age:2d"), change2);
assertQuery(makeIndexedPredicateFilterQuery("-age:3d"), change2, change1);
assertQuery(makeIndexedPredicateFilterQuery("age:3d"));
assertQuery(makeIndexedPredicateFilterQuery("age:2d"), change1);
assertQuery(makeIndexedPredicateFilterQuery("age:1d"), change2, change1);
}
@Test
@@ -1592,6 +1607,9 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
Change change2 = insert(repo, newChange(repo), null, new Timestamp(startMs + thirtyHoursInMs));
TestTimeUtil.setClockStep(0, MILLISECONDS);
// Change1 was last updated on 2009-09-30 21:00:00 -0000
// Change2 was last updated on 2009-10-02 03:00:00 -0000
for (String predicate : Lists.newArrayList("before:", "until:")) {
assertQuery(predicate + "2009-09-29");
assertQuery(predicate + "2009-09-30");
@@ -1604,6 +1622,22 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
assertQuery(predicate + "2009-10-01", change1);
assertQuery(predicate + "2009-10-03", change2, change1);
}
// Same test as above, but using filter code path.
for (String predicate : Lists.newArrayList("before:", "until:")) {
assertQuery(makeIndexedPredicateFilterQuery(predicate + "2009-09-29"));
assertQuery(makeIndexedPredicateFilterQuery(predicate + "2009-09-30"));
assertQuery(makeIndexedPredicateFilterQuery(predicate + "\"2009-09-30 16:59:00 -0400\""));
assertQuery(makeIndexedPredicateFilterQuery(predicate + "\"2009-09-30 20:59:00 -0000\""));
assertQuery(makeIndexedPredicateFilterQuery(predicate + "\"2009-09-30 20:59:00\""));
assertQuery(
makeIndexedPredicateFilterQuery(predicate + "\"2009-09-30 17:02:00 -0400\""), change1);
assertQuery(
makeIndexedPredicateFilterQuery(predicate + "\"2009-10-01 21:02:00 -0000\""), change1);
assertQuery(makeIndexedPredicateFilterQuery(predicate + "\"2009-10-01 21:02:00\""), change1);
assertQuery(makeIndexedPredicateFilterQuery(predicate + "2009-10-01"), change1);
assertQuery(makeIndexedPredicateFilterQuery(predicate + "2009-10-03"), change2, change1);
}
}
@Test
@@ -1616,6 +1650,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
Change change2 = insert(repo, newChange(repo), null, new Timestamp(startMs + thirtyHoursInMs));
TestTimeUtil.setClockStep(0, MILLISECONDS);
// Change1 was last updated on 2009-09-30 21:00:00 -0000
// Change2 was last updated on 2009-10-02 03:00:00 -0000
for (String predicate : Lists.newArrayList("after:", "since:")) {
assertQuery(predicate + "2009-10-03");
assertQuery(predicate + "\"2009-10-01 20:59:59 -0400\"", change2);
@@ -1623,6 +1659,17 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
assertQuery(predicate + "2009-10-01", change2);
assertQuery(predicate + "2009-09-30", change2, change1);
}
// Same test as above, but using filter code path.
for (String predicate : Lists.newArrayList("after:", "since:")) {
assertQuery(makeIndexedPredicateFilterQuery(predicate + "2009-10-03"));
assertQuery(
makeIndexedPredicateFilterQuery(predicate + "\"2009-10-01 20:59:59 -0400\""), change2);
assertQuery(
makeIndexedPredicateFilterQuery(predicate + "\"2009-10-01 20:59:59 -0000\""), change2);
assertQuery(makeIndexedPredicateFilterQuery(predicate + "2009-10-01"), change2);
assertQuery(makeIndexedPredicateFilterQuery(predicate + "2009-09-30"), change2, change1);
}
}
@Test
@@ -3589,6 +3636,48 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
return c.getLastUpdatedOn().getTime();
}
/**
* Generates a search query to test {@link com.google.gerrit.index.query.Matchable} implementation
* of change {@link IndexPredicate}
*
* <p>This code path requires triggering the condition, when
*
* <ol>
* <li>The query is rewritten into multiple {@link IndexedChangeQuery} by {@link
* com.google.gerrit.server.index.change.ChangeIndexRewriter#rewrite}
* <li>The changes are returned from the index by the first {@link IndexedChangeQuery}
* <li>Then constrained in {@link com.google.gerrit.index.query.AndSource#match} by applying all
* parsed predicates from the search query
* <li>Thus, the rest of {@link IndexedChangeQuery} work as filters on the index results, see
* {@link IndexedChangeQuery#match}
* </ol>
*
* The constructed query only constrains by the passed searchTerm for the operator that is being
* tested (for all changes without a reviewer):
*
* <ul>
* <li>The search term 'status:new OR status:merged OR status:abandoned' is used to return all
* changes from the search index.
* <li>The non-indexed search term 'reviewerin:"Empty Group"' is only used to make the right AND
* operand work as a filter (not a data source).
* <li>See how it is rewritten in {@link
* com.google.gerrit.server.index.change.ChangeIndexRewriterTest#threeLevelTreeWithMultipleSources}
* </ul>
*
* @param searchTerm change search term that maps to {@link IndexPredicate} and needs to be tested
* as filter
* @return a search query that allows to test the {@code searchTerm} as a filter.
*/
protected String makeIndexedPredicateFilterQuery(String searchTerm) throws Exception {
String emptyGroupName = "Empty Group";
if (gApi.groups().query(emptyGroupName).get().isEmpty()) {
createGroup(emptyGroupName, "Administrators");
}
String queryPattern =
"(status:new OR status:merged OR status:abandoned) AND (reviewerin:\"%s\" OR %s)";
return String.format(queryPattern, emptyGroupName, searchTerm);
}
private void addComment(int changeId, String message, Boolean unresolved) throws Exception {
ReviewInput input = new ReviewInput();
ReviewInput.CommentInput comment = new ReviewInput.CommentInput();