I think we should be consistent about this completely and not override any methods with default implementations in these Filter* classes.

So that means stuff like FilterTermsEnum.seekExact too. We should review all these classes in FilterAtomicReader.

This makes sense. I found another bug in SortingAtomicReader which doesn't override getCoreCacheKey, this could lead to very bad things if an atomic reader and its sorted view were both used with the same FieldCache instance.

I've started looking at methods that override default impls and would like to have your opinion on some of them:

• shouldn't IndexReader.hasDeletions return numDeletedDocs() > 0 by default instead of being abstract?
• isn't the default impl of TermsEnum.termState dangerous? Shouldn't it throw an UnsupportedOperationException or being abstract instead?

shouldn't IndexReader.hasDeletions return numDeletedDocs() > 0 by default instead of being abstract?

I fell into this trap myself before (naively providing a fake livedocs to a filterreader), there are quite a few traps and inconsistencies:

• if you override the livedocs you currently need to override a bunch of other things too (e.g. hasDeletions, numDocs). we should at least add a javadoc to FilterAtomicReader make it less trappy.
• hasDeletions is abstract, and its easy to forget to implement this.
• SegmentReader says that for hasDeletions/numDocs/maxDoc "Don't call ensureOpen() here (it could affect performance)". But FilterAtomicReader only says this for numDocs and maxDoc and calls it on hasDeletions!
• default implementation in SegmentReader uses liveDocs != null: but i dont know anything checking (e.g. CheckIndex should do this) that the codec's LiveDocsFormat must return null (vs a Bits matching all docs). We should check this if there is code actually making this (undocumented) assumption.

For SegmentReader at least, it seems to be better if it had this implementation: and neither numDocs() and maxDocs() calls ensureOpen() either so I only see it as a safer way to go (we could avoid the sketchy liveDocs != null, but i bet other code elsewhere relies on this too).

isn't the default impl of TermsEnum.termState dangerous? Shouldn't it throw an UnsupportedOperationException or being abstract instead?

I don't know whats going on here: but I agree. it seems the current default implementation is "silently wont work".

Although DocIdSetIterator.advance is abstract, it describes a default implementation that many classes that extend DocsEnum/DocsAndPositionsEnum duplicate. Maybe we should just provide a default implementation for advance, this would save copy-pastes.

In this case I think its a little scary to have a default very-slow implementation. Similar to concerns raised on LUCENE-1592

Patch

• Implements IndexReader.hasDeletions() as numDeletedDocs() > 0 by default (instead of abstract) with no ensureOpen check (SegmentReader uses this default impl instead of liveDocs != null).

• Adds warning in FilterAtomicReader about overriding

• Methods that provide a default implementation are not implemented as in.XXX in Filter*** anymore (in particular Terms.intersect, IndexReader.hasDeletions and IndexReader.get*CacheKey). The test in TestFilterAtomicReader has been modified to ensure this.

• TermsEnum.termState returns a TermState that throws an UOE instead of failing silently

• DocIdSetIterator provides a protected final slowAdvance(int target) method, which advances linearly and can be used in implementations that don't require advance to be fast (and has an assert to make sure that target is greater than the current position).

i think attributes() should be overridden by the Filter** by default. This is basically a getter.

Updated patch. I added back attributes modified the test (+ comments) and added a changelog entry.

+1, thanks for working on these API issues!

[trunk commit] jpountz
http://svn.apache.org/viewvc?view=revision&revision=1464710

LUCENE-4874: Don't override non abstract methods that have an impl through
other abstract methods in FilterAtomicReader and related classes.

[branch_4x commit] jpountz
http://svn.apache.org/viewvc?view=revision&revision=1464776

LUCENE-4874: Don't override non abstract methods that have an impl through
other abstract methods in FilterAtomicReader and related classes (merged from
r1464710 and r1464768).

Thanks for helping Robert!