Skip to content

Commit

Permalink
Correct how range queries serialize to xContent (elastic#81276)
Browse files Browse the repository at this point in the history
Previously range queries used the 'legacy' way of specifying the bounds, which
includes from, to, include_lower, and include_upper. These are considered
deprecated, so we should not be using them in the query output. This commit
switches to the updated notation gte, gt, lte, and lt.
  • Loading branch information
jtibshirani authored Dec 8, 2021
1 parent 2dec141 commit 6652249
Show file tree
Hide file tree
Showing 13 changed files with 107 additions and 93 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,8 @@ public void testFromJson() throws IOException {
+ " \"query\" : {\n"
+ " \"range\" : {\n"
+ " \"mapped_string\" : {\n"
+ " \"from\" : \"agJhRET\",\n"
+ " \"to\" : \"zvqIq\",\n"
+ " \"include_lower\" : true,\n"
+ " \"include_upper\" : true,\n"
+ " \"gte\" : \"agJhRET\",\n"
+ " \"lte\" : \"zvqIq\",\n"
+ " \"boost\" : 1.0\n"
+ " }\n"
+ " }\n"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,10 +306,25 @@ public RangeQueryBuilder relation(String relation) {
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(NAME);
builder.startObject(fieldName);
builder.field(FROM_FIELD.getPreferredName(), maybeConvertToString(this.from));
builder.field(TO_FIELD.getPreferredName(), maybeConvertToString(this.to));
builder.field(INCLUDE_LOWER_FIELD.getPreferredName(), includeLower);
builder.field(INCLUDE_UPPER_FIELD.getPreferredName(), includeUpper);

Object from = maybeConvertToString(this.from);
if (from != null) {
if (includeLower) {
builder.field(GTE_FIELD.getPreferredName(), from);
} else {
builder.field(GT_FIELD.getPreferredName(), from);
}
}

Object to = maybeConvertToString(this.to);
if (to != null) {
if (includeUpper) {
builder.field(LTE_FIELD.getPreferredName(), to);
} else {
builder.field(LT_FIELD.getPreferredName(), to);
}
}

if (timeZone != null) {
builder.field(TIME_ZONE_FIELD.getPreferredName(), timeZone.getId());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,8 @@ public void testFromJson() throws IOException {
+ " \"must_not\" : [ {"
+ " \"range\" : {"
+ " \"age\" : {"
+ " \"from\" : 10,"
+ " \"to\" : 20,"
+ " \"include_lower\" : true,"
+ " \"include_upper\" : true,"
+ " \"gte\" : 10,"
+ " \"lte\" : 20,"
+ " \"boost\" : 1.0"
+ " }"
+ " }"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,7 @@ public void testFromJson() throws IOException {
+ " }, {\n"
+ " \"range\" : {\n"
+ " \"obj1.count\" : {\n"
+ " \"from\" : 5,\n"
+ " \"to\" : null,\n"
+ " \"include_lower\" : false,\n"
+ " \"include_upper\" : true,\n"
+ " \"gt\" : 5,\n"
+ " \"boost\" : 1.0\n"
+ " }\n"
+ " }\n"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,23 @@ public class RangeQueryBuilderTests extends AbstractQueryTestCase<RangeQueryBuil
@Override
protected RangeQueryBuilder doCreateTestQueryBuilder() {
RangeQueryBuilder query;
Object lower, upper;
// switch between numeric and date ranges
switch (randomIntBetween(0, 2)) {
case 0:
// use mapped integer field for numeric range queries
query = new RangeQueryBuilder(randomFrom(INT_FIELD_NAME, INT_RANGE_FIELD_NAME, INT_ALIAS_FIELD_NAME));
query.from(randomIntBetween(1, 100));
query.to(randomIntBetween(101, 200));
lower = randomIntBetween(1, 100);
upper = randomIntBetween(101, 200);
break;
case 1:
// use mapped date field, using date string representation
Instant now = Instant.now();
ZonedDateTime start = now.minusMillis(randomIntBetween(0, 1000000)).atZone(ZoneOffset.UTC);
ZonedDateTime end = now.plusMillis(randomIntBetween(0, 1000000)).atZone(ZoneOffset.UTC);
query = new RangeQueryBuilder(randomFrom(DATE_FIELD_NAME, DATE_RANGE_FIELD_NAME, DATE_ALIAS_FIELD_NAME));
query.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(start));
query.to(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(end));
lower = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(start);
upper = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(end);
// Create timestamp option only then we have a date mapper,
// otherwise we could trigger exception.
if (createSearchExecutionContext().getFieldType(DATE_FIELD_NAME) != null) {
Expand All @@ -77,17 +78,29 @@ protected RangeQueryBuilder doCreateTestQueryBuilder() {
case 2:
default:
query = new RangeQueryBuilder(randomFrom(TEXT_FIELD_NAME, TEXT_ALIAS_FIELD_NAME));
query.from("a" + randomAlphaOfLengthBetween(1, 10));
query.to("z" + randomAlphaOfLengthBetween(1, 10));
lower = "a" + randomAlphaOfLengthBetween(1, 10);
upper = "z" + randomAlphaOfLengthBetween(1, 10);
break;
}
query.includeLower(randomBoolean()).includeUpper(randomBoolean());

// Update query builder with lower bound, sometimes leaving it unset
if (randomBoolean()) {
query.from(null);
if (randomBoolean()) {
query.gte(lower);
} else {
query.gt(lower);
}
}

// Update query builder with upper bound, sometimes leaving it unset
if (randomBoolean()) {
query.to(null);
if (randomBoolean()) {
query.lte(upper);
} else {
query.lt(upper);
}
}

if (query.fieldName().equals(INT_RANGE_FIELD_NAME) || query.fieldName().equals(DATE_RANGE_FIELD_NAME)) {
query.relation(
randomFrom(ShapeRelation.CONTAINS.toString(), ShapeRelation.INTERSECTS.toString(), ShapeRelation.WITHIN.toString())
Expand All @@ -100,22 +113,33 @@ protected RangeQueryBuilder doCreateTestQueryBuilder() {
protected Map<String, RangeQueryBuilder> getAlternateVersions() {
Map<String, RangeQueryBuilder> alternateVersions = new HashMap<>();
RangeQueryBuilder rangeQueryBuilder = new RangeQueryBuilder(INT_FIELD_NAME);
rangeQueryBuilder.from(randomIntBetween(1, 100)).to(randomIntBetween(101, 200));

rangeQueryBuilder.includeLower(randomBoolean());
rangeQueryBuilder.includeUpper(randomBoolean());

if (randomBoolean()) {
rangeQueryBuilder.from(randomIntBetween(1, 100));
}

if (randomBoolean()) {
rangeQueryBuilder.to(randomIntBetween(101, 200));
}

String query = "{\n"
+ " \"range\":{\n"
+ " \""
+ INT_FIELD_NAME
+ "\": {\n"
+ " \""
+ (rangeQueryBuilder.includeLower() ? "gte" : "gt")
+ "\": "
+ " \"include_lower\":"
+ rangeQueryBuilder.includeLower()
+ ",\n"
+ " \"include_upper\":"
+ rangeQueryBuilder.includeUpper()
+ ",\n"
+ " \"from\":"
+ rangeQueryBuilder.from()
+ ",\n"
+ " \""
+ (rangeQueryBuilder.includeUpper() ? "lte" : "lt")
+ "\": "
+ " \"to\":"
+ rangeQueryBuilder.to()
+ "\n"
+ " }\n"
Expand Down Expand Up @@ -244,11 +268,7 @@ public void testIllegalArguments() {
}

public void testToQueryNumericField() throws IOException {
Query parsedQuery = rangeQuery(INT_FIELD_NAME).from(23)
.to(54)
.includeLower(true)
.includeUpper(false)
.toQuery(createSearchExecutionContext());
Query parsedQuery = rangeQuery(INT_FIELD_NAME).gte(23).lt(54).toQuery(createSearchExecutionContext());
// since age is automatically registered in data, we encode it as numeric
assertThat(parsedQuery, instanceOf(IndexOrDocValuesQuery.class));
parsedQuery = ((IndexOrDocValuesQuery) parsedQuery).getIndexQuery();
Expand Down Expand Up @@ -386,10 +406,8 @@ public void testFromJson() throws IOException {
String json = "{\n"
+ " \"range\" : {\n"
+ " \"timestamp\" : {\n"
+ " \"from\" : \"2015-01-01 00:00:00\",\n"
+ " \"to\" : \"now\",\n"
+ " \"include_lower\" : true,\n"
+ " \"include_upper\" : true,\n"
+ " \"gte\" : \"2015-01-01 00:00:00\",\n"
+ " \"lte\" : \"now\",\n"
+ " \"time_zone\" : \"+01:00\",\n"
+ " \"boost\" : 1.0\n"
+ " }\n"
Expand Down Expand Up @@ -427,8 +445,8 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC
};
ZonedDateTime queryFromValue = ZonedDateTime.of(2015, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
ZonedDateTime queryToValue = ZonedDateTime.of(2016, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
query.from(queryFromValue);
query.to(queryToValue);
query.gte(queryFromValue);
query.lte(queryToValue);
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
QueryBuilder rewritten = query.rewrite(searchExecutionContext);
assertThat(rewritten, instanceOf(RangeQueryBuilder.class));
Expand Down Expand Up @@ -462,8 +480,8 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC
};
ZonedDateTime queryFromValue = ZonedDateTime.of(2015, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
ZonedDateTime queryToValue = ZonedDateTime.of(2016, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
query.from(queryFromValue);
query.to(queryToValue);
query.gte(queryFromValue);
query.lte(queryToValue);
query.timeZone(randomZone().getId());
query.format("yyyy-MM-dd");
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
Expand All @@ -487,8 +505,8 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC
};
ZonedDateTime queryFromValue = ZonedDateTime.of(2015, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
ZonedDateTime queryToValue = ZonedDateTime.of(2016, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
query.from(queryFromValue);
query.to(queryToValue);
query.gte(queryFromValue);
query.lte(queryToValue);
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
QueryBuilder rewritten = query.rewrite(searchExecutionContext);
assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class));
Expand All @@ -504,8 +522,8 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC
};
ZonedDateTime queryFromValue = ZonedDateTime.of(2015, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
ZonedDateTime queryToValue = ZonedDateTime.of(2016, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
query.from(queryFromValue);
query.to(queryToValue);
query.gte(queryFromValue);
query.lte(queryToValue);
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
QueryBuilder rewritten = query.rewrite(searchExecutionContext);
assertThat(rewritten, sameInstance(query));
Expand Down
24 changes: 12 additions & 12 deletions x-pack/plugin/eql/src/test/resources/querytranslator_tests.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,25 @@ contains "term":{"serial_event_id":{"value":1,"boost":1.0}
singleNumericFilterLess
process where serial_event_id < 4
;
"range":{"serial_event_id":{"from":null,"to":4,"include_lower":false,"include_upper":false
"range":{"serial_event_id":{"lt":4
;

singleNumericFilterLessEquals
process where serial_event_id <= 4
;
"range":{"serial_event_id":{"from":null,"to":4,"include_lower":false,"include_upper":true
"range":{"serial_event_id":{"lte":4
;

singleNumericFilterGreater
process where serial_event_id > 4
;
"range":{"serial_event_id":{"from":4,"to":null,"include_lower":false,"include_upper":false
"range":{"serial_event_id":{"gt":4
;

singleNumericFilterGreaterEquals
process where serial_event_id >= 4
;
"range":{"serial_event_id":{"from":4,"to":null,"include_lower":true,"include_upper":false
"range":{"serial_event_id":{"gte":4
;

caseInsensitiveEquals
Expand Down Expand Up @@ -93,27 +93,27 @@ mixedTypeFilter
process where process_name : "notepad.exe" or (serial_event_id < 4.5 and serial_event_id >= 3.1)
;
"term":{"process_name":{"value":"notepad.exe","case_insensitive":true,"boost":1.0}
{"bool":{"must":[{"range":{"serial_event_id":{"from":null,"to":4.5,"include_lower":false,"include_upper":false
{"range":{"serial_event_id":{"from":3.1,"to":null,"include_lower":true,"include_upper":false
{"bool":{"must":[{"range":{"serial_event_id":{"lt":4.5
{"range":{"serial_event_id":{"gte":3.1
;

notFilter
process where not (exit_code > -1)
;
{"bool":{"must_not":[{"range":{"exit_code":{"from":-1,"to":null,"include_lower":false,"include_upper":false
{"bool":{"must_not":[{"range":{"exit_code":{"gt":-1
;

notFieldsConjunction
process where not (exit_code > 1 and user_name == "root")
;
{"bool":{"must_not":[{"bool":{"must":[{"range":{"exit_code":{"from":1,"to":null,"include_lower":false,"include_upper":false
{"bool":{"must_not":[{"bool":{"must":[{"range":{"exit_code":{"gt":1
{"term":{"user_name":{"value":"root"
;

notOneFieldConjunctionNotSecondField
process where not (exit_code > 1) and not (user_name == "root")
;
{"bool":{"must":[{"bool":{"must_not":[{"range":{"exit_code":{"from":1,"to":null,"include_lower":false,"include_upper":false
{"bool":{"must":[{"bool":{"must_not":[{"range":{"exit_code":{"gt":1
{"bool":{"must_not":[{"term":{"user_name":{"value":"root"
;

Expand Down Expand Up @@ -893,11 +893,11 @@ twoRangesOnASingleField
// two range queries, because a single range query will apply the two conditions on the same value of the multi-value field.
process where pid > 100 and pid < 200
;
{"bool":{"must":[{"range":{"pid":{"from":100,"to":null,"include_lower":false,"include_upper":false
{"range":{"pid":{"from":null,"to":200,"include_lower":false,"include_upper":false
{"bool":{"must":[{"range":{"pid":{"gt":100
{"range":{"pid":{"lt":200
;

// this type of query (disjunction/conjunction inside a function where one of the conditions is negated) is currently unsupported because
// the "not" has an undefined location of where it should be used in the Painless script
// disjunctionInsideFunctionWithNot
// process where string(pid > 5 and pid != 10) == \"true\"
// process where string(pid > 5 and pid != 10) == \"true\"
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public void testExtraction() throws IOException {
searchRequest,
containsString(
"\"query\":{\"bool\":{\"filter\":[{\"match_all\":{\"boost\":1.0}},"
+ "{\"range\":{\"time\":{\"from\":0,\"to\":4000,\"include_lower\":true,\"include_upper\":false,"
+ "{\"range\":{\"time\":{\"gte\":0,\"lt\":4000,"
+ "\"format\":\"epoch_millis\",\"boost\":1.0}}}]"
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ public void testExtraction() throws IOException {
searchRequest,
containsString(
"\"query\":{\"bool\":{\"filter\":[{\"match_all\":{\"boost\":1.0}},"
+ "{\"range\":{\"time\":{\"from\":1000,\"to\":4000,\"include_lower\":true,\"include_upper\":false,"
+ "{\"range\":{\"time\":{\"gte\":1000,\"lt\":4000,"
+ "\"format\":\"epoch_millis\",\"boost\":1.0}}}]"
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public void testExtractionGivenSpecifiedChunk() throws IOException {
searchRequest,
containsString(
"\"query\":{\"bool\":{\"filter\":[{\"match_all\":{\"boost\":1.0}},"
+ "{\"range\":{\"time\":{\"from\":1000,\"to\":2300,\"include_lower\":true,\"include_upper\":false,"
+ "{\"range\":{\"time\":{\"gte\":1000,\"lt\":2300,"
+ "\"format\":\"epoch_millis\",\"boost\":1.0}}}]"
)
);
Expand Down Expand Up @@ -201,7 +201,7 @@ public void testExtractionGivenSpecifiedChunkAndAggs() throws IOException {
searchRequest,
containsString(
"\"query\":{\"bool\":{\"filter\":[{\"match_all\":{\"boost\":1.0}},"
+ "{\"range\":{\"time\":{\"from\":1000,\"to\":2300,\"include_lower\":true,\"include_upper\":false,"
+ "{\"range\":{\"time\":{\"gte\":1000,\"lt\":2300,"
+ "\"format\":\"epoch_millis\",\"boost\":1.0}}}]"
)
);
Expand Down Expand Up @@ -440,9 +440,9 @@ public void testExtractionGivenAutoChunkAndIntermediateEmptySearchShouldReconfig
assertThat(capturedSearchRequests.size(), equalTo(2));

String searchRequest = capturedSearchRequests.get(0).toString().replaceAll("\\s", "");
assertThat(searchRequest, containsString("\"from\":100000,\"to\":400000"));
assertThat(searchRequest, containsString("\"gte\":100000,\"lt\":400000"));
searchRequest = capturedSearchRequests.get(1).toString().replaceAll("\\s", "");
assertThat(searchRequest, containsString("\"from\":200000,\"to\":400000"));
assertThat(searchRequest, containsString("\"gte\":200000,\"lt\":400000"));
}

public void testCancelGivenNextWasNeverCalled() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public void testSinglePageExtraction() throws IOException {
searchRequest,
containsString(
"\"query\":{\"bool\":{\"filter\":[{\"match_all\":{\"boost\":1.0}},"
+ "{\"range\":{\"time\":{\"from\":1000,\"to\":2000,\"include_lower\":true,\"include_upper\":false,"
+ "{\"range\":{\"time\":{\"gte\":1000,\"lt\":2000,"
+ "\"format\":\"epoch_millis\",\"boost\":1.0}}}]"
)
);
Expand Down Expand Up @@ -248,7 +248,7 @@ public void testMultiplePageExtraction() throws IOException {
searchRequest1,
containsString(
"\"query\":{\"bool\":{\"filter\":[{\"match_all\":{\"boost\":1.0}},"
+ "{\"range\":{\"time\":{\"from\":1000,\"to\":10000,\"include_lower\":true,\"include_upper\":false,"
+ "{\"range\":{\"time\":{\"gte\":1000,\"lt\":10000,"
+ "\"format\":\"epoch_millis\",\"boost\":1.0}}}]"
)
);
Expand Down Expand Up @@ -477,7 +477,7 @@ public void testDomainSplitScriptField() throws IOException {
searchRequest,
containsString(
"\"query\":{\"bool\":{\"filter\":[{\"match_all\":{\"boost\":1.0}},"
+ "{\"range\":{\"time\":{\"from\":1000,\"to\":2000,\"include_lower\":true,\"include_upper\":false,"
+ "{\"range\":{\"time\":{\"gte\":1000,\"lt\":2000,"
+ "\"format\":\"epoch_millis\",\"boost\":1.0}}}]"
)
);
Expand Down
Loading

0 comments on commit 6652249

Please sign in to comment.