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

Add ES API compatible date time parsers #5461

Closed
wants to merge 1 commit into from

Conversation

kuzaxak
Copy link
Collaborator

@kuzaxak kuzaxak commented Sep 28, 2024

Description

ES uses own names for date formats. I implemented the ones that I was able to find in OSD.

Test cases taken from doc and OS test.

This PR is fixing issue and restore compatibility with OSD.

How was this PR tested?

Added unit tests to cover new parsers.

@kuzaxak kuzaxak force-pushed the issue/5460-es-api-date-format branch from a1eae0f to adb8152 Compare September 28, 2024 13:10
ES uses [own names][1] for date formats. I implemented the ones that I was able to find in OSD.

Test cases taken from [doc][1] and [OS test][2].

This PR is fixing [issue](#5460) and restore compatibility with OSD.

[1]: https://opensearch.org/docs/latest/field-types/supported-field-types/date/
[2]: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/test/java/org/opensearch/common/time/DateFormattersTests.java
@kuzaxak kuzaxak force-pushed the issue/5460-es-api-date-format branch from adb8152 to 113eaee Compare September 28, 2024 13:11
@etolbakov
Copy link
Collaborator

@kuzaxak you might be interested in checking this related PR. As far as I understand Paul’s idea was to enable the support using a regexset
#5248

Maybe we can finalize it
Cc @fulmicoton

@kuzaxak
Copy link
Collaborator Author

kuzaxak commented Sep 28, 2024

@kuzaxak you might be interested in checking this related PR. As far as I understand Paul’s idea was to enable the support using a regexset #5248

Maybe we can finalize it Cc @fulmicoton

Okay, I missed it. Do you have plans to finish it? Or maybe I can help you?
As of now, this issue is blocking my onboarding QW with OpenSearch Dashboards constantly sending strict_date_optional_time in a format field.

I can patch requests on the fly if implementation takes longer than a week.

@etolbakov

@kuzaxak
Copy link
Collaborator Author

kuzaxak commented Sep 28, 2024

Closing as #5248 should be a generic solution instead of a partial one presented in this PR.

@kuzaxak kuzaxak closed this Sep 28, 2024
@etolbakov
Copy link
Collaborator

etolbakov commented Sep 28, 2024

I was curious to work on that just for sake of learning, so don’t have any sticky timelines.
IIRC the latest status for that ticket: I was a bit stuck and needed Paul’s input.

If you can proceed/ take a lead on this PR I think it will be great!

@kuzaxak

@kuzaxak kuzaxak deleted the issue/5460-es-api-date-format branch September 28, 2024 13:26
@kuzaxak
Copy link
Collaborator Author

kuzaxak commented Sep 28, 2024

If you can proceed/ take a lead on this PR I think it will be great!

I can. What is left to be done there?

I see that you added some commented-out tests. Does it mean we need to implement Logix to make them work?

And I think strict_date_optional_time should be extended to accept only year as required param.

@etolbakov

@etolbakov
Copy link
Collaborator

I added a bit more tests for week patterns which should be ok.
The commented failing tests are a question mark - do we need them at all (I mean, if Quickwit team would like to support the functionality)
My initial idea was to port all "format"-related tests from here:
https://github.com/opensearch-project/OpenSearch/blob/main/server/src/test/java/org/opensearch/common/joda/JavaJodaTimeDuellingTests.java#L260
but maybe it’s an overkill.

I also see that "cargo nexttest" step is failing.
However, local execution of "clippy" is working fine, haven’t solved that.
@kuzaxak

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

Successfully merging this pull request may close these issues.

2 participants