Skip to content

Commit 18663b0

Browse files
committed
Don't index ranges including NOW in percolator (#52748)
Currently, date ranges queries using NOW-based date math are rewritten to MatchAllDocs queries when being preprocessed for the percolator. However, since we added the verification step, this can result in incorrect matches when percolator queries are run without scores. This commit changes things to instead wrap date queries that use NOW with a new DateRangeIncludingNowQuery. This is a simple wrapper query that returns its delegate at rewrite time, but it can be detected by the percolator QueryAnalyzer and be dealt with accordingly. This also allows us to remove a method on QueryRewriteContext, and push all logic relating to NOW-based ranges into the DateFieldMapper. Fixes #52617
1 parent 1a14ae4 commit 18663b0

File tree

10 files changed

+147
-60
lines changed

10 files changed

+147
-60
lines changed

modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java

+1-8
Original file line numberDiff line numberDiff line change
@@ -410,14 +410,7 @@ public void parse(ParseContext context) throws IOException {
410410
Version indexVersion = context.mapperService().getIndexSettings().getIndexVersionCreated();
411411
createQueryBuilderField(indexVersion, queryBuilderField, queryBuilder, context);
412412

413-
QueryBuilder queryBuilderForProcessing = queryBuilder.rewrite(new QueryShardContext(queryShardContext) {
414-
415-
@Override
416-
public boolean convertNowRangeToMatchAll() {
417-
return true;
418-
}
419-
});
420-
Query query = toQuery(queryShardContext, isMapUnmappedFieldAsText(), queryBuilderForProcessing);
413+
Query query = toQuery(queryShardContext, isMapUnmappedFieldAsText(), queryBuilder);
421414
processQuery(query, context);
422415
}
423416

modules/percolator/src/main/java/org/elasticsearch/percolator/QueryAnalyzer.java

+5
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.apache.lucene.util.NumericUtils;
4141
import org.elasticsearch.Version;
4242
import org.elasticsearch.common.lucene.search.function.FunctionScoreQuery;
43+
import org.elasticsearch.index.query.DateRangeIncludingNowQuery;
4344

