Lucene - Core
  1. Lucene - Core
  2. LUCENE-3853

Improve MIGRATE.txt for FieldsEnum/TermsEnum

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: None
    • Component/s: core/other
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      The text in MIGRATE.txt that explains the new situation with FieldsEnum seems to be missing a level of object in explaining TermEnum. Perhaps it didn't keep with the code. here's a patch to MIGRATE.txt.

      1. LUCENE-3853.patch
        3 kB
        Benson Margulies
      2. LUCENE-3853.patch
        3 kB
        Benson Margulies
      3. LUCENE-3853.patch
        1 kB
        Benson Margulies
      4. LUCENE-3853.patch
        0.7 kB
        Benson Margulies

        Activity

        Hide
        Robert Muir added a comment -

        Hi Benson, thanks for the patch.

        I think it would be better though if our example didnt recommend MultiFields (or at least noted that this is the slow/trappy way to access terms)
        Also we need some null checks I think in the example, because e.g. fields() is allowed to return null, so it should be checked before it has an
        iterator...

        but i think this is overall a great idea? perhaps if we come up with a good example we can think of a place in the javadocs it could reside as well,
        so its not just in the migration guide (e.g. if you are a new Lucene user, you likely wouldnt even look at MIGRATE.txt but this would still be useful).

        Show
        Robert Muir added a comment - Hi Benson, thanks for the patch. I think it would be better though if our example didnt recommend MultiFields (or at least noted that this is the slow/trappy way to access terms) Also we need some null checks I think in the example, because e.g. fields() is allowed to return null, so it should be checked before it has an iterator... but i think this is overall a great idea? perhaps if we come up with a good example we can think of a place in the javadocs it could reside as well, so its not just in the migration guide (e.g. if you are a new Lucene user, you likely wouldnt even look at MIGRATE.txt but this would still be useful).
        Hide
        Uwe Schindler added a comment -

        In most cases, requesting a FieldsEnum is not needed, you normally use AtomicReader.fields().terms("fieldname") [with several null-checks]. I think this code should be prominently on the howto.

        Show
        Uwe Schindler added a comment - In most cases, requesting a FieldsEnum is not needed, you normally use AtomicReader.fields().terms("fieldname") [with several null-checks] . I think this code should be prominently on the howto.
        Hide
        Benson Margulies added a comment -

        So, the explanation below what I patchd that I failed to read the first time sort-of endorses Multi-Field.

        I happen to be updating code of someone else's that want to walk all of the fields, which is how I ran into this.

        One possibility is to change the comment I added to say 'this is not the recommended way, read below for more information on this' and then focus effort on howto and examples instead of MIGRATION/CHANGE, rather than making the little Migratin example bigger and bigger?

        Where in the tree do you generally put examples?

        Show
        Benson Margulies added a comment - So, the explanation below what I patchd that I failed to read the first time sort-of endorses Multi-Field. I happen to be updating code of someone else's that want to walk all of the fields, which is how I ran into this. One possibility is to change the comment I added to say 'this is not the recommended way, read below for more information on this' and then focus effort on howto and examples instead of MIGRATION/CHANGE, rather than making the little Migratin example bigger and bigger? Where in the tree do you generally put examples?
        Hide
        Benson Margulies added a comment -

        Improved version.

        Show
        Benson Margulies added a comment - Improved version.
        Hide
        Benson Margulies added a comment -

        Further adjustments to reflect advice from Uwe.

        Show
        Benson Margulies added a comment - Further adjustments to reflect advice from Uwe.
        Hide
        Uwe Schindler added a comment - - edited

        Nice!

        some comments:

        • "If you pass a SegmentReader to SlowCompositeReaderWrapper it will simply return reader.fields()," should be "If you pass a SegmentReader to SlowCompositeReaderWrapper.wrap() it will simply return itsself,"
        • The MultiFields comment should be in the line of @lucene.internal (changed from @lucene.experimental). @lucene.internal will already print the "internal" warning. We should review the other Multi* classes, too. They are all internal not experimental.
        Show
        Uwe Schindler added a comment - - edited Nice! some comments: "If you pass a SegmentReader to SlowCompositeReaderWrapper it will simply return reader.fields()," should be "If you pass a SegmentReader to SlowCompositeReaderWrapper.wrap() it will simply return itsself," The MultiFields comment should be in the line of @lucene.internal (changed from @lucene.experimental). @lucene.internal will already print the "internal" warning. We should review the other Multi* classes, too. They are all internal not experimental.
        Hide
        Benson Margulies added a comment -

        Once more with edits as per Uwe.

        Show
        Benson Margulies added a comment - Once more with edits as per Uwe.

          People

          • Assignee:
            Unassigned
            Reporter:
            Benson Margulies
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development