-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Allow format sort values of date fields #70357
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
Conversation
1093423
to
c1d7f75
Compare
Pinging @elastic/es-search (Team:Search) |
@jrodewig Would you mind taking a look at the doc changes? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @dnhatn!
I left a couple of non-blocking suggestions that you can ignore if wanted.
docs/reference/search/search-your-data/sort-search-results.asciidoc
Outdated
Show resolved
Hide resolved
docs/reference/search/search-your-data/paginate-search-results.asciidoc
Outdated
Show resolved
Hide resolved
…iidoc Co-authored-by: James Rodewig <[email protected]>
….asciidoc Co-authored-by: James Rodewig <[email protected]>
@jrodewig Thanks for the quick review. I've integrated your suggestions. Both of them are great 👍. |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment regarding the docs, LGTM otherwise.
docs/reference/search/search-your-data/paginate-search-results.asciidoc
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanded docs LGTM. Thanks @dnhatn.
|
||
resp = client().prepareSearch("test") | ||
.addSort(SortBuilders.fieldSort("start_date").setFormat("dd/MM/yyyy")) | ||
.addSort(SortBuilders.fieldSort("end_date")) // it's okay as we use the default format, but the output does not format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a requirement that searchAfter formats and formats in Sort should match?
Or it is ok because "yyyy-MM-dd"
is a default formant for end_date
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or it is ok because "yyyy-MM-dd" is a default formant for end_date?
This is okay because end_date
has the format yyyy-MM-dd
. I reworded this comment in 9c537d5.
Isn't there a requirement that searchAfter formats and formats in Sort should match?
The formats (specified by sort) will try to parse the search_after parameters. If the parameters do not conform, then we will fail the request as a bad request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the clarifications, Nhat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dnhatn Thanks Nhat, excellent work! All the changes LGTM!
Thanks everyone :). |
If a search after request targets multiple indices and some of its sort field has type `date` in one index but `date_nanos` in other indices, then Elasticsearch won't interpret the search_after parameter correctly in every target index. The sort value of a date field by default is a long of milliseconds since the epoch while a date_nanos field is a long of nanoseconds. This commit introduces the `format` parameter in the sort field so a sort value of a date or date_nanos will be formatted using a date format in a search response. The below example illustrates how to use this new parameter. ```js { "query": { "match_all": {} }, "sort": [ { "timestamp": { "order": "asc", "format": "strict_date_optional_time_nanos" } } ] } ``` ```js { "query": { "match_all": {} }, "sort": [ { "timestamp": { "order": "asc", "format": "strict_date_optional_time_nanos" } } ], "search_after": [ "2015-01-01T12:10:30.123456789Z" // in `strict_date_optional_time_nanos` format ] } ``` Closes elastic#69192
If a search after request targets multiple indices and some of its sort field has type `date` in one index but `date_nanos` in other indices, then Elasticsearch won't interpret the search_after parameter correctly in every target index. The sort value of a date field by default is a long of milliseconds since the epoch while a date_nanos field is a long of nanoseconds. This commit introduces the `format` parameter in the sort field so a sort value of a date or date_nanos will be formatted using a date format in a search response. The below example illustrates how to use this new parameter. ```js { "query": { "match_all": {} }, "sort": [ { "timestamp": { "order": "asc", "format": "strict_date_optional_time_nanos" } } ] } ``` ```js { "query": { "match_all": {} }, "sort": [ { "timestamp": { "order": "asc", "format": "strict_date_optional_time_nanos" } } ], "search_after": [ "2015-01-01T12:10:30.123456789Z" // in `strict_date_optional_time_nanos` format ] } ``` Closes #69192
If a search after request targets multiple indices and some of its sort field has type
date
in one index butdate_nanos
in other indices, then Elasticsearch won't interpret the search_after parameter correctly in every target index. The sort value of a date field by default is a long of milliseconds since the epoch while a date_nanos field is a long of nanoseconds.This commit introduces the
format
parameter in the sort field so a sort value of a date or date_nanos will be formatted using a date format in a search response.The below example illustrates how to use this new parameter.
Closes #69192