Stop closing index input when loading NRTSuggester #14271
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cloning the index input is not necessary when loading the NRTSUggester. Also, it causes a bug as the clone gets closed right before the suggester instance gets returned, which makes it unusable in the later lookup method which slices the index input while it's already closed. This was not discovered so far due to a testing gap that is fixed by apache#14270
jpountz
approved these changes
Feb 21, 2025
| dictClone.seek(offset); | ||
| suggester = NRTSuggester.load(dictClone, fstLoadMode); | ||
| } | ||
| IndexInput indexInput = dictIn.slice("NRTSuggester", offset, dictIn.length() - offset); |
There was a problem hiding this comment.
This got me confused wrt the PR title since slices are technically clones. Maybe update the title to say "stop closing the clone" instead of "stop cloning"?
There was a problem hiding this comment.
Technically, we no longer call clone, that is what I meant :) but I see your point that we call it indirectly now.
javanna
changed the title
|
Thanks all! |
javanna
added a commit
that referenced
this pull request
Feb 21, 2025
javanna
added a commit
that referenced
this pull request
Feb 21, 2025
Cloning the index input is not necessary when loading the NRTSUggester. Also, it causes a bug as the clone gets closed right before the suggester instance gets returned, which makes it unusable in the later lookup method which slices the index input while it's already closed. This was not discovered so far due to a testing gap that is fixed by #14270
javanna
added a commit
that referenced
this pull request
Feb 21, 2025
Cloning the index input is not necessary when loading the NRTSUggester. Also, it causes a bug as the clone gets closed right before the suggester instance gets returned, which makes it unusable in the later lookup method which slices the index input while it's already closed. This was not discovered so far due to a testing gap that is fixed by #14270
javanna
added a commit
to javanna/lucene
that referenced
this pull request
Mar 17, 2025
All the existing completion postings format load their FSTs on-heap. It is possible to customize that behaviour by mainintaing a custom postings format that override the fst load mode. TestSuggestField attempted to test all modes, but in practice, it can only test the default load mode because the read path relies on the default constructor called via SPI, that does not allow providing the fst load mode. This commit changes the default value to OFF_HEAP, and addresses the testing gap in TestSuggestField, so that all load modes are tested. This unveiled a bug when loading FST off heap and the index input is a MemorySegmentIndexInput which is fixed by apache#14271.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closing the index input is not necessary when loading the NRTSuggester. Also, it causes a bug as the clone gets closed right before the suggester instance gets returned, which makes it unusable in the later lookup method which slices the index input while it's already closed. This is addressed by creating a slice, that is not further closed, so that the index input is still usable once the suggester is returned.
This was not discovered so far due to a testing gap that is fixed by #14270.
The bug was introduced by #13524 , stacktrace follows: