-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Optimize date rounding #128687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize date rounding #128687
Changes from 4 commits
f253a14
1b2ce2a
3a44da9
553b8af
4349769
4fc03ea
dd9ff01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,7 +61,8 @@ public enum DateTimeUnit { | |
|
||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return DateUtils.roundWeekIntervalOfWeekYear(utcMillis, multiplier); | ||
assert multiplier == 1; | ||
return DateUtils.roundWeekOfWeekYear(utcMillis); | ||
} | ||
|
||
@Override | ||
|
@@ -74,7 +75,8 @@ long extraLocalOffsetLookup() { | |
|
||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return multiplier == 1 ? DateUtils.roundYear(utcMillis) : DateUtils.roundYearInterval(utcMillis, multiplier); | ||
assert multiplier == 1; | ||
return DateUtils.roundYear(utcMillis); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please confirm my understanding that this check was showing in profile? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that one specifically, but the equivalent for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If they're small but called a lot, I would statistically expect them to still show up. |
||
} | ||
|
||
@Override | ||
|
@@ -87,9 +89,8 @@ long extraLocalOffsetLookup() { | |
|
||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return multiplier == 1 | ||
? DateUtils.roundQuarterOfYear(utcMillis) | ||
: DateUtils.roundIntervalMonthOfYear(utcMillis, multiplier * 3); | ||
assert multiplier == 1; | ||
return DateUtils.roundQuarterOfYear(utcMillis); | ||
} | ||
|
||
@Override | ||
|
@@ -102,7 +103,8 @@ long extraLocalOffsetLookup() { | |
|
||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return multiplier == 1 ? DateUtils.roundMonthOfYear(utcMillis) : DateUtils.roundIntervalMonthOfYear(utcMillis, multiplier); | ||
assert multiplier == 1; | ||
return DateUtils.roundMonthOfYear(utcMillis); | ||
} | ||
|
||
@Override | ||
|
@@ -113,7 +115,8 @@ long extraLocalOffsetLookup() { | |
DAY_OF_MONTH((byte) 5, "day", ChronoField.DAY_OF_MONTH, true, ChronoField.DAY_OF_MONTH.getBaseUnit().getDuration().toMillis()) { | ||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return DateUtils.roundFloor(utcMillis, this.ratio * multiplier); | ||
assert multiplier == 1; | ||
return DateUtils.roundFloor(utcMillis, this.ratio); | ||
} | ||
|
||
@Override | ||
|
@@ -124,15 +127,16 @@ long extraLocalOffsetLookup() { | |
HOUR_OF_DAY((byte) 6, "hour", ChronoField.HOUR_OF_DAY, true, ChronoField.HOUR_OF_DAY.getBaseUnit().getDuration().toMillis()) { | ||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return DateUtils.roundFloor(utcMillis, ratio * multiplier); | ||
assert multiplier == 1; | ||
return DateUtils.roundFloor(utcMillis, ratio); | ||
} | ||
|
||
@Override | ||
long extraLocalOffsetLookup() { | ||
return ratio; | ||
} | ||
}, | ||
MINUTES_OF_HOUR( | ||
MINUTE_OF_HOUR( | ||
(byte) 7, | ||
"minute", | ||
ChronoField.MINUTE_OF_HOUR, | ||
|
@@ -141,7 +145,8 @@ long extraLocalOffsetLookup() { | |
) { | ||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return DateUtils.roundFloor(utcMillis, ratio * multiplier); | ||
assert multiplier == 1; | ||
return DateUtils.roundFloor(utcMillis, ratio); | ||
} | ||
|
||
@Override | ||
|
@@ -158,13 +163,40 @@ long extraLocalOffsetLookup() { | |
) { | ||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return DateUtils.roundFloor(utcMillis, ratio * multiplier); | ||
assert multiplier == 1; | ||
return DateUtils.roundFloor(utcMillis, ratio); | ||
} | ||
|
||
@Override | ||
long extraLocalOffsetLookup() { | ||
return ratio; | ||
} | ||
}, | ||
YEARS_OF_CENTURY((byte) 9, "years", ChronoField.YEAR_OF_ERA, false, 12) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to add these two new enums to the tests? |
||
private final long extraLocalOffsetLookup = TimeUnit.DAYS.toMillis(366); | ||
|
||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return multiplier == 1 ? DateUtils.roundYear(utcMillis) : DateUtils.roundYearInterval(utcMillis, multiplier); | ||
} | ||
|
||
@Override | ||
long extraLocalOffsetLookup() { | ||
return extraLocalOffsetLookup; | ||
} | ||
}, | ||
MONTHS_OF_YEAR((byte) 10, "months", ChronoField.MONTH_OF_YEAR, false, 1) { | ||
private final long extraLocalOffsetLookup = TimeUnit.DAYS.toMillis(31); | ||
|
||
@Override | ||
long roundFloor(long utcMillis, int multiplier) { | ||
return multiplier == 1 ? DateUtils.roundMonthOfYear(utcMillis) : DateUtils.roundIntervalMonthOfYear(utcMillis, multiplier); | ||
} | ||
|
||
@Override | ||
long extraLocalOffsetLookup() { | ||
return extraLocalOffsetLookup; | ||
} | ||
}; | ||
|
||
private final byte id; | ||
|
@@ -222,8 +254,10 @@ public static DateTimeUnit resolve(byte id) { | |
case 4 -> MONTH_OF_YEAR; | ||
case 5 -> DAY_OF_MONTH; | ||
case 6 -> HOUR_OF_DAY; | ||
case 7 -> MINUTES_OF_HOUR; | ||
case 7 -> MINUTE_OF_HOUR; | ||
case 8 -> SECOND_OF_MINUTE; | ||
case 9 -> YEARS_OF_CENTURY; | ||
case 10 -> MONTHS_OF_YEAR; | ||
default -> throw new ElasticsearchException("Unknown date time unit id [" + id + "]"); | ||
}; | ||
} | ||
|
@@ -480,7 +514,7 @@ private LocalDateTime truncateLocalDateTime(LocalDateTime localDateTime) { | |
case SECOND_OF_MINUTE: | ||
return localDateTime.withNano(0); | ||
|
||
case MINUTES_OF_HOUR: | ||
case MINUTE_OF_HOUR: | ||
return LocalDateTime.of( | ||
localDateTime.getYear(), | ||
localDateTime.getMonthValue(), | ||
|
@@ -517,6 +551,12 @@ private LocalDateTime truncateLocalDateTime(LocalDateTime localDateTime) { | |
case YEAR_OF_CENTURY: | ||
return LocalDateTime.of(LocalDate.of(localDateTime.getYear(), 1, 1), LocalTime.MIDNIGHT); | ||
|
||
case YEARS_OF_CENTURY: | ||
return LocalDateTime.of(LocalDate.of(localDateTime.getYear(), 1, 1), LocalTime.MIDNIGHT); | ||
|
||
case MONTHS_OF_YEAR: | ||
return LocalDateTime.of(localDateTime.getYear(), localDateTime.getMonthValue(), 1, 0, 0); | ||
|
||
default: | ||
throw new IllegalArgumentException("NOT YET IMPLEMENTED for unit " + unit); | ||
} | ||
|
@@ -879,6 +919,8 @@ private LocalDateTime nextRelevantMidnight(LocalDateTime localMidnight) { | |
case MONTH_OF_YEAR -> localMidnight.plus(1, ChronoUnit.MONTHS); | ||
case QUARTER_OF_YEAR -> localMidnight.plus(3, ChronoUnit.MONTHS); | ||
case YEAR_OF_CENTURY -> localMidnight.plus(1, ChronoUnit.YEARS); | ||
case YEARS_OF_CENTURY -> localMidnight.plus(1, ChronoUnit.YEARS); | ||
case MONTHS_OF_YEAR -> localMidnight.plus(1, ChronoUnit.MONTHS); | ||
default -> throw new IllegalArgumentException("Unknown round-to-midnight unit: " + unit); | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this change. If we want the change as a short term one, to profile the outcome in an integrated system, it's OK, but otherwise we would better change the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure either.
Yeah, that's the intention