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

Remove mapping.date.round_ceil setting for date math parsing #8889

Merged
merged 1 commit into from Dec 15, 2014

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Dec 11, 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 #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. The one caveat
is handling malformed H:MM values, but this is a recently added
feature, and I don't think we should have our own special format here,
but instead enforce the standard [+-]HH:MM.

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 #8598

@rjernst
Copy link
Member Author

rjernst commented Dec 11, 2014

Note that there are a bunch of integration tests scattered around that are testing date math. Given the much better unit tests here now, I would like to remove the majority of them, leaving only basic checks of date math integration, but leaving extensive testing for the format to the unit tests. However, I do not yet have that change here, but wanted to first get a feel for opinions on their removal.

@rjernst rjernst added :Core/Infra/Settings Settings infrastructure and APIs v2.0.0-beta1 review labels Dec 11, 2014
@jpountz
Copy link
Contributor

jpountz commented Dec 11, 2014

The change looks good to me. Big +1 on removing all the scattered date-math tests and focusing on having good unit tests in only one or two test suites.

@brwe brwe added the >breaking label Dec 12, 2014
@rjernst
Copy link
Member Author

rjernst commented Dec 13, 2014

@jpountz I've removed some tests. Let me know if you think I went to far...or not far enough.

@jpountz
Copy link
Contributor

jpountz commented Dec 15, 2014

@rjernst LGTM

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 merged commit 3728728 into elastic:master Dec 15, 2014
@rjernst rjernst deleted the fix/8598 branch January 21, 2015 23:22
@clintongormley clintongormley changed the title Settings: Remove mapping.date.round_ceil setting for date math parsing Remove mapping.date.round_ceil setting for date math parsing Jun 6, 2015
jpountz added a commit to jpountz/elasticsearch that referenced this pull request Oct 7, 2016
Elasticsearch 1.x used to implicitly round up upper bounds of queries when they
were inclusive so that eg. `[2016-09-18 TO 2016-09-20]` would actually run
`[2016-09-18T00:00:00.000Z TO 2016-09-20T23:59:59.999Z]` and include dates like
`2016-09-20T15:32:44`. This behaviour was lost in the cleanups of elastic#8889.

Closes elastic#20579
jpountz added a commit that referenced this pull request Oct 7, 2016
Elasticsearch 1.x used to implicitly round up upper bounds of queries when they
were inclusive so that eg. `[2016-09-18 TO 2016-09-20]` would actually run
`[2016-09-18T00:00:00.000Z TO 2016-09-20T23:59:59.999Z]` and include dates like
`2016-09-20T15:32:44`. This behaviour was lost in the cleanups of #8889.

Closes #20579
jpountz added a commit that referenced this pull request Oct 7, 2016
Elasticsearch 1.x used to implicitly round up upper bounds of queries when they
were inclusive so that eg. `[2016-09-18 TO 2016-09-20]` would actually run
`[2016-09-18T00:00:00.000Z TO 2016-09-20T23:59:59.999Z]` and include dates like
`2016-09-20T15:32:44`. This behaviour was lost in the cleanups of #8889.

Closes #20579
jpountz added a commit that referenced this pull request Oct 7, 2016
Elasticsearch 1.x used to implicitly round up upper bounds of queries when they
were inclusive so that eg. `[2016-09-18 TO 2016-09-20]` would actually run
`[2016-09-18T00:00:00.000Z TO 2016-09-20T23:59:59.999Z]` and include dates like
`2016-09-20T15:32:44`. This behaviour was lost in the cleanups of #8889.

Closes #20579
jpountz added a commit that referenced this pull request Oct 7, 2016
Elasticsearch 1.x used to implicitly round up upper bounds of queries when they
were inclusive so that eg. `[2016-09-18 TO 2016-09-20]` would actually run
`[2016-09-18T00:00:00.000Z TO 2016-09-20T23:59:59.999Z]` and include dates like
`2016-09-20T15:32:44`. This behaviour was lost in the cleanups of #8889.

Closes #20579
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove legacy settings for restricting date rounding
4 participants