Skip to content

Commit eb6f5e2

Browse files
committed
Fix inconsistent roundup behavior for date ranges using Temporal object as terms
`roundUp` works well for terms that contain date math expressions. Otherwise, if falling through to using roundUp parsers, the behavior can be surprising. Instead, these parsers fill missing date components with the max possible value, e.g. `2099-12` becomes `2099-12-01T23:59:59.999_999_999Z`. Prior to switching to Java time, Joda's LocalTime#toString() consistently formatted time using `HH:mm:ss.SSS`. Without and missing component, roundUp parsers behave just like normal parsers. Java's LocalTime, however, outputs the shortest possible representation and might omit seconds, milliseconds, etc. This interferes badly with the behavior of the roundup parser. And as a result of this, depending on the term, the lower bound could end up larger than the upper bound, e.g. `1970-01-06T09:49Z` becomes `1970-01-06T09:49:59.999Z`. This PR disables roundUp parsers if the lower/upper term is a Temporal object to restore the behavior prior to switching to Java time.
1 parent 80946ce commit eb6f5e2

File tree

2 files changed

+13
-19
lines changed

2 files changed

+13
-19
lines changed

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

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,13 @@
3737
import java.time.ZoneId;
3838
import java.time.ZoneOffset;
3939
import java.time.ZonedDateTime;
40+
import java.time.temporal.Temporal;
4041
import java.util.ArrayList;
4142
import java.util.Arrays;
4243
import java.util.List;
4344
import java.util.Set;
4445
import java.util.function.BiFunction;
46+
import java.util.function.LongSupplier;
4547

4648
/** Enum defining the type of range */
4749
public enum RangeType {
@@ -293,28 +295,22 @@ public Query rangeQuery(
293295

294296
DateMathParser dateMathParser = (parser == null) ? DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.toDateMathParser() : parser;
295297
boolean roundUp = includeLower == false; // using "gt" should round lower bound up
296-
Long low = lowerTerm == null
297-
? minValue()
298-
: dateMathParser.parse(
299-
lowerTerm instanceof BytesRef ? ((BytesRef) lowerTerm).utf8ToString() : lowerTerm.toString(),
300-
context::nowInMillis,
301-
roundUp,
302-
zone
303-
).toEpochMilli();
298+
Long low = lowerTerm == null ? minValue() : parseRangeTerm(dateMathParser, lowerTerm, zone, roundUp, context::nowInMillis);
304299

305300
roundUp = includeUpper; // using "lte" should round upper bound up
306-
Long high = upperTerm == null
307-
? maxValue()
308-
: dateMathParser.parse(
309-
upperTerm instanceof BytesRef ? ((BytesRef) upperTerm).utf8ToString() : upperTerm.toString(),
310-
context::nowInMillis,
311-
roundUp,
312-
zone
313-
).toEpochMilli();
314-
301+
Long high = upperTerm == null ? maxValue() : parseRangeTerm(dateMathParser, upperTerm, zone, roundUp, context::nowInMillis);
315302
return createRangeQuery(field, hasDocValues, low, high, includeLower, includeUpper, relation);
316303
}
317304

305+
private static long parseRangeTerm(DateMathParser parser, Object term, ZoneId zone, boolean roundUp, LongSupplier nowInMillis) {
306+
String termString = term instanceof BytesRef ? ((BytesRef) term).utf8ToString() : term.toString();
307+
// `roundUp` may only be used if the term is not a Temporal object. LocalTime#toString() and similar will output
308+
// the shortest possible representation and might omit seconds, milliseconds, etc.
309+
// That interferes badly with the behavior of the roundup parser leading to unexpected results.
310+
boolean mayRoundUp = (term instanceof Temporal) == false;
311+
return parser.parse(termString, nowInMillis, roundUp && mayRoundUp, zone).toEpochMilli();
312+
}
313+
318314
@Override
319315
public Query withinQuery(String field, Object from, Object to, boolean includeLower, boolean includeUpper) {
320316
return LONG.withinQuery(field, from, to, includeLower, includeUpper);

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ public void testRangeQuery() throws Exception {
8989
/**
9090
* test the queries are correct if from/to are adjacent and the range is exclusive of those values
9191
*/
92-
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/86284")
9392
public void testRangeQueryIntersectsAdjacentValues() throws Exception {
9493
SearchExecutionContext context = createContext();
9594
ShapeRelation relation = randomFrom(ShapeRelation.values());
@@ -143,7 +142,6 @@ public void testRangeQueryIntersectsAdjacentValues() throws Exception {
143142
/**
144143
* check that we catch cases where the user specifies larger "from" than "to" value, not counting the include upper/lower settings
145144
*/
146-
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/86284")
147145
public void testFromLargerToErrors() throws Exception {
148146
SearchExecutionContext context = createContext();
149147
RangeFieldType ft = createDefaultFieldType();

0 commit comments

Comments
 (0)