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
        0.7 kB
        Benson Margulies
      2. LUCENE-3853.patch
        1 kB
        Benson Margulies
      3. LUCENE-3853.patch
        3 kB
        Benson Margulies
      4. LUCENE-3853.patch
        3 kB
        Benson Margulies

        Activity

        Benson Margulies created issue -
        Benson Margulies made changes -
        Field Original Value New Value
        Attachment LUCENE-3853.patch [ 12517239 ]
        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.
        Benson Margulies made changes -
        Description The text in CHANGES.txt that explains the new situation with FieldsEnum doesn't explain how to get one, and the explanation of TermEnum seems not to have kept up with the code. here's a patch to CHANGES.txt. The text in CHANGES.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 CHANGES.txt.
        Uwe Schindler made changes -
        Summary Improve CHANGES.txt for FieldsEnum/TermsEnum Improve MIGRATE.txt for FieldsEnum/TermsEnum
        Description The text in CHANGES.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 CHANGES.txt. 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.
        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.
        Benson Margulies made changes -
        Attachment LUCENE-3853.patch [ 12517240 ]
        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.
        Benson Margulies made changes -
        Attachment LUCENE-3853.patch [ 12517244 ]
        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.
        Benson Margulies made changes -
        Attachment LUCENE-3853.patch [ 12517258 ]

          People

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

            Dates

            • Created:
              Updated:

              Development