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

Rounded date ranges with lte should set include_upper to false #8424

Closed
clintongormley opened this issue Nov 10, 2014 · 0 comments
Closed

Rounded date ranges with lte should set include_upper to false #8424

clintongormley opened this issue Nov 10, 2014 · 0 comments

Comments

@clintongormley
Copy link

As discussed in #7203 the current behaviour of lt/lte with date rounding is inconsistent. For instance, with the following docs indexed:

DELETE /t

POST /t/t/_bulk
{"index":{"_id":1}}
{"date":"2014/11/07 00:00:00"}
{"index":{"_id":2}}
{"date":"2014/11/07 01:00:00"}
{"index":{"_id":3}}
{"date":"2014/11/08 00:00:00"}
{"index":{"_id":4}}
{"date":"2014/11/08 01:00:00"}
{"index":{"_id":5}}
{"date":"2014/11/09 00:00:00"}
{"index":{"_id":6}}
{"date":"2014/11/09 01:00:00"}

This query with lt:

GET /_search?sort=date
{
  "query": {
    "range": {
      "date": {
        "lt": "2014/11/08||/d"
      }
    }
  }
}

correctly returns 2014/11/07 00:00:00 and 2014/11/07 01:00:00, but this query with lte:

GET /_search?sort=date
{
  "query": {
    "range": {
      "date": {
        "lte": "2014/11/08||/d"
      }
    }
  }
}

incorrectly returns:

  • 2014/11/07 00:00:00
  • 2014/11/07 01:00:00
  • 2014/11/08 00:00:00
  • 2014/11/08 01:00:00
  • 2014/11/09 00:00:00

It should not include that last document. The lte parameter, when used with date rounding, should use ceil() to round the date up, as it does today, but include_upper should be set to false.

@clintongormley clintongormley added the good first issue low hanging fruit label Nov 14, 2014
@rjernst rjernst self-assigned this Nov 18, 2014
rjernst added a commit to rjernst/elasticsearch that referenced this issue 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:
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
@rjernst rjernst removed the help wanted adoptme label Nov 19, 2014
@jpountz jpountz added the v1.3.6 label Nov 21, 2014
rjernst added a commit that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants