Merge PostingsEnum and ImpactsEnum. #14716
Conversation
When impacts were introduced, I tried to avoid touching `PostingsEnum`, which had a relatively stable API while impacts were rather experimental. Impacts have been quite successful, so I think it's now time to make things simpler by adding support to impacts directly into `PostingsEnum`? More specifically: - `PostingsEnum` now has `advanceShallow` and `getImpacts` methods. - You can request impacts on a `PostingsEnum` by using the `IMPACTS` flag. - When the `IMPACTS` flag is not set, `PostingsEnum` may return dummy impacts (as for term frequencies when not requested).
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the PR. |
| */ | ||
| public abstract ImpactsEnum impacts(int flags) throws IOException; | ||
| @Deprecated |
There was a problem hiding this comment.
Should we remove this and deprecate in 10x?
There was a problem hiding this comment.
Yes! I was hoping to remove in a subsequent PR.
| * Flag to pass to {@link TermsEnum#postings(PostingsEnum, int)} to get impacts in the returned | ||
| * enum. | ||
| */ | ||
| public static final short IMPACTS = FREQS | 1 << 7; |
There was a problem hiding this comment.
Just wild thought: can we make impacts work on PostingEnum.FREQ instead introducing this flag? We can delay the reading of impact bytes (where is needsImpacts mostly used today) to getImpacts, so that it won't cause any noticeable overhead.
There was a problem hiding this comment.
Interesting idea. Let me check the performance impact.
There was a problem hiding this comment.
After a few iterations on wikibigall, it seems to work fine with the current codec. However it looks challenging to apply correctly to old codecs, which use a different PostingsEnum impl when impacts are requested, and that I'd like to avoid touching too much.
There was a problem hiding this comment.
I imagine that having a flag also has some benefits, e.g. some impls could decide to store impacts in a separate file in the future and only open this file if impacts are requested.
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog-check label to it and you will stop receiving this reminder on future updates to the PR. |
|
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! |
When impacts were introduced, I tried to avoid touching
PostingsEnum, which had a relatively stable API while impacts were rather experimental. Impacts have been quite successful, so I think it's now time to make things simpler by adding support to impacts directly intoPostingsEnum?More specifically:
PostingsEnumnow hasadvanceShallowandgetImpactsmethods.PostingsEnumby using theIMPACTSflag.IMPACTSflag is not set,PostingsEnummay return dummy impacts (as for term frequencies when not requested).