Adopt last updated time predicates to reuse for merged time predicates
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
diff --git a/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java b/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java
index d05e91c..40ac603 100644
--- a/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java
+++ b/java/com/google/gerrit/elasticsearch/ElasticQueryBuilder.java
@@ -29,7 +29,6 @@
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 @@
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()));
}
diff --git a/java/com/google/gerrit/index/query/TimestampRangePredicate.java b/java/com/google/gerrit/index/query/TimestampRangePredicate.java
index 38b2b73..42f8aa8 100644
--- a/java/com/google/gerrit/index/query/TimestampRangePredicate.java
+++ b/java/com/google/gerrit/index/query/TimestampRangePredicate.java
@@ -34,6 +34,10 @@
super(def, name, value);
}
+ protected Timestamp getValueTimestamp(I object) {
+ return (Timestamp) this.getField().get(object);
+ }
+
public abstract Date getMinTimestamp();
public abstract Date getMaxTimestamp();
diff --git a/java/com/google/gerrit/server/query/change/AfterPredicate.java b/java/com/google/gerrit/server/query/change/AfterPredicate.java
index df5a71d..8f92d9a 100644
--- a/java/com/google/gerrit/server/query/change/AfterPredicate.java
+++ b/java/com/google/gerrit/server/query/change/AfterPredicate.java
@@ -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 @@
@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();
}
}
diff --git a/java/com/google/gerrit/server/query/change/AgePredicate.java b/java/com/google/gerrit/server/query/change/AgePredicate.java
index 36eb5b7..d38789f 100644
--- a/java/com/google/gerrit/server/query/change/AgePredicate.java
+++ b/java/com/google/gerrit/server/query/change/AgePredicate.java
@@ -17,7 +17,6 @@
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 @@
@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;
}
}
diff --git a/java/com/google/gerrit/server/query/change/BeforePredicate.java b/java/com/google/gerrit/server/query/change/BeforePredicate.java
index dacabc0..6e28ce6 100644
--- a/java/com/google/gerrit/server/query/change/BeforePredicate.java
+++ b/java/com/google/gerrit/server/query/change/BeforePredicate.java
@@ -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 @@
@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();
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index ff90c3f..8f0e711 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -142,7 +142,7 @@
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 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 @@
@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 @@
@Operator
public Predicate<ChangeData> after(String value) throws QueryParseException {
- return new AfterPredicate(value);
+ return new AfterPredicate(ChangeField.UPDATED, ChangeQueryBuilder.OPERATOR_AFTER, value);
}
@Operator
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
index f5d3bf7..e5b2ffb 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
@@ -107,6 +107,32 @@
}
@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)");
Predicate<ChangeData> out = rewrite(in);
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 1de548f..5274d94 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -85,6 +85,7 @@
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.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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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();
