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

Allows nanosecond resolution in search_after #60328

Merged
merged 4 commits into from
Jul 29, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jul 28, 2020

This fixes search_after to properly parse string formatted dates that
have nanosecond resolution.

Closes #52424

This fixes `search_after` to properly parse string formatted dates that
have nanosecond resolution.

Closes elastic#52424
@nik9000 nik9000 added >bug :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.10.0 labels Jul 28, 2020
@nik9000 nik9000 requested a review from jimczi July 28, 2020 18:07
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 28, 2020
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left one question, LGTM otherwise

- length: {hits.hits: 1 }
- match: {hits.hits.0._index: test }
- match: {hits.hits.0._source.timestamp: "2019-10-21 08:30:04.828733" }
- match: {hits.hits.0.sort: [1571646604828733000] }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use the formatted version in the sort output ? We don't allow this format (nanoseconds since epoch) when ingesting or querying a date_nanos field so that's an exception here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm honestly not sure! I kind of figured the contact of search_after was "take the last sort value and stick it into search_after" but I see that we support formatted.

I also wonder what counts as breaking here. If we were to change the result to be formatted I imagine we'll break someone.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do support formatted output so my point was that the sort section should return the formatted version in order to avoid returning long that represents nanoseconds since epoch. I agree that this is not in the scope of this change so let's merge this one and we can discuss the formatting of sort values in a follow up (I can open the issue) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that this is not in the scope of this change so let's merge this one and we can discuss the formatting of sort values in a follow up (I can open the issue) ?

👍

@nik9000 nik9000 merged commit 944a6c2 into elastic:master Jul 29, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Jul 29, 2020
This fixes `search_after` to properly parse string formatted dates that
have nanosecond resolution.

Closes elastic#52424
nik9000 added a commit that referenced this pull request Aug 3, 2020
…0426)

Allows nanosecond resolution in search_after (#60328)

This fixes `search_after` to properly parse string formatted dates that
have nanosecond resolution.

Closes #52424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

data_nanos based search_after query with custom timestamp format causes exception
4 participants