[BUG] ignore trigrams with null terminator byte when constructing full text index #4422
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.
Description of changes
Summarize the changes made by this PR.
The encoding/decoding method we use for packing a trigram into a u64 cannot handle null terminator bytes. E.x.
decode(encode(a\0b)) == a
. This resulted in index writers and the blockfile writer disagreeing on the ordering of mutations (index writers sorted witha\0b
, the blockfile writer ordered witha
).To work around this, any trigrams containing null terminator bytes are now ignored. An (arguably cleaner) alternative would be to replace null terminator bytes with something like a space instead.
Test plan
How are these changes tested?
Added test currently fails on main.
Ran the
full_text
benchmark to confirm that no performance regression is introduced.Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?
n/a