DateMath: Fix semantics of rounding with inclusive/exclusive ranges. #8556
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Date math rounding currently works by rounding the date up or down based on the scope of the rounding. For example, if you have the date `2009-12-24||/d` it will round down to the inclusive lower end `2009-12-24T00:00:00.000` and round up to the non-inclusive date `2009-12-25T00:00:00.000`. The range endpoint semantics work as follows: gt - round D down, and use > that value gte - round D down, and use >= that value lt - round D down, and use < lte - round D up, and use <= There are 2 problems with these semantics: 1. lte ends up including the upper value, which should be non-inclusive 2. gt only excludes the beginning of the date, not the entire rounding scope This change makes the range endpoint semantics symmetrical. First, it changes the parser to round up and down using the first (same as before) and last (1 ms less than before) values of the rounding scope. This makes both rounded endpoints inclusive. The range endpoint semantics are then as follows: gt - round D up, and use > that value gte - round D down, and use >= that value lt - round D down, and use < that value lte - round D up, and use <= that value closes elastic#8424
| long now = context == null ? System.currentTimeMillis() : context.nowInMillis(); | ||
| DateMathParser dateParser = dateMathParser; | ||
| if (forcedDateParser != null) { | ||
| dateParser = forcedDateParser; | ||
| } | ||
| long time = includeUpper && roundCeil ? dateParser.parseRoundCeil(value, now, zone) : dateParser.parse(value, now, zone); | ||
| return time; | ||
| boolean roundUp = inclusive && roundCeil; // TODO: what is roundCeil?? |
There was a problem hiding this comment.
This is a hard override (via index.mapping.date.round_ceil setting, which default to true) to disallow rounding up. Not sure if this actuall set to false, seems undesired to me.
There was a problem hiding this comment.
I will open a separate issue to deal with this odd setting.
|
LGTM |
|
LGTM^2 |
rjernst
added a commit
that referenced
this pull request
Nov 21, 2014
rjernst
added a commit
that referenced
this pull request
Nov 21, 2014
Date math rounding currently works by rounding the date up or down based on the scope of the rounding. For example, if you have the date `2009-12-24||/d` it will round down to the inclusive lower end `2009-12-24T00:00:00.000` and round up to the non-inclusive date `2009-12-25T00:00:00.000`. The range endpoint semantics work as follows: * `gt` - round D down, and use > that value * `gte` - round D down, and use >= that value * `lt` - round D down, and use < * `lte` - round D up, and use <= There are 2 problems with these semantics: * `lte` ends up including the upper value, which should be non-inclusive * `gt` only excludes the beginning of the date, not the entire rounding scope This change makes the range endpoint semantics symmetrical. First, it changes the parser to round up and down using the first (same as before) and last (1 ms less than before) values of the rounding scope. This makes both rounded endpoints inclusive. The range endpoint semantics are then as follows: * `gt` - round D up, and use > that value * `gte` - round D down, and use >= that value * `lt` - round D down, and use < that value * `lte` - round D up, and use <= that value closes #8424 closes #8556
rjernst
added a commit
that referenced
this pull request
Nov 21, 2014
rjernst
added a commit
that referenced
this pull request
Nov 21, 2014
rjernst
added a commit
to rjernst/elasticsearch
that referenced
this pull request
Dec 15, 2014
The setting `mapping.date.round_ceil` (and the undocumented setting `index.mapping.date.parse_upper_inclusive`) affect how date ranges using `lte` are parsed. In elastic#8556 the semantics of date rounding were solidified, eliminating the need to have different parsing functions whether the date is inclusive or exclusive. This change removes these legacy settings and improves the tests for the date math parser (now at 100% coverage!). It also removes the unnecessary function `DateMathParser.parseTimeZone` for which the existing `DateTimeZone.forID` handles all use cases. Any user previously using these settings can refer to the changed semantics and change their query accordingly. This is a breaking change because even dates without datemath previously used the different parsing functions depending on context. closes elastic#8598 closes elastic#8889
clintongormley
added
:Search/Search
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Date math rounding currently works by rounding the date up or down based
on the scope of the rounding. For example, if you have the date
2009-12-24||/dit will round down to the inclusive lower end2009-12-24T00:00:00.000and round up to the non-inclusive date2009-12-25T00:00:00.000.The range endpoint semantics work as follows:
gt- round D down, and use > that valuegte- round D down, and use >= that valuelt- round D down, and use <lte- round D up, and use <=There are 2 problems with these semantics:
lteends up including the upper value, which should be non-inclusivegtonly excludes the beginning of the date, not the entire rounding scopeThis change makes the range endpoint semantics symmetrical. First, it
changes the parser to round up and down using the first (same as before)
and last (1 ms less than before) values of the rounding scope. This
makes both rounded endpoints inclusive. The range endpoint semantics
are then as follows:
gt- round D up, and use > that valuegte- round D down, and use >= that valuelt- round D down, and use < that valuelte- round D up, and use <= that valuecloses #8424