Lucene - Core
  1. Lucene - Core
  2. LUCENE-609

Lazy field loading breaks backward compat

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: core/other
    • Labels:
      None

      Description

      Document.getField() and Document.getFields() have changed in a non backward compatible manner.
      Simple code like the following no longer compiles:
      Field x = mydoc.getField("x");

        Activity

        Hide
        Yonik Seeley added a comment -

        I'll work up a quick patch to revert those APIs and add new ones for Fieldable.

        Show
        Yonik Seeley added a comment - I'll work up a quick patch to revert those APIs and add new ones for Fieldable.
        Hide
        Yonik Seeley added a comment -

        OK, here's the quick'n'dirty version that passes lucene unit tests.

        • The new getFieldable methods can be considered temporary until we decide if that's really the best way to provide access to the new functionallity.
        • At least some of the Fieldable instances in all of the unit tests should be reverted back to Field so we get some test coverage (that is what everyone still uses).
        Show
        Yonik Seeley added a comment - OK, here's the quick'n'dirty version that passes lucene unit tests. The new getFieldable methods can be considered temporary until we decide if that's really the best way to provide access to the new functionallity. At least some of the Fieldable instances in all of the unit tests should be reverted back to Field so we get some test coverage (that is what everyone still uses).
        Hide
        Yonik Seeley added a comment -

        I just verified that Solr now compiles/works correctly again with this patch.
        Any objections to committing it?

        Show
        Yonik Seeley added a comment - I just verified that Solr now compiles/works correctly again with this patch. Any objections to committing it?
        Hide
        Grant Ingersoll added a comment -

        I think there is a typo of getFielables on Document.java, other than that looks good.

        Show
        Grant Ingersoll added a comment - I think there is a typo of getFielables on Document.java, other than that looks good.
        Hide
        Yonik Seeley added a comment -

        Committed (with typo fixes

        The main point for me was to return backward compatibility of getField(s)
        Other committers should feel free to change/remove the getFieldable methods... they are still new enough to be considered temporary IMO.

        Show
        Yonik Seeley added a comment - Committed (with typo fixes The main point for me was to return backward compatibility of getField(s) Other committers should feel free to change/remove the getFieldable methods... they are still new enough to be considered temporary IMO.
        Hide
        Chuck Williams added a comment -

        I'm late to the discussion and have only read the patch file, but it seems invalid to me. Won't getField() get a class cast exception when it encounters a Fieldable that is not a Field? The semantics of getField() would have to be something like, "only get this field if it is a Field rather than some other kind of Fieldable", which means it would have to do type testing on the members of fields.

        I think it is much better to remove this patch and leave Fieldable as is. Searchable was the same kind of thing. IndexReader is an abstract super class for the different types of readers. When I did ParallelWriter, I had the same problem and had to introduce Writable since IndexWriter is not an abstract class and ParallelWriter is a different implementation. I think it is best to introduce all the abstract classes now for fundamental types that have multiple implementations.

        Chuck

        Show
        Chuck Williams added a comment - I'm late to the discussion and have only read the patch file, but it seems invalid to me. Won't getField() get a class cast exception when it encounters a Fieldable that is not a Field? The semantics of getField() would have to be something like, "only get this field if it is a Field rather than some other kind of Fieldable", which means it would have to do type testing on the members of fields. I think it is much better to remove this patch and leave Fieldable as is. Searchable was the same kind of thing. IndexReader is an abstract super class for the different types of readers. When I did ParallelWriter, I had the same problem and had to introduce Writable since IndexWriter is not an abstract class and ParallelWriter is a different implementation. I think it is best to introduce all the abstract classes now for fundamental types that have multiple implementations. Chuck
        Hide
        Yonik Seeley added a comment -

        > I'm late to the discussion
        Yes, I didn't leave much time for debate I really wanted to get back to something backward compatible so I could update Solr to use the latest Lucene.

        > but it seems invalid to me. Won't getField() get a class cast exception

        Yes, as I noted here:
        http://www.nabble.com/Fieldable-breaks-backward-compatibility-t1825407.html#a4979233
        But only if you are using the new Field options. That's the price to pay for backward compatibility, but it's a much better alternative than breaking everyones code when it's not necessary.

        > it would have to do type testing on the members of fields.

        The JVM does this for us

        > Searchable was the same kind of thing.

        I don't recall if it did breack backward compatibility, but even so... how many people write their own Searchers/IndexReaders vs how many people call Document.getField()?

        Show
        Yonik Seeley added a comment - > I'm late to the discussion Yes, I didn't leave much time for debate I really wanted to get back to something backward compatible so I could update Solr to use the latest Lucene. > but it seems invalid to me. Won't getField() get a class cast exception Yes, as I noted here: http://www.nabble.com/Fieldable-breaks-backward-compatibility-t1825407.html#a4979233 But only if you are using the new Field options. That's the price to pay for backward compatibility, but it's a much better alternative than breaking everyones code when it's not necessary. > it would have to do type testing on the members of fields. The JVM does this for us > Searchable was the same kind of thing. I don't recall if it did breack backward compatibility, but even so... how many people write their own Searchers/IndexReaders vs how many people call Document.getField()?

          People

          • Assignee:
            Yonik Seeley
            Reporter:
            Yonik Seeley
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development