Lazy field loading breaks backward compat

• Key: LUCENE-609
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Yonik Seeley
• Reporter: Yonik Seeley
• Component/s: Other
• Affects Version/s: 2.1
• Fix Version/s: 2.1

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");

Comments

[Yonik Seeley]

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

[Yonik Seeley]

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).

[Yonik Seeley]

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

[Grant Ingersoll]

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

[Yonik Seeley]

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.

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

[Yonik Seeley]

> 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()?