4445
import java.util.ArrayList;
4546
import java.util.Arrays;
@@ -155,6 +156,10 @@ Result getResult() {
155156

156157
@Override
157158
public QueryVisitor getSubVisitor(Occur occur, Query parent) {
159+
if (parent instanceof DateRangeIncludingNowQuery) {
160+
terms.add(Result.UNKNOWN);
161+
return QueryVisitor.EMPTY_VISITOR;
162+
}
158163
this.verified = isVerified(parent);
159164
if (occur == Occur.MUST || occur == Occur.FILTER) {
160165
ResultBuilder builder = new ResultBuilder(version, true);

modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorFieldMapperTests.java

+11
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,17 @@ public void testPercolatorFieldMapper() throws Exception {
510510
assertThat(doc.rootDoc().getFields(fieldType.queryBuilderField.name()).length, equalTo(1));
511511
qbSource = doc.rootDoc().getFields(fieldType.queryBuilderField.name())[0].binaryValue();
512512
assertQueryBuilder(qbSource, queryBuilder);
513+
514+
queryBuilder = rangeQuery("date_field").from("now");
515+
doc = mapperService.documentMapper().parse(new SourceToParse("test", "doc", "1", BytesReference.bytes(XContentFactory
516+
.jsonBuilder()
517+
.startObject()
518+
.field(fieldName, queryBuilder)
519+
.endObject()),
520+
XContentType.JSON));
521+
assertThat(doc.rootDoc().getFields(fieldType.extractionResultField.name()).length, equalTo(1));
522+
assertThat(doc.rootDoc().getFields(fieldType.extractionResultField.name())[0].stringValue(),
523+
equalTo(EXTRACTION_FAILED));
513524
}
514525

515526
public void testStoringQueries() throws Exception {

modules/percolator/src/test/java/org/elasticsearch/percolator/PercolatorQuerySearchIT.java

+33
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import static org.elasticsearch.common.xcontent.XContentFactory.yamlBuilder;
5050
import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
5151
import static org.elasticsearch.index.query.QueryBuilders.commonTermsQuery;
52+
import static org.elasticsearch.index.query.QueryBuilders.constantScoreQuery;
5253
import static org.elasticsearch.index.query.QueryBuilders.geoBoundingBoxQuery;
5354
import static org.elasticsearch.index.query.QueryBuilders.geoDistanceQuery;
5455
import static org.elasticsearch.index.query.QueryBuilders.geoPolygonQuery;
@@ -941,4 +942,36 @@ public void testDisallowExpensiveQueries() throws IOException {
941942
assertAcked(client().admin().cluster().updateSettings(updateSettingsRequest).actionGet());
942943
}
943944
}
945+
946+
public void testWrappedWithConstantScore() throws Exception {
947+
948+
assertAcked(client().admin().indices().prepareCreate("test")
949+
.addMapping("_doc", "d", "type=date", "q", "type=percolator")
950+
);
951+
952+
client().prepareIndex("test", "_doc").setId("1")
953+
.setSource(jsonBuilder().startObject().field("q",
954+
boolQuery().must(rangeQuery("d").gt("now"))
955+
).endObject())
956+
.execute().actionGet();
957+
958+
client().prepareIndex("test", "_doc").setId("2")
959+
.setSource(jsonBuilder().startObject().field("q",
960+
boolQuery().must(rangeQuery("d").lt("now"))
961+
).endObject())
962+
.execute().actionGet();
963+
964+
client().admin().indices().prepareRefresh().get();
965+
966+
SearchResponse response = client().prepareSearch("test").setQuery(new PercolateQueryBuilder("q",
967+
BytesReference.bytes(jsonBuilder().startObject().field("d", "2020-02-01T15:00:00.000+11:00").endObject()),
968+
XContentType.JSON)).get();
969+
assertEquals(1, response.getHits().getTotalHits().value);
970+
971+
response = client().prepareSearch("test").setQuery(constantScoreQuery(new PercolateQueryBuilder("q",
972+
BytesReference.bytes(jsonBuilder().startObject().field("d", "2020-02-01T15:00:00.000+11:00").endObject()),
973+
XContentType.JSON))).get();
974+
assertEquals(1, response.getHits().getTotalHits().value);
975+
976+
}
944977
}

server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java

+11-2
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import org.elasticsearch.index.fielddata.IndexFieldData;
5050
import org.elasticsearch.index.fielddata.IndexNumericFieldData.NumericType;
5151
import org.elasticsearch.index.fielddata.plain.DocValuesIndexFieldData;
52+
import org.elasticsearch.index.query.DateRangeIncludingNowQuery;
5253
import org.elasticsearch.index.query.QueryRewriteContext;
5354
import org.elasticsearch.index.query.QueryShardContext;
5455
import org.elasticsearch.search.DocValueFormat;
@@ -386,19 +387,24 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower
386387
DateMathParser parser = forcedDateParser == null
387388
? dateMathParser
388389
: forcedDateParser;
390+
boolean[] nowUsed = new boolean[1];
391+
LongSupplier nowSupplier = () -> {
392+
nowUsed[0] = true;
393+
return context.nowInMillis();
394+
};
389395
long l, u;
390396
if (lowerTerm == null) {
391397
l = Long.MIN_VALUE;
392398
} else {
393-
l = parseToLong(lowerTerm, !includeLower, timeZone, parser, context::nowInMillis);
399+
l = parseToLong(lowerTerm, !includeLower, timeZone, parser, nowSupplier);
394400
if (includeLower == false) {
395401
++l;
396402
}
397403
}
398404
if (upperTerm == null) {
399405
u = Long.MAX_VALUE;
400406
} else {
401-
u = parseToLong(upperTerm, includeUpper, timeZone, parser, context::nowInMillis);
407+
u = parseToLong(upperTerm, includeUpper, timeZone, parser, nowSupplier);
402408
if (includeUpper == false) {
403409
--u;
404410
}
@@ -408,6 +414,9 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower
408414
Query dvQuery = SortedNumericDocValuesField.newSlowRangeQuery(name(), l, u);
409415
query = new IndexOrDocValuesQuery(query, dvQuery);
410416
}
417+
if (nowUsed[0]) {
418+
query = new DateRangeIncludingNowQuery(query);
419+
}
411420
return query;
412421
}
413422

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/*
2+
* Licensed to Elasticsearch under one or more contributor
3+
* license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright
5+
* ownership. Elasticsearch licenses this file to you under
6+
* the Apache License, Version 2.0 (the "License"); you may
7+
* not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.elasticsearch.index.query;
21+
22+
import org.apache.lucene.index.IndexReader;
23+
import org.apache.lucene.search.BooleanClause;
24+
import org.apache.lucene.search.Query;
25+
import org.apache.lucene.search.QueryVisitor;
26+
27+
import java.io.IOException;
28+
import java.util.Objects;
29+
30+
/**
31+
* A simple wrapper class that indicates that the wrapped query has made use of NOW
32+
* when parsing its datemath. Useful for preprocessors such as the percolator that
33+
* need to know when not to extract dates from the query.
34+
*/
35+
public class DateRangeIncludingNowQuery extends Query {
36+
37+
private final Query in;
38+
39+
public DateRangeIncludingNowQuery(Query in) {
40+
this.in = in;
41+
}
42+
43+
public Query getQuery() {
44+
return in;
45+
}
46+
47+
@Override
48+
public Query rewrite(IndexReader reader) throws IOException {
49+
return in;
50+
}
51+
52+
@Override
53+
public String toString(String field) {
54+
return "DateRangeIncludingNowQuery(" + in + ")";
55+
}
56+
57+
@Override
58+
public void visit(QueryVisitor visitor) {
59+
in.visit(visitor.getSubVisitor(BooleanClause.Occur.MUST, this));
60+
}
61+
62+
@Override
63+
public boolean equals(Object o) {
64+
if (this == o) return true;
65+
if (o == null || getClass() != o.getClass()) return false;
66+
DateRangeIncludingNowQuery that = (DateRangeIncludingNowQuery) o;
67+
return Objects.equals(in, that.in);
68+
}
69+
70+
@Override
71+
public int hashCode() {
72+
return Objects.hash(in);
73+
}
74+
}

server/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java

-11
Original file line numberDiff line numberDiff line change
@@ -124,15 +124,4 @@ public void onFailure(Exception e) {
124124
}
125125
}
126126

