From eb6f5e2f3be511301835bcb281571f2da79cb8cb Mon Sep 17 00:00:00 2001 From: Moritz Mack Date: Tue, 6 May 2025 09:03:37 +0200 Subject: [PATCH 1/3] 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. --- .../elasticsearch/index/mapper/RangeType.java | 30 ++++++++----------- .../index/mapper/RangeFieldTypeTests.java | 2 -- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java index 0cec1bb9b6a4a..0e374c9b2dc96 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java @@ -37,11 +37,13 @@ import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; +import java.time.temporal.Temporal; import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Set; import java.util.function.BiFunction; +import java.util.function.LongSupplier; /** Enum defining the type of range */ public enum RangeType { @@ -293,28 +295,22 @@ public Query rangeQuery( DateMathParser dateMathParser = (parser == null) ? DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.toDateMathParser() : parser; boolean roundUp = includeLower == false; // using "gt" should round lower bound up - Long low = lowerTerm == null - ? minValue() - : dateMathParser.parse( - lowerTerm instanceof BytesRef ? ((BytesRef) lowerTerm).utf8ToString() : lowerTerm.toString(), - context::nowInMillis, - roundUp, - zone - ).toEpochMilli(); + Long low = lowerTerm == null ? minValue() : parseRangeTerm(dateMathParser, lowerTerm, zone, roundUp, context::nowInMillis); roundUp = includeUpper; // using "lte" should round upper bound up - Long high = upperTerm == null - ? maxValue() - : dateMathParser.parse( - upperTerm instanceof BytesRef ? ((BytesRef) upperTerm).utf8ToString() : upperTerm.toString(), - context::nowInMillis, - roundUp, - zone - ).toEpochMilli(); - + Long high = upperTerm == null ? maxValue() : parseRangeTerm(dateMathParser, upperTerm, zone, roundUp, context::nowInMillis); return createRangeQuery(field, hasDocValues, low, high, includeLower, includeUpper, relation); } + private static long parseRangeTerm(DateMathParser parser, Object term, ZoneId zone, boolean roundUp, LongSupplier nowInMillis) { + String termString = term instanceof BytesRef ? ((BytesRef) term).utf8ToString() : term.toString(); + // `roundUp` may only be used if the term is not a Temporal object. LocalTime#toString() and similar will output + // the shortest possible representation and might omit seconds, milliseconds, etc. + // That interferes badly with the behavior of the roundup parser leading to unexpected results. + boolean mayRoundUp = (term instanceof Temporal) == false; + return parser.parse(termString, nowInMillis, roundUp && mayRoundUp, zone).toEpochMilli(); + } + @Override public Query withinQuery(String field, Object from, Object to, boolean includeLower, boolean includeUpper) { return LONG.withinQuery(field, from, to, includeLower, includeUpper); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java index 1a5f3fbcda6bb..5d8c603d5010b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java @@ -89,7 +89,6 @@ public void testRangeQuery() throws Exception { /** * test the queries are correct if from/to are adjacent and the range is exclusive of those values */ - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/86284") public void testRangeQueryIntersectsAdjacentValues() throws Exception { SearchExecutionContext context = createContext(); ShapeRelation relation = randomFrom(ShapeRelation.values()); @@ -143,7 +142,6 @@ public void testRangeQueryIntersectsAdjacentValues() throws Exception { /** * check that we catch cases where the user specifies larger "from" than "to" value, not counting the include upper/lower settings */ - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/86284") public void testFromLargerToErrors() throws Exception { SearchExecutionContext context = createContext(); RangeFieldType ft = createDefaultFieldType(); From 087aa525e0bc130fab90770f6fcb4aac30052d87 Mon Sep 17 00:00:00 2001 From: Moritz Mack Date: Tue, 6 May 2025 09:19:27 +0200 Subject: [PATCH 2/3] Update docs/changelog/127739.yaml --- docs/changelog/127739.yaml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 docs/changelog/127739.yaml diff --git a/docs/changelog/127739.yaml b/docs/changelog/127739.yaml new file mode 100644 index 0000000000000..e88ff1a6b51c1 --- /dev/null +++ b/docs/changelog/127739.yaml @@ -0,0 +1,7 @@ +pr: 127739 +summary: Fix inconsistent roundup behavior for date ranges using Temporal object as + terms +area: Infra/Core +type: bug +issues: + - 86284 From 4e23cd0be505a0a9081638ba4e0c82e5c05946b1 Mon Sep 17 00:00:00 2001 From: Moritz Mack Date: Wed, 7 May 2025 10:58:20 +0200 Subject: [PATCH 3/3] PR feedback --- .../elasticsearch/index/mapper/RangeType.java | 17 ++++++---- .../index/mapper/RangeFieldTypeTests.java | 34 +++++++++++++++++++ 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java index 0e374c9b2dc96..1ad65874d1872 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java @@ -303,12 +303,17 @@ public Query rangeQuery( } private static long parseRangeTerm(DateMathParser parser, Object term, ZoneId zone, boolean roundUp, LongSupplier nowInMillis) { - String termString = term instanceof BytesRef ? ((BytesRef) term).utf8ToString() : term.toString(); - // `roundUp` may only be used if the term is not a Temporal object. LocalTime#toString() and similar will output - // the shortest possible representation and might omit seconds, milliseconds, etc. - // That interferes badly with the behavior of the roundup parser leading to unexpected results. - boolean mayRoundUp = (term instanceof Temporal) == false; - return parser.parse(termString, nowInMillis, roundUp && mayRoundUp, zone).toEpochMilli(); + String termString; + if (term instanceof BytesRef bytesRefTerm) { + termString = bytesRefTerm.utf8ToString(); + } else { + termString = term.toString(); + // `roundUp` may only be used if the term is not a Temporal object. LocalTime#toString() and similar will output + // the shortest possible representation and might omit seconds, milliseconds, etc. + // That interferes badly with the behavior of the roundup parser leading to unexpected results. + roundUp = roundUp && (term instanceof Temporal) == false; + } + return parser.parse(termString, nowInMillis, roundUp, zone).toEpochMilli(); } @Override diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java index 5d8c603d5010b..ec4f4fe427a2e 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java @@ -86,6 +86,40 @@ public void testRangeQuery() throws Exception { ); } + public void testDateRangeQueryDoesNotRoundupTemporals() { + type = RangeType.DATE; + SearchExecutionContext context = createContext(); + RangeFieldType ft = createDefaultFieldType(); + ShapeRelation relation = randomFrom(ShapeRelation.values()); + + // explicitly not using nextFrom here to provide a concrete, visual example. + // see the behavioral difference compared to testRangeQueryIntersectsAdjacentDates + ZonedDateTime from = ZonedDateTime.parse("2025-05-01T14:10:00.000Z"); + ZonedDateTime to = ZonedDateTime.parse("2025-05-01T14:11:00.000Z"); + + // usage of roundUp parsers is wrong if terms are Temporal objects, otherwise it would arbitrarily change the bounds + // depending on the string representation + assertEquals( + getExpectedRangeQuery(relation, from, to, false, true), + ft.rangeQuery(from, to, false, true, relation, null, null, context) + ); + } + + public void testRangeQueryIntersectsAdjacentDates() { + type = RangeType.DATE; + SearchExecutionContext context = createContext(); + ShapeRelation relation = randomFrom(ShapeRelation.values()); + RangeFieldType ft = createDefaultFieldType(); + + // explicitly not using nextFrom here to provide a concrete, visual example of the roundUp behavior + String from = "2025-05-01T14:10Z"; // transformed to 2025-05-01T14:10:59.999999999Z by roundUp parser + String to = "2025-05-01T14:11Z"; + + Query rangeQuery = ft.rangeQuery(from, to, false, false, relation, null, null, context); + assertThat(rangeQuery, instanceOf(IndexOrDocValuesQuery.class)); + assertThat(((IndexOrDocValuesQuery) rangeQuery).getIndexQuery(), instanceOf(MatchNoDocsQuery.class)); + } + /** * test the queries are correct if from/to are adjacent and the range is exclusive of those values */