Skip to content

Conversation

@dnhatn
Copy link
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
Collaborator

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

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.

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

@dnhatn
Copy link
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
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
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.

LGTM, thanks @dnhatn

@mayya-sharipova
Copy link
Contributor

mayya-sharipova commented Sep 23, 2021

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
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
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
Contributor

mayya-sharipova commented Sep 23, 2021

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?)

Copy link
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
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
…c#78230)

We can't enable the sort optimization with points in scroll requests
until LUCENE-10119 is integrated.
elasticsearchmachine pushed a commit that referenced this pull request Sep 24, 2021
#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
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

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

Development

Successfully merging this pull request may close these issues.

5 participants