127-
/**
128-
* In pre-processing contexts that happen at index time 'now' date ranges should be replaced by a {@link MatchAllQueryBuilder}.
129-
* Otherwise documents that should match at query time would never match and the document that have fallen outside the
130-
* date range would continue to match.
131-
*
132-
* @return indicates whether range queries with date ranges using 'now' are rewritten to a {@link MatchAllQueryBuilder}.
133-
*/
134-
public boolean convertNowRangeToMatchAll() {
135-
return false;
136-
}
137-
138127
}

server/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java

-10
Original file line numberDiff line numberDiff line change
@@ -451,16 +451,6 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC
451451

452452
@Override
453453
protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException {
454-
// Percolator queries get rewritten and pre-processed at index time.
455-
// If a range query has a date range using 'now' and 'now' gets resolved at index time then
456-
// the pre-processing uses that to pre-process. This can then lead to mismatches at query time.
457-
if (queryRewriteContext.convertNowRangeToMatchAll()) {
458-
if ((from() != null && from().toString().contains("now")) ||
459-
(to() != null && to().toString().contains("now"))) {
460-
return new MatchAllQueryBuilder();
461-
}
462-
}
463-
464454
final MappedFieldType.Relation relation = getRelation(queryRewriteContext);
465455
switch (relation) {
466456
case DISJOINT:

server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java

+10
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import org.elasticsearch.index.mapper.DateFieldMapper.Resolution;
4949
import org.elasticsearch.index.mapper.MappedFieldType.Relation;
5050
import org.elasticsearch.index.mapper.ParseContext.Document;
51+
import org.elasticsearch.index.query.DateRangeIncludingNowQuery;
5152
import org.elasticsearch.index.query.QueryRewriteContext;
5253
import org.elasticsearch.index.query.QueryShardContext;
5354
import org.joda.time.DateTimeZone;
@@ -269,6 +270,15 @@ BigArrays.NON_RECYCLING_INSTANCE, null, null, null, null, null, xContentRegistry
269270
assertEquals(expected,
270271
ft.rangeQuery(date1, date2, true, true, null, null, null, context).rewrite(new MultiReader()));
271272

273+
instant1 = nowInMillis;
274+
instant2 = instant1 + 100;
275+
expected = new DateRangeIncludingNowQuery(new IndexOrDocValuesQuery(
276+
LongPoint.newRangeQuery("field", instant1, instant2),
277+
SortedNumericDocValuesField.newSlowRangeQuery("field", instant1, instant2)
278+
));
279+
assertEquals(expected,
280+
ft.rangeQuery("now", instant2, true, true, null, null, null, context));
281+
272282
ft.setIndexOptions(IndexOptions.NONE);
273283
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
274284
() -> ft.rangeQuery(date1, date2, true, true, null, null, null, context));

server/src/test/java/org/elasticsearch/index/query/RangeQueryBuilderTests.java

+2-29
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,8 @@ public void testDateRangeQueryTimezone() throws IOException {
346346
"}";
347347
QueryShardContext context = createShardContext();
348348
Query parsedQuery = parseQuery(query).toQuery(context);
349+
assertThat(parsedQuery, instanceOf(DateRangeIncludingNowQuery.class));
350+
parsedQuery = ((DateRangeIncludingNowQuery)parsedQuery).getQuery();
349351
assertThat(parsedQuery, instanceOf(IndexOrDocValuesQuery.class));
350352
parsedQuery = ((IndexOrDocValuesQuery) parsedQuery).getIndexQuery();
351353
assertThat(parsedQuery, instanceOf(PointRangeQuery.class));
@@ -569,35 +571,6 @@ public void testParseRelation() {
569571
assertEquals(ShapeRelation.INTERSECTS, builder.relation());
570572
}
571573

572-
public void testConvertNowRangeToMatchAll() throws IOException {
573-
RangeQueryBuilder query = new RangeQueryBuilder(DATE_FIELD_NAME);
574-
DateTime queryFromValue = new DateTime(2019, 1, 1, 0, 0, 0, ISOChronology.getInstanceUTC());
575-
DateTime queryToValue = new DateTime(2020, 1, 1, 0, 0, 0, ISOChronology.getInstanceUTC());
576-
if (randomBoolean()) {
577-
query.from("now");
578-
query.to(queryToValue);
579-
} else if (randomBoolean()) {
580-
query.from(queryFromValue);
581-
query.to("now");
582-
} else {
583-
query.from("now");
584-
query.to("now+1h");
585-
}
586-
QueryShardContext queryShardContext = createShardContext();
587-
QueryBuilder rewritten = query.rewrite(queryShardContext);
588-
assertThat(rewritten, instanceOf(RangeQueryBuilder.class));
589-
590-
queryShardContext = new QueryShardContext(queryShardContext) {
591-
592-
@Override
593-
public boolean convertNowRangeToMatchAll() {
594-
return true;
595-
}
596-
};
597-
rewritten = query.rewrite(queryShardContext);
598-
assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class));
599-
}
600-
601574
public void testTypeField() throws IOException {
602575
RangeQueryBuilder builder = QueryBuilders.rangeQuery("_type")
603576
.from("value1");

0 commit comments

Comments
 (0)