Solr
  1. Solr
  2. SOLR-398

Widen return type of FiledType.createField to Fieldable in order to maximize flexibility

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2, 1.3
    • Fix Version/s: 4.0-ALPHA
    • Component/s: update
    • Labels:
      None

      Description

      FieldType.createField currently returns Field.

      In order to maximize flexibility for developers to extend Solr, it should return Fieldable.

      1. trunk-FieldType-5.patch
        10 kB
        Espen Amble Kolstad
      2. trunk-FieldType-4.patch
        8 kB
        Espen Amble Kolstad
      3. trunk-FieldType-3.patch
        8 kB
        Espen Amble Kolstad
      4. fieldable.patch
        12 kB
        Andrzej Bialecki
      5. 1.2-FieldType-2.patch
        7 kB
        Espen Amble Kolstad

        Activity

        Hide
        Espen Amble Kolstad added a comment -

        Here are patches fro trunk and 1.2

        Show
        Espen Amble Kolstad added a comment - Here are patches fro trunk and 1.2
        Hide
        Otis Gospodnetic added a comment -

        I'm not on a computer where I can try this out, but this looks simple and shouldn't hold up OpenPipe.
        All unit tests still pass, Espen?

        Show
        Otis Gospodnetic added a comment - I'm not on a computer where I can try this out, but this looks simple and shouldn't hold up OpenPipe. All unit tests still pass, Espen?
        Hide
        Grant Ingersoll added a comment -

        This will break back-compatibility, right? Better to deprecate and add a new method?

        Show
        Grant Ingersoll added a comment - This will break back-compatibility, right? Better to deprecate and add a new method?
        Hide
        Yonik Seeley added a comment -

        This will break back-compatibility, right?

        Unfortunately, yes. That's why I only changed methods that accepted a Field, but couldn't change return types because of potential code like: Field f = ft.createField()

        Better to deprecate and add a new method?

        Probably, if creation of fields might be done in custom request handlers.

        Show
        Yonik Seeley added a comment - This will break back-compatibility, right? Unfortunately, yes. That's why I only changed methods that accepted a Field, but couldn't change return types because of potential code like: Field f = ft.createField() Better to deprecate and add a new method? Probably, if creation of fields might be done in custom request handlers.
        Hide
        Espen Amble Kolstad added a comment -

        Sorry for the late reply! I've been on holiday.

        I've given it another go where I've deprecated createField and added createFieldable in both FieldType and SchemaField.
        FieldType.createFieldable currently only calls createField to maintain backwards compatability

        Comments?

        Show
        Espen Amble Kolstad added a comment - Sorry for the late reply! I've been on holiday. I've given it another go where I've deprecated createField and added createFieldable in both FieldType and SchemaField. FieldType.createFieldable currently only calls createField to maintain backwards compatability Comments?
        Hide
        Espen Amble Kolstad added a comment - - edited

        I've updated the patch for trunk

        All tests pass

        Any chance of this making it into 1.3?

        Espen

        Show
        Espen Amble Kolstad added a comment - - edited I've updated the patch for trunk All tests pass Any chance of this making it into 1.3? Espen
        Hide
        Espen Amble Kolstad added a comment -

        Here's an updated patch for trunk

        Show
        Espen Amble Kolstad added a comment - Here's an updated patch for trunk
        Hide
        Andrzej Bialecki added a comment -

        Great minds think alike I was about to submit exactly the same patch when I noticed this JIRA.

        One thing is missing in your patch - DocumentBuilder:282 should use "out.getFieldable()" instead of "out.getField()". This is required if you provide a custom Fieldable implementation (!instanceof o.a.l.d.Field) in FieldType subclasses, because o.a.l.d.Document.getField tries to cast the result to o.a.l.d.Field, whereas Document.getFieldable is happy with any subclass of Fieldable

        Show
        Andrzej Bialecki added a comment - Great minds think alike I was about to submit exactly the same patch when I noticed this JIRA. One thing is missing in your patch - DocumentBuilder:282 should use "out.getFieldable()" instead of "out.getField()". This is required if you provide a custom Fieldable implementation (!instanceof o.a.l.d.Field) in FieldType subclasses, because o.a.l.d.Document.getField tries to cast the result to o.a.l.d.Field, whereas Document.getFieldable is happy with any subclass of Fieldable
        Hide
        Espen Amble Kolstad added a comment -

        You are right. There are some other places where Document.getField(String name) was used. I changed them as well.
        I've included the new patch

        Document.getField(String name) should be rewritten or deprecated ?!

        Show
        Espen Amble Kolstad added a comment - You are right. There are some other places where Document.getField(String name) was used. I changed them as well. I've included the new patch Document.getField(String name) should be rewritten or deprecated ?!
        Hide
        Espen Amble Kolstad added a comment -

        All tests pass

        Show
        Espen Amble Kolstad added a comment - All tests pass
        Hide
        Andrzej Bialecki added a comment -

        Patch updated to current trunk. One concrete use case where this is needed is Fieldable implementations that provide different values of tokenStreamValue() and stringValue(), e.g. when using external tools to provide a pre-tokenized value of the field.

        Show
        Andrzej Bialecki added a comment - Patch updated to current trunk. One concrete use case where this is needed is Fieldable implementations that provide different values of tokenStreamValue() and stringValue(), e.g. when using external tools to provide a pre-tokenized value of the field.
        Hide
        Hoss Man added a comment -

        Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email...

        http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

        Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed.

        A unique token for finding these 240 issues in the future: hossversioncleanup20100527

        Show
        Hoss Man added a comment - Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed. A unique token for finding these 240 issues in the future: hossversioncleanup20100527
        Hide
        Robert Muir added a comment -

        Bulk move 3.2 -> 3.3

        Show
        Robert Muir added a comment - Bulk move 3.2 -> 3.3
        Hide
        Robert Muir added a comment -

        3.4 -> 3.5

        Show
        Robert Muir added a comment - 3.4 -> 3.5
        Hide
        Hoss Man added a comment -

        Bulk of fixVersion=3.6 -> fixVersion=4.0 for issues that have no assignee and have not been updated recently.

        email notification suppressed to prevent mass-spam
        psuedo-unique token identifying these issues: hoss20120321nofix36

        Show
        Hoss Man added a comment - Bulk of fixVersion=3.6 -> fixVersion=4.0 for issues that have no assignee and have not been updated recently. email notification suppressed to prevent mass-spam psuedo-unique token identifying these issues: hoss20120321nofix36
        Hide
        Ryan McKinley added a comment -

        In trunk (4.x), FieldType now returns the interface IndexableField

        Show
        Ryan McKinley added a comment - In trunk (4.x), FieldType now returns the interface IndexableField

          People

          • Assignee:
            Unassigned
            Reporter:
            Espen Amble Kolstad
          • Votes:
            4 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development