Merge DocsEnum and DocsAndPositionsEnum into PostingsEnum [LUCENE-4524]

asfimport commented 12 years ago

spinnoff from http://www.gossamer-threads.com/lists/lucene/java-dev/172261

hey folks, 

I have spend a hell lot of time on the positions branch to make 
positions and offsets working on all queries if needed. The one thing 
that bugged me the most is the distinction between DocsEnum and 
DocsAndPositionsEnum. Really when you look at it closer DocsEnum is a 
DocsAndFreqsEnum and if we omit Freqs we should return a DocIdSetIter. 
Same is true for 
DocsAndPostionsAndPayloadsAndOffsets*YourFancyFeatureHere*Enum. I 
don't really see the benefits from this. We should rather make the 
interface simple and call it something like PostingsEnum where you 
have to specify flags on the TermsIterator and if we can't provide the 
sufficient enum we throw an exception? 
I just want to bring up the idea here since it might simplify a lot 
for users as well for us when improving our positions / offset etc. 
support. 

thoughts? Ideas? 

simon

Migrated from LUCENE-4524 by Simon Willnauer (@s1monw), 2 votes, resolved Feb 06 2015 Attachments: LUCENE-4524.patch (versions: 6) Linked issues:

3952

asfimport commented 11 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

here is an initial patch that moves this over. I really just did some initial porting and this patch has still some problems.

I removed DocsAndPosEnum entirely and changed how the DocsEnum Flags work such that we only have TermsEnum#docs and a simple sugar method for docsAndPos which should go away IMO. We need to figure out what kind of behavior those flags should trigger ie. if we have no freqs we still return and enum while no pos we return null.

anyway, most of the patch is rename etc. all test pass, comments welcome

asfimport commented 11 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

new patch bringing back TermsEnum#docsAndPositions(...) this make this entire thing way simpler and I think this is how it should be. All tests pass and I think this is pretty close already.

asfimport commented 11 years ago

Alan Woodward (@romseygeek) (migrated from JIRA)

The javadocs on PostingsReaderBase are a bit funky, but otherwise this looks great. Simplifies things a lot.

asfimport commented 11 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

alan, I agree this might make a lot of things simple in #3952 like passing instances down to the actual scorer. This might even help us to get the API straight.

I will go over the patch tomorrow again and straight out javadocs etc. I plan to commit this to trunk and then backport to 4.2 I think this this case we should really break BW compat and just go ahead and remove the DocsAndPositionsEnum class entirely. Any objections?

asfimport commented 11 years ago

Robert Muir (@rmuir) (migrated from JIRA)

I agree its a great! change, but perhaps this should be committed to the 2878 branch for now to see how it plays in practice?

Otherwise I have a few concerns about committing to trunk (none of which have to do with the particular patch, just problems in general)

Scorer.java will be strange, as it will have various methods like nextPosition(), getPayload(), that do not work. Of course this API would be fantastic and make total sense if this was the api for #3952, but in the meantime it would be odd and confusing.
I'm not sure how i feel about nextPosition(), getPayload(), start/endOffset() all going from abstract to 'default nonfunctioning impl'. It might be the right tradeoff, but i definitely liked them better as abstract. Its hard to tell without seeing how the Scorer integration would ultimately play out.
with DocsEnum and DocsAndPositionsEnum merged, the possibility of reuse bugs in various postings implementations is increased. We should review our own code here...

asfimport commented 11 years ago

Simon Willnauer (@s1monw) (migrated from JIRA)

this change is really unrelated to #3952 it removes a unnecessary duplication. The fact that Scorer extends DocEnum is not concerning me here since the main purpose of this class is not the Scorer API. This should really go on trunk since given the discussion on the list this is independent of #3952. If there are bugs in reuse we should catch them in the tests no? I mean we can add even more tests for this particular problem so we catch them quicker. I would be ok with makeing them abstract but this really is not a big deal here. I would want to move forward here quickly on trunk at least we can merge later into 4.x if needed since this might go out of date quickly.

asfimport commented 11 years ago

Steven Rowe (@sarowe) (migrated from JIRA)

Bulk move 4.4 issues to 4.5 and 5.0

asfimport commented 10 years ago

Uwe Schindler (@uschindler) (migrated from JIRA)

Move issue to Lucene 4.9.