Skip to content
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

DateMath: Fix semantics of rounding with inclusive/exclusive ranges. #8556

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@rjernst
Copy link
Member

commented Nov 19, 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

DateMath: Fix semantics of rounding with inclusive/exclusive ranges.
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 #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??

This comment has been minimized.

Copy link
@martijnvg

martijnvg Nov 20, 2014

Member

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.

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 20, 2014

Author Member

I will open a separate issue to deal with this odd setting.

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 21, 2014

Contributor

+1

@martijnvg

This comment has been minimized.

Copy link
Member

commented Nov 20, 2014

LGTM

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2014

LGTM^2

@rjernst rjernst closed this in fae9dca Nov 21, 2014

@rjernst rjernst removed the review label Nov 21, 2014

rjernst added a commit that referenced this pull request Nov 21, 2014

rjernst added a commit that referenced this pull request Nov 21, 2014

DateMath: Fix semantics of rounding with inclusive/exclusive ranges.
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

Settings: Remove `mapping.date.round_ceil` setting for date math parsing
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

@rjernst rjernst deleted the rjernst:fix/8424 branch Jan 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.