use isBinary cached variable instead of instanceof in Field

• Key: LUCENE-1217
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Trivial
• Assignee: Michael McCandless
• Reporter: Eks Dev
• Component/s: Other

Description

Field class can hold three types of values,
See: AbstractField.java protected Object fieldsData = null;

currently, mainly RTTI (instanceof) is used to determine the type of the value stored in particular instance of the Field, but for binary value we have mixed RTTI and cached variable "boolean isBinary"

This patch makes consistent use of cached variable isBinary.

Benefit: consistent usage of method to determine run-time type for binary case (reduces chance to get out of sync on cached variable). It should be slightly faster as well.

Thinking aloud:
Would it not make sense to maintain type with some integer/byte"poor man's enum" (Interface with a couple of constants)
code:java{
public static final interface Type{
public static final byte BOOLEAN = 0;
public static final byte STRING = 1;
public static final byte READER = 2;
....
}
}

and use that instead of isBinary + instanceof?

Comments

[Michael McCandless]

Patch looks good. I will commit shortly. Thanks Eks Dev.

Would it not make sense to maintain type with some integer/byte"poor man's enum" (Interface with a couple of constants)

Or we could wait until Java 5 (3.0) and use real enums?

Or ... maybe we should have subclasses of Field (TextField, BinaryField,
ReaderField, TokenStreamField) which override the corresponding method
(and the base Field.java would still implement these methods but
return null)? Though this would be a rather large change...

[Eks Dev]

thanks fof looking into it!
Subclassing now with backwards compatibility would be clumsy, I was thinking about it but could not find clean way to make it.

>>Or we could wait until Java 5 (3.0) and use real enums?
yes, that is ultimate solution, but my line of thoughts was that "poor man's enum"->java 5 enum migration would be trivial later... but do not change working code kicks-in here

[Michael McCandless]

Actually seeing a test failure with this:

[junit] Testcase: testLazyFields(org.apache.lucene.index.TestFieldsReader): FAILED
[junit] bytes is null and it shouldn't be
[junit] junit.framework.AssertionFailedError: bytes is null and it shouldn't be
[junit] at org.apache.lucene.index.TestFieldsReader.testLazyFields(TestFieldsReader.java:132)

[Eks Dev]

hah, this bug just justified this patch
sorry, I should have run tests before... nothing is trivial enough.
The problem was indeed isBinary that went out of sync in LazyField, new patch follows

[Eks Dev]

new patch, fixes isBinary status in LazyField

[Michael McCandless]

OK the new patch passes all tests – thanks!

One unrelated thing I noticed: it looks like you can get a binary LazyField and then ask for its stringValue(), and vice-versa. Ie we are failing to check in binaryValue() that the field is in fact binary even though when we create the LazyField we know whether it is. I'll open a separate issue for this.

[Doug Cutting]

fix typo that's been bugging me