-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Bug in JavaDateFormatter when using DOY #89096
Comments
Note that we tried a few other variants to "make it work":
|
I'll debug this a bit and figure out where it lands. |
yeah - it looks indeed like it's https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java#L48-L59. All those default looks wrong if you are using month of year. The reason FWIW, if I comment out those two bits of rounding it doesn't crash. Not sure if that's the fix, but it doesn't crash. Here's the test I'm running locally:
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
I've sent this to the core-infra folks who know this code much better than I do. This is specifically about the "round up" parser. |
@nik9000 do you know the conditions that cause ES to use the "round up" parser? It would help us communicate the extent of the issue to our users. Also, please note that this issue does not occur with ES6, presumably because it uses Joda time rather than Java time. |
Yeah. I imagine that's what it was. That was a big change and lots of stuff snuck in, unfortunately.
It looks like |
@nik9000 I am surprised this was not reported before. In my industry, DOY searches are quite common because we several systems that prefer to denote dates as DOY (Mars 2020, Deep Space Network, etc.). I assume that others have run into the issue but are simply converting DOY into MM/DD. Unfortunately, this is not a great option for us because we allow users to submit free-form queries, so we would have to parse and convert those queries. How/when could we expect to have a timeframe for a fix? |
Sorry about that! Day of year queries aren't something I'd use, but I'm not
in your industry. Anyway!
The tags I've put on this should cause an expert in this stuff to have a
look at it before long. Sometime this week would be normal. But it might
take longer, depending. Summer in the northern hemisphere has many folks
away from their keyboard. Even then, I'm not sure where it's fall on their
schedule. I hate breaking things, but given how long it's been like this
that implies a slightly smaller blast radius.
You can certainly have a look yourself. Like I said, I commented out some
code and everything seemed better. But I didn't run the test suite. I
really may have broken other things. Deleting code can do that....
It'd need to be reviewed by one of those experts. They'd know exactly how
good our tests are here.
After that there's actually cutting the release. We have a policy of never
telling folks when we will cut releases. Mostly it's because the dates
shift a fair bit and we don't want to promise.
…On Mon, Aug 8, 2022, 2:24 PM HayDegha0917 ***@***.***> wrote:
@nik9000 <https://github.com/nik9000> I am surprised this was not
reported before. In my industry, DOY searches are quite common because we
several systems that prefer to denote dates as DOY (Mars 2020, Deep Space
Network, etc.). I assume that others have run into the issue but are simply
converting DOY into MM/DD. Unfortunately, this is not a great option for us
because we allow users to submit free-form queries, so we would have to
parse and convert those queries.
How/when could we expect to have a timeframe for a fix?
—
Reply to this email directly, view it on GitHub
<#89096 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABUXIS3J4P63VPXP4KIIXLVYFGFRANCNFSM55QBN3RA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
it is the same family of bugs as #58986 it is because when parsing I think the conclusion is the same as in the linked issue. #58986 (comment) |
@pgomulka I don't pretend to understand the inner workings fo ES, but I don't understand why you don't delegate the job of parsing to the JDK and then apply whatever rounding algorithm you want to the outcome. In fact, you could easily parse to the internal representation then back to a standard representation and then apply your round-up logic ( |
@HayDegha0917 I did open an issue to the JDK regarding this https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8250514 but the problem comes down to ES not being able to know up front what format (and fields effectively) will be used. If we knew what format will be used, we could use the advice from the JDK issue. The reason this worked previously in 6.x was because joda had parseInto method, which from what I understand was a "predecessor" of parseDefaulting in java.time. Turns out it works differently in some cases. I am open for ideas and PRs. Do read the original issue |
@pgomulka I see the advice in the JDK issue and, from their perspective, it makes complete sense. Have you tried default all of the available fields of elasticsearch/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java Line 53 in be7c741
I also do not entirely understand why you are explicitly setting the default values. I have not programmed in Java for some time, so I am not sure what would happen if you simply omitted all the |
We did some additional investigation and found this additional issue. If you follow all the steps of the test case above and, instead of submitting the query in DSL format, you submit it as a query string, then Elasticsearch swallows the exception silently and returns incorrect results:
The query above returns
In fact, it should return 3 results. If you modify the query as follows:
then it returns 3 results:
Consequently, the user does not even have a notion that something went wrong. |
Hi @HayDegha0917, thanks for these additional instructions. We have a solution in mind and we are working on a fix that will handle this correctly. |
@grcevski good to hear! Thanks for directing attention to this issue. Please let us know when a patch is available (or, I suppose, GitHub will let us know). |
Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes elastic#89096 closes elastic#58986
Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes #89096 closes #58986
Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes elastic#89096 closes elastic#58986
Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfMonth, MonthOfYear, ClockHourOfAMPM and HourOfAmPM closes #89096 closes #58986 backports #89693
Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes elastic#89096 closes elastic#58986
Date rounding logic should take into account the fields that will be parsed be a parser. If a parser has a DayOfYear field, the rounding logic should not try to default DayOfMonth as it will conflict with DayOfYear However the DateTimeFormatter does not have a public method to return information of fields that will be parsed. The hacky workaround is to rely on toString() implementation that will return a field info when it was defined with textual pattern. This commits introduced conditional logic for DayOfYear, ClockHourOfAMPM and HourOfAmPM closes #89096 closes #58986 backports #89693
Elasticsearch Version
7.10.2
Installed Plugins
No response
Java Version
1.8
OS Version
CentOS
Problem Description
When defining a mapping that has a field that uses DOY format, data gets indexed correctly However, we run into issues when using the rounding parser. See steps below for a test case.
We believe the issue is related to this section of code:
https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java#L48-L59
Steps to Reproduce
All steps complete without error except for the range filter, which outputs:
If we omit the explicit format in the filter, we instead get
If we change
lte
tolt
in the filter, we get a valid response:If we use a custom format in the field mapping that does not use DOY, everything works fine.
Logs (if relevant)
No response
The text was updated successfully, but these errors were encountered: