Lucene - Core
  1. Lucene - Core
  2. LUCENE-3620

FilterIndexReader does not override all of IndexReader methods

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      FilterIndexReader does not override all of IndexReader methods. We've hit an error in LUCENE-3573 (and fixed it). So I thought to write a simple test which asserts that FIR overrides all methods of IR (and we can filter our methods that we don't think that it should override). The test is very simple (attached), and it currently fails over these methods:

      getRefCount
      incRef
      tryIncRef
      decRef
      reopen
      reopen
      reopen
      reopen
      clone
      numDeletedDocs
      document
      setNorm
      setNorm
      termPositions
      deleteDocument
      deleteDocuments
      undeleteAll
      getIndexCommit
      getUniqueTermCount
      getTermInfosIndexDivisor
      

      I didn't yet fix anything in FIR – if you spot a method that you think we should not override and delegate, please comment.

      1. LUCENE-3620.patch
        15 kB
        Shai Erera
      2. LUCENE-3620.patch
        3 kB
        Shai Erera
      3. LUCENE-3620.patch
        2 kB
        Shai Erera
      4. LUCENE-3620-trunk.patch
        2 kB
        Shai Erera

        Issue Links

          Activity

          Hide
          Shai Erera added a comment -

          Attached patch against 3x adds the test to TestFilterIndexReader.

          Even if there are methods which you don't think need to be overridden by FIR, I prefer that we still override them and call super.(), with a comment why we don't delegate.

          Show
          Shai Erera added a comment - Attached patch against 3x adds the test to TestFilterIndexReader. Even if there are methods which you don't think need to be overridden by FIR, I prefer that we still override them and call super.(), with a comment why we don't delegate.
          Hide
          Uwe Schindler added a comment -

          Hi Shai, thats wanted that e.g. reopen/clone are not delegated. Because reopen/openIfChanged must return a new instance of the current IndexReader (which extends FilterIndexReader). By simply delegating it would violate this.

          Just a comment to your previous mail: If you call super.reopen() you would confuse the backwards layer (using VirtualMethod) for the transition to doOpenIfChanged, so please don't do it. Just leave it unimplemented.

          I would extends the test to add the "wanted" methods in a HashSet so it will not check them.

          • reopen(), doOpenIfChanged(), clone(): see above
          • numDeletedDocs should be final in IndexReader
          • deleteDocument & similar may be delegated (but i am not sure)
          Show
          Uwe Schindler added a comment - Hi Shai, thats wanted that e.g. reopen/clone are not delegated. Because reopen/openIfChanged must return a new instance of the current IndexReader (which extends FilterIndexReader). By simply delegating it would violate this. Just a comment to your previous mail: If you call super.reopen() you would confuse the backwards layer (using VirtualMethod) for the transition to doOpenIfChanged, so please don't do it. Just leave it unimplemented. I would extends the test to add the "wanted" methods in a HashSet so it will not check them. reopen(), doOpenIfChanged(), clone(): see above numDeletedDocs should be final in IndexReader deleteDocument & similar may be delegated (but i am not sure)
          Hide
          Shai Erera added a comment -

          Then why don't we make FilterIndexReader abstract class, and declare reopen/doOpenIfChanged/clone abstract? I don't think it's useful to just init FilterIndexReader right? And clearly those methods need to be overridden by sub-classes, or they would already receive UnsupportedOperationException no?

          numDeletedDocs should be final in IndexReader

          Ok. Is it an acceptable backwards-break?

          I will post a patch which updates FIR and the test.

          Show
          Shai Erera added a comment - Then why don't we make FilterIndexReader abstract class, and declare reopen/doOpenIfChanged/clone abstract? I don't think it's useful to just init FilterIndexReader right? And clearly those methods need to be overridden by sub-classes, or they would already receive UnsupportedOperationException no? numDeletedDocs should be final in IndexReader Ok. Is it an acceptable backwards-break? I will post a patch which updates FIR and the test.
          Hide
          Shai Erera added a comment -

          How about making those methods final on IR:
          getRefCount
          incRef
          tryIncRef
          decRef
          They reference refCount, and I cannot think of an IR extension that would want to override any of these methods?

          numDeletedDocs – should be made final, per Uwe's proposal.

          These methods, IMO, can be made final as well, because they delegate to other abstract methods:
          document(int) – delegates to document(int, FieldSelector)
          setNorm – delegates to doSetNorm.
          setNorm – deprecated, but delegates to setNorm above.
          termPositions(Term) – calls termPositions().
          deleteDocument(int) – delegates to doDelete.
          deleteDocuments – calls deleteDocument.
          undeleteAll – delegates to doUndeleteAll().

          If you disagree, then I will override them in FIR.

          Show
          Shai Erera added a comment - How about making those methods final on IR: getRefCount incRef tryIncRef decRef They reference refCount, and I cannot think of an IR extension that would want to override any of these methods? numDeletedDocs – should be made final, per Uwe's proposal. These methods, IMO, can be made final as well, because they delegate to other abstract methods: document(int) – delegates to document(int, FieldSelector) setNorm – delegates to doSetNorm. setNorm – deprecated, but delegates to setNorm above. termPositions(Term) – calls termPositions(). deleteDocument(int) – delegates to doDelete. deleteDocuments – calls deleteDocument. undeleteAll – delegates to doUndeleteAll(). If you disagree, then I will override them in FIR.
          Hide
          Shai Erera added a comment -

          Patch against 3x:

          • Adds a HashSet of methods that should not be overridden by FilterIndexReader.
            • If a method appears there and is not overridden, it is an error.
            • If a method appears there and is overridden, it is an error as well.
          • Override more methods by FIR.

          see previous comment for the rest of the methods.

          Show
          Shai Erera added a comment - Patch against 3x: Adds a HashSet of methods that should not be overridden by FilterIndexReader. If a method appears there and is not overridden, it is an error. If a method appears there and is overridden, it is an error as well. Override more methods by FIR. see previous comment for the rest of the methods.
          Hide
          Uwe Schindler added a comment -

          Then why don't we make FilterIndexReader abstract class, and declare reopen/doOpenIfChanged/clone abstract? I don't think it's useful to just init FilterIndexReader right? And clearly those methods need to be overridden by sub-classes, or they would already receive UnsupportedOperationException no?

          It would be abstract already on IndexReader if it would not be optional - thats the reason, to not make it abstract. So FIR should reflect this behaviour. We already have some FIRs that explicitely do not support reopen (like SlowMultiReaderWrapper).

          I agree with making all those methods final you mentioned above.

          Show
          Uwe Schindler added a comment - Then why don't we make FilterIndexReader abstract class, and declare reopen/doOpenIfChanged/clone abstract? I don't think it's useful to just init FilterIndexReader right? And clearly those methods need to be overridden by sub-classes, or they would already receive UnsupportedOperationException no? It would be abstract already on IndexReader if it would not be optional - thats the reason, to not make it abstract. So FIR should reflect this behaviour. We already have some FIRs that explicitely do not support reopen (like SlowMultiReaderWrapper). I agree with making all those methods final you mentioned above.
          Hide
          Shai Erera added a comment -

          Patch makes the mentioned methods final, modifies SolrIndexReader and other IR extensions (ParallelReader, Instantiated, MemoryIndex) to not override them.

          Also added a CHANGES entry under backwards compatibility.

          If there are no objections, I will commit it later today.

          Show
          Shai Erera added a comment - Patch makes the mentioned methods final, modifies SolrIndexReader and other IR extensions (ParallelReader, Instantiated, MemoryIndex) to not override them. Also added a CHANGES entry under backwards compatibility. If there are no objections, I will commit it later today.
          Hide
          Shai Erera added a comment -

          Committed rev 1211413 to 3x. Porting to trunk

          Show
          Shai Erera added a comment - Committed rev 1211413 to 3x. Porting to trunk
          Hide
          Shai Erera added a comment -

          Patch adds the test to TestFilterIndexReader. Uwe asked that I do not commit these changes (test + FIR/IR fixes) until he merges in the branch on IR-read-only. We decided that Uwe will apply that patch to the branch, fix FIR/IR there and merge the branch afterwards.

          Show
          Shai Erera added a comment - Patch adds the test to TestFilterIndexReader. Uwe asked that I do not commit these changes (test + FIR/IR fixes) until he merges in the branch on IR-read-only. We decided that Uwe will apply that patch to the branch, fix FIR/IR there and merge the branch afterwards.
          Hide
          Uwe Schindler added a comment -

          I added the patch to the LUCENE-3606 branch and fixed FilterIndexReader there (was missing 2 methods: getIndexCommit and getTermInfosIndexDivisor). The CHANGES.txt was merged to trunk.

          Show
          Uwe Schindler added a comment - I added the patch to the LUCENE-3606 branch and fixed FilterIndexReader there (was missing 2 methods: getIndexCommit and getTermInfosIndexDivisor). The CHANGES.txt was merged to trunk.
          Hide
          Uwe Schindler added a comment -

          In general like reopen, getSequentialSubReaders should default to return null in FilterIndexReader. If it delegates, the filter has no chance to filter the segments - leading to bugs...

          Show
          Uwe Schindler added a comment - In general like reopen, getSequentialSubReaders should default to return null in FilterIndexReader. If it delegates, the filter has no chance to filter the segments - leading to bugs...
          Hide
          Shai Erera added a comment -

          I added the patch to the LUCENE-3606 branch and fixed FilterIndexReader there

          That's great, thanks ! So can I resolve this issue?

          Show
          Shai Erera added a comment - I added the patch to the LUCENE-3606 branch and fixed FilterIndexReader there That's great, thanks ! So can I resolve this issue?
          Hide
          Uwe Schindler added a comment -

          I will do it once the branch is merged back.

          Show
          Uwe Schindler added a comment - I will do it once the branch is merged back.
          Hide
          Uwe Schindler added a comment -

          Committed to trunk through merge of LUCENE-3606.

          Show
          Uwe Schindler added a comment - Committed to trunk through merge of LUCENE-3606 .

            People

            • Assignee:
              Shai Erera
              Reporter:
              Shai Erera
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development