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.

Sign up for GitHub

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

Jump to bottom

Fix range query on date fields for number inputs #63692

Merged
merged 17 commits into from
Dec 1, 2020
Merged

Fix range query on date fields for number inputs #63692

merged 17 commits into from
Dec 1, 2020

Conversation

cbuescher
Copy link
Member

Currently, if you write a date range query with numeric 'to' or 'from' bounds,
they can be interpreted as years if no format is provided. We use
"strict_date_optional_time||epoch_millis" in this case that can interpret inputs
like 1000 as the year 1000 for example. We should change this to always
interpret and parse numbers in this case with the second option "epoch_millis"
if no other formatter was provided.

Closes #63680

Fix range quers on date fields for number inputs
033cd0e 
Currently, if you write a date range query with numeric 'to' or 'from' bounds,
they can be interpreted as years if no format is provided. We use
"strict_date_optional_time||epoch_millis" in this case that can interpret inputs
like 1000 as the year 1000 for example. We should change this to always
interpret and parse numbers in this case with the second option "epoch_millis"
if no other formatter was provided.

Closes elastic#63680
@cbuescher cbuescher added >bug :Search/Search Search-related issues that do not fall into other categories v8.0.0 labels Oct 14, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Oct 14, 2020
@cbuescher
Copy link
Member Author

This is a potential fix, as @nik9000 mentioned on the issue we should discuss if we want this kind of change and, if so, should it be considered a breaking change in 8.0 or a bugfix. I tend to see this as a bug fix since the current behaviour is really trappy and very likely not expected by the user.

@cbuescher cbuescher changed the title Fix range quers on date fields for number inputs Fix range query on date fields for number inputs Oct 14, 2020
@cbuescher
Copy link
Member Author

@nik9000 thanks for the review, I added another iteration but personally prefered the earlier variant. let me know what you think. Maybe I missed some simpler option?

@nik9000
Copy link
Member

nik9000 commented Oct 22, 2020

This happens when you submit a doc too!

nik9000 added a commit that referenced this pull request Oct 22, 2020
@nik9000
Use better date in test
eb885fb 
Dates less than `10000` can be interpreted as years instead of millis
since epoch. Both in queries and when parsing dates. This makes sure
that one of our tests doesn't use these confusing dates.....

Closes #63969
Relates to #63692
nik9000 added a commit that referenced this pull request Oct 22, 2020
@nik9000
Use better date in test
2b0d4bc 
Dates less than `10000` can be interpreted as years instead of millis
since epoch. Both in queries and when parsing dates. This makes sure
that one of our tests doesn't use these confusing dates.....

Closes #63969
Relates to #63692
@cbuescher
Copy link
Member Author

@nik9000 just going through open PRs atm, do you think this needs changes or additions? Or just another round of discussion maybe?

nik9000
nik9000 approved these changes Nov 30, 2020
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Should we add something to the breaking changes list?

@cbuescher
Copy link
Member Author

@nik9000 thanks for the review, I added a blip to the docs and an entry in the breaking changes list. Can you take another short quick look to see if what I added there makes sense to you?

nik9000
nik9000 approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think 8.0.0-alpha1.asciidoc could also use a line for this.

@cbuescher cbuescher merged commit c327794 into elastic:master Dec 1, 2020
@jakelandis jakelandis mentioned this pull request Feb 22, 2021
Closed
@cbuescher cbuescher mentioned this pull request Jun 17, 2021
Closed
@masseyke masseyke mentioned this pull request Aug 9, 2021
Closed
80 tasks
@dnhatn dnhatn mentioned this pull request Sep 8, 2021
Merged
dnhatn added a commit that referenced this pull request Sep 10, 2021
@dnhatn
Use epoch_millis in can_match date range tests (#77464)
c6700d7 
There is a known issue in 7.x where a date parameter of a range query 
can be interpreted as the number of years if no format is provided.
This commit always uses epoch_millis format to avoid this issue in
CanMatchPreFilterSearchPhaseTests.

Relates #63692
Closes #77122
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Sep 13, 2021
@dnhatn
Use epoch_millis in can_match date range tests (elastic#77464)
cb123bb 
There is a known issue in 7.x where a date parameter of a range query 
can be interpreted as the number of years if no format is provided.
This commit always uses epoch_millis format to avoid this issue in
CanMatchPreFilterSearchPhaseTests.

Relates elastic#63692
Closes elastic#77122
@dnhatn dnhatn mentioned this pull request Sep 13, 2021
Merged
elasticsearchmachine pushed a commit that referenced this pull request Sep 13, 2021
@dnhatn
Use epoch_millis in can_match date range tests (#77464) (#77631)
6e9e283 
There is a known issue in 7.x where a date parameter of a range query 
can be interpreted as the number of years if no format is provided.
This commit always uses epoch_millis format to avoid this issue in
CanMatchPreFilterSearchPhaseTests.

Relates #63692
Closes #77122
@hkulekci hkulekci mentioned this pull request Feb 17, 2022
Merged
23 tasks
@RC-Lee RC-Lee mentioned this pull request Feb 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
>breaking >bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

range query on dates can see small numbers of millis as a year
4 participants
@cbuescher @elasticmachine @nik9000 @jakelandis