[Fix] Binary search the entries when all suffixes have the same length in a leaf block. - #11888
Conversation
…s(NOT_FOUND, END).
…nd greater term exists.
|
@jpountz I have fixed the code and passed the tests, please have a review. |
|
Thanks @vsop-479. Do you know if the test you added to terms can be improved in such a way that it would have caught this bug? |
|
@jpountz I added a test case for this bug, but it only works for the tests with PostingsFormat which's DEFAULT_MIN_BLOCK_SIZE is 25, add build the first block with 25 entries, such as TestLucene90PostingsFormat. |
… block to TestLucene90PostingsFormat.
|
@jpountz I moved the test case for this bug to TestLucene90PostingsFormat. Please have a review. |
|
Thanks for looking into a test for this. @mikemccand Would you have time to have a look at this? I knew it was complex, but even more than I had anticipated, so I would appreciate another look to get a sense of whether there could be another bug lurking? |
|
@jpountz Is there any progress on this PR? |
Update from main.
|
Append some performance data. Note that the results have quite diversity from different rounds. round1round2round3 |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
@mikemccand I could use your help to review this change, it's quite deep in the guts of block tree. |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
|
@jpountz But it won't pass if this DefaultPostingsFormat do not use I also tried to set DefaultCodec to test target greater than the last entry of matched block case, like this:
Do you have any idea about this? |
|
I like this idea! It seems like it'd especially help primary key lookup against fixed length IDs like UUID? Hmm, the QPS in the |
mikemccand
left a comment
There was a problem hiding this comment.
Thank you for tackling this tricky optimization @vsop-479!
I need some more time to stare at the binarySearchTermLeaf but not until later ... so I'm publishing first comments :)
No, this was on |
|
@mikemccand Thanks for your review. iter1iter2iter3 |
mikemccand
left a comment
There was a problem hiding this comment.
I added a few more comments -- still need to find some more time to stare at the new binary search some more. Thanks @vsop-479!
|
Thanks for your comments @mikemccand . I have fixed them, and also removed the stale change entry about this change. |
mikemccand
left a comment
There was a problem hiding this comment.
OK this looks good to me -- the PKLookup gains look compelling and significant -- exciting!
I left one tiny grammar comment about javadoc/comment.
… the same length in a leaf block. (#11888) * Binary search the entries when all suffixes have the same length in a leaf block. * add comment on allEqual. * BackwardsCompatibility: keep the same logic on fillTerm and SeekStatus(NOT_FOUND, END). * Update comments: modify scan to binary search. * Add unit test for binarySearchTermLeaf. * Format code. * Assert the value of termsEnum.term() is correct after seeking. * Add CHANGES entry. * Clarify "leaf block _of the terms dict_" * Set suffixesReader's position. * Advance to the greater term If binary search ended at the less term. * Assert termsEnum's position after seeking. * Tidy. * Advance to the greater term if binary search ended at the less term: nextEnt plus 1. * Advance to the greater term if binary search ended at the less term and greater term exists. * Add test case: target greater than the last entry of the matched block. * Move test case that target greater than the last entry of the matched block to TestLucene90PostingsFormat. * Move test case for target greater than the last entry of the matched block to TestLucene99PostingsFormat * Clarify code. * Replace ternary with verbose if. * Replace seekExact with seekCeil. * Replace division by 2 with logical right shift. * Remove assert ste.termExists. * Clarify code. * Remove stale change entry. * Fix comment. --------- Co-authored-by: Adrien Grand <jpountz@gmail.com>
|
Oooh this change gave a nice pop (~5.4%, ~915 -> 964 K lookups/sec) to the primary key lookup nightly benchy: https://home.apache.org/~mikemccand/lucenebench/PKLookup.html I'll add an annotation, exciting! |
|
Glad to know that. Thanks @mikemccand . |
Fix #11722 's bug.