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 efficiency of BoundedBreakIteratorScanner fragmentation algorithm #89041

Merged
merged 7 commits into from
Aug 3, 2022

Conversation

thelink2012
Copy link
Contributor

This is PR #75306 (a fix for PR #73785) but in an user-repository such that Allow edits by maintainers is possible. As follows is a description of the original PR. See those other PRs for previous discussions.

As discussed in #73569 the current implementation is too slow in certain scenarios.

The inefficient part of the code can be stated as the following problem:

Given a text (getText()) and a position in this text (offset), find the sentence boundary before and after the offset, in such a way that the after boundary is maximal but respects end boundary - start boundary < fragment size.

In case it's impossible to produce an after boundary that respects the said condition, use the nearest boundary following offset.

The current approach begins by finding the nearest preceding and following boundaries, and expands the following boundary greedily while it respects the problem restriction. This is fine asymptotically, but BreakIterator which is used to find each boundary is sometimes expensive [Lucene noticed too].

The new approach maximizes the after boundary by scanning for the last boundary preceding the position that would cause the condition to be violated (i.e. knowing start boundary and offset, how many characters are left before resulting length is fragment size). If this scan finds the start boundary, it means it's impossible to satisfy the problem restriction, and we get the first boundary following offset instead (or better, since we already scanned [offset, targetEndOffset], start from targetEndOffset + 1).

In theory, both approaches should produce exactly the same outputs given the same (text, offset, fragment_size) tuple. But it doesn't. As far as my investigation went, BreakIterator doesn't seem to be commutative. Previous method calls affects other calls. So I guess whatever change we make to this algorithm, may produce results that differ from each other. It is rare, but happens. Would love to be wrong on this though.

Highlighting related unit and integration tests pass. Though I'm not passing in tests that seem completely unrelated to this. See below.

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.5.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 2, 2022
@romseygeek romseygeek self-assigned this Aug 2, 2022
@romseygeek romseygeek added >enhancement :Search Relevance/Highlighting How a query matched a document and removed needs:triage Requires assignment of a team area label labels Aug 2, 2022
@romseygeek
Copy link
Contributor

@elasticmachine ok to test

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Aug 2, 2022
@romseygeek
Copy link
Contributor

@elasticmachine generate changelog

@romseygeek
Copy link
Contributor

@elasticmachine update branch

@pugnascotia pugnascotia assigned pugnascotia and unassigned romseygeek Aug 3, 2022
@pugnascotia
Copy link
Contributor

@elasticmachine generate changelog

@pugnascotia pugnascotia assigned romseygeek and unassigned pugnascotia Aug 3, 2022
Copy link
Contributor

@romseygeek romseygeek 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 @thelink2012

@romseygeek romseygeek merged commit 6bf5078 into elastic:main Aug 3, 2022
@thelink2012
Copy link
Contributor Author

Hey @romseygeek. Thanks for the merge. Would it be possible for this patch to be backported to previous versions (at least 8.4)?

@javanna
Copy link
Member

javanna commented Aug 9, 2022

heya @thelink2012 we only backport bug-fixes to patch releases, and this does not qualify as one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team :Search Relevance/Highlighting How a query matched a document Team:Search Meta label for search team v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants