Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-1217

use isBinary cached variable instead of instanceof in Field

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: core/other
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      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?

      1. LUCENE-1217.patch
        1 kB
        Eks Dev
      2. Lucene-1217-take1.patch
        2 kB
        Eks Dev

        Issue Links

          Activity

          Hide
          mikemccand Michael McCandless added a comment -

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

          Show
          mikemccand Michael McCandless added a comment - 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...
          Hide
          eksdev Eks Dev added a comment -

          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

          Show
          eksdev Eks Dev added a comment - 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
          Hide
          mikemccand Michael McCandless added a comment -

          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)

          Show
          mikemccand Michael McCandless added a comment - 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)
          Hide
          eksdev Eks Dev added a comment -

          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

          Show
          eksdev Eks Dev added a comment - 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
          Hide
          eksdev Eks Dev added a comment -

          new patch, fixes isBinary status in LazyField

          Show
          eksdev Eks Dev added a comment - new patch, fixes isBinary status in LazyField
          Hide
          mikemccand Michael McCandless added a comment -

          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.

          Show
          mikemccand Michael McCandless added a comment - 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.
          Hide
          cutting Doug Cutting added a comment -

          fix typo that's been bugging me

          Show
          cutting Doug Cutting added a comment - fix typo that's been bugging me

            People

            • Assignee:
              mikemccand Michael McCandless
              Reporter:
              eksdev Eks Dev
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development