Lucene - Core
  1. Lucene - Core
  2. LUCENE-3777

trapping overloaded ctors/setters in Field/NumericField/DocValuesField

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      In trunk, these apis let you easily create a field, but my concern is this:

      public NumericField(String name, int value)
      public NumericField(String name, long value)
      ..
      public Field(String name, int value, FieldType type)
      public Field(String name, long value, FieldType type)
      ..
      public void setValue(int value)
      public void setValue(long value)
      ..
      public DocValuesField(String name, int value, DocValues.Type docValueType)
      public DocValuesField(String name, long value, DocValues.Type docValueType)
      

      I really don't like overloaded ctors/setters where the compiler can type-promote you,
      I think it makes the apis hard to use.

      Instead for the setters I think we sohuld have setIntValue, setLongValue, ...
      For the ctors, I see two other options:

      1. factories like DocValuesField.newIntField()
      2. subclasses like IntField

      I don't have any patch for this, but I think we should discuss and fix before these apis are released.

      1. LUCENE-3777.patch
        177 kB
        Michael McCandless
      2. LUCENE-3777.patch
        86 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        I committed under the wrong issue – see LUCENE-3077 for the commit log.

        Show
        Michael McCandless added a comment - I committed under the wrong issue – see LUCENE-3077 for the commit log.
        Hide
        Michael McCandless added a comment -

        Patch fixing DocValuesField: I broke up DocValuesField into one Field class per type (IntDocValuesField, etc.).

        Show
        Michael McCandless added a comment - Patch fixing DocValuesField: I broke up DocValuesField into one Field class per type (IntDocValuesField, etc.).
        Hide
        Michael McCandless added a comment -

        Thanks Robert, I committed first part... I'll leave this open to work on DocValuesField next.

        Show
        Michael McCandless added a comment - Thanks Robert, I committed first part... I'll leave this open to work on DocValuesField next.
        Hide
        Robert Muir added a comment -

        Thanks Mike, such patches go out of date quickly... +1 to commit and remove this part of the trap!

        Show
        Robert Muir added a comment - Thanks Mike, such patches go out of date quickly... +1 to commit and remove this part of the trap!
        Hide
        Michael McCandless added a comment -

        Patch, splitting NF into Int/Long/Float/DoubleField, and changing
        Field.setValue(T value) -> Field.setTValue(T value).

        Tests pass... I'd like to commit this first (big, rote patch, will
        conflict soon) and then do DocValuesField separately...

        Show
        Michael McCandless added a comment - Patch, splitting NF into Int/Long/Float/DoubleField, and changing Field.setValue(T value) -> Field.setTValue(T value). Tests pass... I'd like to commit this first (big, rote patch, will conflict soon) and then do DocValuesField separately...
        Hide
        Chris Male added a comment -

        I prefer sugar classes (new IntField(7), new IntValueField(7)) instead of static factory methods (NumericField.newIntField(7), DocValuesField.newIntField(7))...

        +1 to sugar classes. It gives us strong type safety and the ability to add new classes overtime.

        Show
        Chris Male added a comment - I prefer sugar classes (new IntField(7), new IntValueField(7)) instead of static factory methods (NumericField.newIntField(7), DocValuesField.newIntField(7))... +1 to sugar classes. It gives us strong type safety and the ability to add new classes overtime.
        Hide
        Michael McCandless added a comment -

        I agree this is trappy! Especially because these are sugar APIs... which should especially not be trappy.

        I don't know what was the intention to change this in trunk,

        This was my fault: I did this under LUCENE-3453... each of our Field impls (well, Field, NF, DVF) had their own setters to change their value... I consolidated all of these under Field's APIs, which I agree are trappy.

        I think we should just break with 3.x here and change Field.setValue(T x) -> Field.setTValue(T x).

        I prefer sugar classes (new IntField(7), new IntValueField(7)) instead of static factory methods (NumericField.newIntField(7), DocValuesField.newIntField(7))...

        I'll take a crack at this.

        Show
        Michael McCandless added a comment - I agree this is trappy! Especially because these are sugar APIs... which should especially not be trappy. I don't know what was the intention to change this in trunk, This was my fault: I did this under LUCENE-3453 ... each of our Field impls (well, Field, NF, DVF) had their own setters to change their value... I consolidated all of these under Field's APIs, which I agree are trappy. I think we should just break with 3.x here and change Field.setValue(T x) -> Field.setTValue(T x). I prefer sugar classes (new IntField(7), new IntValueField(7)) instead of static factory methods (NumericField.newIntField(7), DocValuesField.newIntField(7))... I'll take a crack at this.
        Hide
        Uwe Schindler added a comment -

        The biggest trap is silently creating wrong types that are incompatible with what your index has:

        new NumericField(..., 5L) vs new NumericField(..., 5) is producing something totaly different. Please note the "L", forgetting to add this, you suddenly get completely incompatible fields.

        This is unreleaseable!

        Show
        Uwe Schindler added a comment - The biggest trap is silently creating wrong types that are incompatible with what your index has: new NumericField(..., 5L) vs new NumericField(..., 5) is producing something totaly different. Please note the "L", forgetting to add this, you suddenly get completely incompatible fields. This is unreleaseable!
        Hide
        Uwe Schindler added a comment -

        Hi Robert,

        that type overloading was exactly the reason why in 3.x we have setXxxValue(). I don't know what was the intention to change this in trunk, but the type overloading on int/long/float/byte is very trappy. We should change this back to what we had in 3.x!

        factories like DocValuesField.newIntField()

        Thats how NumericRangeQuery looks like (it's exactly to overcome the problem with type overloading).

        Show
        Uwe Schindler added a comment - Hi Robert, that type overloading was exactly the reason why in 3.x we have setXxxValue(). I don't know what was the intention to change this in trunk, but the type overloading on int/long/float/byte is very trappy. We should change this back to what we had in 3.x! factories like DocValuesField.newIntField() Thats how NumericRangeQuery looks like (it's exactly to overcome the problem with type overloading).

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development