Skip to content

Commit

Permalink
Fix round up of date range without rounding (#43303)
Browse files Browse the repository at this point in the history
Today when searching for an exclusive range the java date math parser rounds up the value
with the granularity of the operation. So when searching for values that are greater than
"now-2M" the parser rounds up the operation to "now-1M". This behavior was introduced when
we migrated to java date but it looks like a bug since the joda math parser rounds up values
but only when a rounding is used. So "now/M" is rounded to "now-1ms" (minus 1ms to get the largest inclusive value)
in the joda parser if the result should be exclusive but no rounding is applied if the input
is a simple operation like "now-1M". This change restores the joda behavior in order to have
a consistent parsing in all versions.

Closes #43277
  • Loading branch information
jimczi authored Jun 20, 2019
1 parent d0984db commit 215fc4c
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
public interface DateMathParser {

/**
* Parse a date math expression without timzeone info and rounding down.
* Parse a date math expression without timezone info and rounding down.
*/
default Instant parse(String text, LongSupplier now) {
return parse(text, now, false, (ZoneId) null);
Expand All @@ -43,8 +43,8 @@ default Instant parse(String text, LongSupplier now) {

// exists for backcompat, do not use!
@Deprecated
default Instant parse(String text, LongSupplier now, boolean roundUp, DateTimeZone tz) {
return parse(text, now, roundUp, tz == null ? null : ZoneId.of(tz.getID()));
default Instant parse(String text, LongSupplier now, boolean roundUpProperty, DateTimeZone tz) {
return parse(text, now, roundUpProperty, tz == null ? null : ZoneId.of(tz.getID()));
}

/**
Expand All @@ -65,11 +65,11 @@ default Instant parse(String text, LongSupplier now, boolean roundUp, DateTimeZo
* s second
*
*
* @param text the input
* @param now a supplier to retrieve the current date in milliseconds, if needed for additions
* @param roundUp should the result be rounded up
* @param tz an optional timezone that should be applied before returning the milliseconds since the epoch
* @return the parsed date as an Instant since the epoch
* @param text the input
* @param now a supplier to retrieve the current date in milliseconds, if needed for additions
* @param roundUpProperty should the result be rounded up with the granularity of the rounding (e.g. <code>now/M</code>)
* @param tz an optional timezone that should be applied before returning the milliseconds since the epoch
* @return the parsed date as an Instant since the epoch
*/
Instant parse(String text, LongSupplier now, boolean roundUp, ZoneId tz);
Instant parse(String text, LongSupplier now, boolean roundUpProperty, ZoneId tz);
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public class JavaDateMathParser implements DateMathParser {
}

@Override
public Instant parse(String text, LongSupplier now, boolean roundUp, ZoneId timeZone) {
public Instant parse(String text, LongSupplier now, boolean roundUpProperty, ZoneId timeZone) {
Instant time;
String mathString;
if (text.startsWith("now")) {
Expand All @@ -73,16 +73,16 @@ public Instant parse(String text, LongSupplier now, boolean roundUp, ZoneId time
} else {
int index = text.indexOf("||");
if (index == -1) {
return parseDateTime(text, timeZone, roundUp);
return parseDateTime(text, timeZone, roundUpProperty);
}
time = parseDateTime(text.substring(0, index), timeZone, false);
mathString = text.substring(index + 2);
}

return parseMath(mathString, time, roundUp, timeZone);
return parseMath(mathString, time, roundUpProperty, timeZone);
}

private Instant parseMath(final String mathString, final Instant time, final boolean roundUp,
private Instant parseMath(final String mathString, final Instant time, final boolean roundUpProperty,
ZoneId timeZone) throws ElasticsearchParseException {
if (timeZone == null) {
timeZone = ZoneOffset.UTC;
Expand Down Expand Up @@ -133,78 +133,79 @@ private Instant parseMath(final String mathString, final Instant time, final boo
case 'y':
if (round) {
dateTime = dateTime.withDayOfYear(1).with(LocalTime.MIN);
if (roundUpProperty) {
dateTime = dateTime.plusYears(1);
}
} else {
dateTime = dateTime.plusYears(sign * num);
}
if (roundUp) {
dateTime = dateTime.plusYears(1);
}
break;
case 'M':
if (round) {
dateTime = dateTime.withDayOfMonth(1).with(LocalTime.MIN);
if (roundUpProperty) {
dateTime = dateTime.plusMonths(1);
}
} else {
dateTime = dateTime.plusMonths(sign * num);
}
if (roundUp) {
dateTime = dateTime.plusMonths(1);
}
break;
case 'w':
if (round) {
dateTime = dateTime.with(TemporalAdjusters.previousOrSame(DayOfWeek.MONDAY)).with(LocalTime.MIN);
if (roundUpProperty) {
dateTime = dateTime.plusWeeks(1);
}
} else {
dateTime = dateTime.plusWeeks(sign * num);
}
if (roundUp) {
dateTime = dateTime.plusWeeks(1);
}
break;
case 'd':
if (round) {
dateTime = dateTime.with(LocalTime.MIN);
if (roundUpProperty) {
dateTime = dateTime.plusDays(1);
}
} else {
dateTime = dateTime.plusDays(sign * num);
}
if (roundUp) {
dateTime = dateTime.plusDays(1);
}
break;
case 'h':
case 'H':
if (round) {
dateTime = dateTime.withMinute(0).withSecond(0).withNano(0);
if (roundUpProperty) {
dateTime = dateTime.plusHours(1);
}
} else {
dateTime = dateTime.plusHours(sign * num);
}
if (roundUp) {
dateTime = dateTime.plusHours(1);
}
break;
case 'm':
if (round) {
dateTime = dateTime.withSecond(0).withNano(0);
if (roundUpProperty) {
dateTime = dateTime.plusMinutes(1);
}
} else {
dateTime = dateTime.plusMinutes(sign * num);
}
if (roundUp) {
dateTime = dateTime.plusMinutes(1);
}
break;
case 's':
if (round) {
dateTime = dateTime.withNano(0);
if (roundUpProperty) {
dateTime = dateTime.plusSeconds(1);
}
} else {
dateTime = dateTime.plusSeconds(sign * num);
}
if (roundUp) {
dateTime = dateTime.plusSeconds(1);
}
break;
default:
throw new ElasticsearchParseException("unit [{}] not supported for date math [{}]", unit, mathString);
}
if (roundUp) {
if (round && roundUpProperty) {
// subtract 1 millisecond to get the largest inclusive value
dateTime = dateTime.minus(1, ChronoField.MILLI_OF_SECOND.getBaseUnit());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,13 @@ public void testNow() {

assertDateMathEquals("now", "2014-11-18T14:27:32", now, false, null);
assertDateMathEquals("now+M", "2014-12-18T14:27:32", now, false, null);
assertDateMathEquals("now+M", "2014-12-18T14:27:32", now, true, null);
assertDateMathEquals("now-2d", "2014-11-16T14:27:32", now, false, null);
assertDateMathEquals("now-2d", "2014-11-16T14:27:32", now, true, null);
assertDateMathEquals("now/m", "2014-11-18T14:27", now, false, null);
assertDateMathEquals("now/m", "2014-11-18T14:27:59.999Z", now, true, null);
assertDateMathEquals("now/M", "2014-11-01T00:00:00", now, false, null);
assertDateMathEquals("now/M", "2014-11-30T23:59:59.999Z", now, true, null);

// timezone does not affect now
assertDateMathEquals("now/m", "2014-11-18T14:27", now, false, DateTimeZone.forID("+02:00"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,13 @@ public void testNow() {

assertDateMathEquals("now", "2014-11-18T14:27:32", now, false, null);
assertDateMathEquals("now+M", "2014-12-18T14:27:32", now, false, null);
assertDateMathEquals("now+M", "2014-12-18T14:27:32", now, true, null);
assertDateMathEquals("now-2d", "2014-11-16T14:27:32", now, false, null);
assertDateMathEquals("now-2d", "2014-11-16T14:27:32", now, true, null);
assertDateMathEquals("now/m", "2014-11-18T14:27", now, false, null);
assertDateMathEquals("now/m", "2014-11-18T14:27:59.999Z", now, true, null);
assertDateMathEquals("now/M", "2014-11-01T00:00:00", now, false, null);
assertDateMathEquals("now/M", "2014-11-30T23:59:59.999Z", now, true, null);

// timezone does not affect now
assertDateMathEquals("now/m", "2014-11-18T14:27", now, false, ZoneId.of("+02:00"));
Expand Down

0 comments on commit 215fc4c

Please sign in to comment.