Lucene - Core
  1. Lucene - Core
  2. LUCENE-4152

add one-syllable method to IndexReader enumerate subreaders

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Description is exactly as written.

      getSequentialSubReaders/getTopLevelReaderContext, these method names are way too long/unuseable. They also have tricky semantics (e.g. returning null).

      In lucene 4, people cannot just use any indexreader and get a merged view. So we need to make this stuff easy on them:

      • single-syllable method name (leaves(), subs(), i will think on this)
      • supports enhanced for-loop (no returning null or anything like that)
      • on indexreader (not atomic or composite, plain old indexreader)
      1. LUCENE-4152.patch
        0.9 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment - - edited

          They also have tricky semantics (e.g. returning null).

          No longer since January's refactoring, leaves() returns always something != null. And getSeqSubReaders is only available on CompositeReaders, which always have subReaders. See the other issue LUCENE-3866 where I did some more refactoring to make all this easier.

          In general I agree with that, as leaves() and children()/subs() would then just be a shortcut to getTopReaderContext()'s methods (leaves(), children() - both are Iterable, no need to change anything) (I would rename that one to as[Top]Context()).

          Returning plain subReaders without Contexts is not really useful, as all of Lucene's Query logic uses AtomicReaderContext, so leaves() on IndexReader returning the same as getTopReaderContext().leaves() is the way to go. This method can be added as final "easy-use method". I hope you look at my other patch @ LUCENE-3866, because it shows how simple the code is now without ReaderUtil.Gather.

          Additionally, as noted in LUCENE-3866, getSequentialSubReaders() in CompositeReader should be made protected, user code does not need to use it. It's solely there for building the reader hierarchy, later available using IRC.leaves() and IRC.children(). Currently this method is only used by tests anymore (which can be rewritten). In CompositeReader, getSequentialSubReaders() should just be the protected abstract way for the API on top to get the inner structure, but not for the outside.

          Show
          Uwe Schindler added a comment - - edited They also have tricky semantics (e.g. returning null). No longer since January's refactoring, leaves() returns always something != null. And getSeqSubReaders is only available on CompositeReaders, which always have subReaders. See the other issue LUCENE-3866 where I did some more refactoring to make all this easier. In general I agree with that, as leaves() and children()/subs() would then just be a shortcut to getTopReaderContext()'s methods (leaves(), children() - both are Iterable, no need to change anything) (I would rename that one to as [Top] Context()). Returning plain subReaders without Contexts is not really useful, as all of Lucene's Query logic uses AtomicReaderContext, so leaves() on IndexReader returning the same as getTopReaderContext().leaves() is the way to go. This method can be added as final "easy-use method". I hope you look at my other patch @ LUCENE-3866 , because it shows how simple the code is now without ReaderUtil.Gather. Additionally, as noted in LUCENE-3866 , getSequentialSubReaders() in CompositeReader should be made protected, user code does not need to use it. It's solely there for building the reader hierarchy, later available using IRC.leaves() and IRC.children(). Currently this method is only used by tests anymore (which can be rewritten). In CompositeReader, getSequentialSubReaders() should just be the protected abstract way for the API on top to get the inner structure, but not for the outside.
          Hide
          Robert Muir added a comment -

          ok, so after LUCENE-3866 is resolved maybe all that is needed here is an eclipse rename?

          Show
          Robert Muir added a comment - ok, so after LUCENE-3866 is resolved maybe all that is needed here is an eclipse rename?
          Hide
          Uwe Schindler added a comment -

          Yes, and some final methods in IndexReader abstract class for easy usage, so you can do:

          for (AtomicReaderContext ctx : reader.leaves()) ...
          
          Show
          Uwe Schindler added a comment - Yes, and some final methods in IndexReader abstract class for easy usage, so you can do: for (AtomicReaderContext ctx : reader.leaves()) ...
          Hide
          Hoss Man added a comment -

          bulk cleanup of 4.0-ALPHA / 4.0 Jira versioning. all bulk edited issues have hoss20120711-bulk-40-change in a comment

          Show
          Hoss Man added a comment - bulk cleanup of 4.0-ALPHA / 4.0 Jira versioning. all bulk edited issues have hoss20120711-bulk-40-change in a comment
          Hide
          Robert Muir added a comment -

          rmuir20120906-bulk-40-change

          Show
          Robert Muir added a comment - rmuir20120906-bulk-40-change
          Hide
          Robert Muir added a comment -

          trivial patch

          Show
          Robert Muir added a comment - trivial patch
          Hide
          Michael McCandless added a comment -

          +1, we should make this easy/obvious.

          Show
          Michael McCandless added a comment - +1, we should make this easy/obvious.
          Hide
          Robert Muir added a comment -

          I'd like to commit this. ill then do a search to see if there is any example code/tests etc that can be simplified.

          Show
          Robert Muir added a comment - I'd like to commit this. ill then do a search to see if there is any example code/tests etc that can be simplified.
          Hide
          Uwe Schindler added a comment -

          As followup I will make getSequentialSubReaders protected, as this method is only there to make the top reader context. It should not be used outside CompositeReader.

          Show
          Uwe Schindler added a comment - As followup I will make getSequentialSubReaders protected, as this method is only there to make the top reader context. It should not be used outside CompositeReader.
          Hide
          Uwe Schindler added a comment -

          Closed after release.

          Show
          Uwe Schindler added a comment - Closed after release.

            People

            • Assignee:
              Robert Muir
              Reporter:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development