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

Improve scroll search by using IndexSearcher#searchAfter(...) #4968

Merged
merged 1 commit into from Mar 21, 2014

Conversation

martijnvg
Copy link
Member

PR for #4940

@@ -134,6 +136,78 @@ private static long optionalSum(long left, long right) {
return Math.min(left, right) == -1 ? -1 : left + right;
}

public ScoreDoc[] sortDocsForScroll(AtomicArray<? extends QuerySearchResultProvider> resultsArr) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

The body of this method is very similar to sortDocs, would it possible somehow to share more code with sortDocs?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not that similar. The sortDocs method need to take from into account all the time, while the sortDocsForScroll method never needs to.

@martijnvg
Copy link
Member Author

Updated PR that includes @jpountz and @spinscale feedback points.

if (lastEmittedDocPerShard[scoreDoc.shardIndex] == null) {
lastEmittedDocPerShard[scoreDoc.shardIndex] = scoreDoc;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be simpler by iterating forward as this would remove the need for a null check:

for (int i = 0; i < sortedShardList.length; ++i) {
    ScoreDoc scoreDoc = sortedShardList[i];
    lastEmittedDocPerShard[scoreDoc.shardIndex] = scoreDoc;
}

@martijnvg
Copy link
Member Author

@jpountz Your comments make sense, I updated the PR with another commit that addresses your comments.

this.docIds = list.buffer;
this.size = list.size();
this.lastEmittedDoc = lastEmittedDoc;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the other constructor call this one?

@jpountz
Copy link
Contributor

jpountz commented Mar 18, 2014

This pull request looks good in general:

  • I like the duel tests
  • The comparator logic for compareTop looks good to me (even the tricky ordinal-based comparator).

However, there are a few places where the useClassicScroll assignment looks weird (used before being set)? I'm also wondering if the old scroll mode should be given a more despicable name than classic? Otherwise it isn't clear that performance is much worse when this parameter is false?

@jpountz
Copy link
Contributor

jpountz commented Mar 20, 2014

LGTM

…of regular search methods which rely on `from` for pagination.

This prevents the creation of priority queues of `from + size`, instead the size of the priority queue will always be equal to `size`.

Closes elastic#4940
@martijnvg martijnvg merged commit 947c5f6 into elastic:master Mar 21, 2014
@martijnvg martijnvg deleted the improvements/search-after2 branch May 18, 2015 23:32
@clintongormley clintongormley added >enhancement v2.0.0-beta1 v1.2.0 :Search/Search Search-related issues that do not fall into other categories labels Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v1.2.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants