Skip to content

Disable sort optimization in search_after and scroll requests#78230

Merged
dnhatn merged 6 commits intoelastic:masterfrom
dnhatn:scroll-request-disable-sorts
Sep 24, 2021
Merged

Disable sort optimization in search_after and scroll requests#78230
dnhatn merged 6 commits intoelastic:masterfrom
dnhatn:scroll-request-disable-sorts

Conversation

@dnhatn
Copy link
Copy Markdown
Member

@dnhatn dnhatn commented Sep 23, 2021

We can't enable the sort optimization with points in scroll requests until LUCENE-10119 is integrated.

I labelled this PR non-issue for an unreleased bug.

@dnhatn dnhatn added >non-issue :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.16.0 labels Sep 23, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Sep 23, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Sep 23, 2021

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

jimczi
jimczi reviewed Sep 23, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Should we disable for all request with search_after and not only scroll ones ?

@dnhatn
Copy link
Copy Markdown
Member Author

dnhatn commented Sep 23, 2021

@jimczi I think search_after should be okay, but it's safer to disable them all now.

@dnhatn
Copy link
Copy Markdown
Member Author

dnhatn commented Sep 23, 2021

@jimczi I've disabled the optimization for search_after requests in 6d8ea42. Can you take another look? Thank you!

@dnhatn dnhatn requested a review from jimczi September 23, 2021 16:29
jimczi
jimczi approved these changes Sep 23, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM, thanks @dnhatn

@mayya-sharipova
Copy link
Copy Markdown
Contributor

mayya-sharipova commented Sep 23, 2021 •
edited
Loading

Elasticsearch search_after is not affected by this, and I think we don't need to disable it.

The Lucene bug was for cases:

  1. there is a single sort and
  2. the exact FieldDoc from the previous page is provided with the exact Lucene docID, as we do for scroll scrollContext.lastEmittedDoc

For a general elasticsearch with search_after we do SearchAfterBuilder::buildFieldDoc and we either

  1. don't know the exact docId field of FieldDoc
  2. or if a user is using _doc or _shard_doc with the main sort, there will not be a single sort

@dnhatn
Copy link
Copy Markdown
Member Author

dnhatn commented Sep 23, 2021

@mayya-sharipova I agree with your analysis. @jimczi Do you still prefer to disable the sort optimization in search_after requests?

@jimczi
Copy link
Copy Markdown
Contributor

jimczi commented Sep 23, 2021

or if a user is using _doc or _shard_doc with the main sort, there will not be a single sort

Not necessarily, _shard_doc can be used as the single sort in a PIT.

It's not really an issue anyway since we don't intend to release any version with this regression.

@mayya-sharipova
Copy link
Copy Markdown
Contributor

mayya-sharipova commented Sep 23, 2021 •
edited
Loading

Not necessarily, _shard_doc can be used as the single sort in a PIT.

I think sort optimization is only applied for long fields, and is not applicable if there is a single sort on _shard_doc and this sort does't use NumericComparator.

But I am also ok to disable sort optimization with search_after completely for now to be completely sure we don't access it wrongly somewhere else internally (_seq?)

mayya-sharipova
mayya-sharipova approved these changes Sep 23, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@dnhatn Thanks a lot.

@dnhatn
Copy link
Copy Markdown
Member Author

dnhatn commented Sep 24, 2021

@jimczi @mayya-sharipova Thanks for reviews.

@dnhatn dnhatn merged commit 5e9c242 into elastic:master Sep 24, 2021
@dnhatn dnhatn deleted the scroll-request-disable-sorts branch September 24, 2021 00:22
@dnhatn dnhatn changed the title Disable sort optimization with points in scroll requests Disable sort optimization in search_after and scroll requests Sep 24, 2021
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Sep 24, 2021
@dnhatn
Disable sort optimization in search_after and scroll requests (elasti…
a8c5d0b 
…c#78230)

We can't enable the sort optimization with points in scroll requests
until LUCENE-10119 is integrated.
This was referenced Sep 24, 2021
Merged
Merged
elasticsearchmachine pushed a commit that referenced this pull request Sep 24, 2021
@dnhatn
Disable sort optimization in search_after and scroll requests (#78230) (
5701490 
#78285)

We can't enable the sort optimization with points in scroll requests
until LUCENE-10119 is integrated.
dnhatn added a commit that referenced this pull request Sep 24, 2021
@dnhatn
Upgrade to Lucene9-snapshot-94c6e261274 (#78286)
7d82dbf 
Upgrade to a new Lucene 9 snapshot that includes LUCENE-10119 so we can 
re-enable the sort optimization with points for scroll and search_after
requests.

Relates #78230
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mayya-sharipova mayya-sharipova mayya-sharipova approved these changes

@jimczi jimczi jimczi approved these changes

Assignees

No one assigned

Labels

>non-issue :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.16.0 v8.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dnhatn @elasticmachine @mayya-sharipova @jimczi @jakelandis