Lucene - Core
  1. Lucene - Core
  2. LUCENE-4833

Fix default MergePolicy in IndexWriterConfig

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Although the default merge policy is TieredMergePolicy (as documented in IndexWriterConfig constructor), setMergePolicy assumes that the default is LogByteSizeMergePolicy.

      1. LUCENE-4833.patch
        10 kB
        Adrien Grand
      2. LUCENE-4833.patch
        2 kB
        Adrien Grand

        Activity

        Hide
        Adrien Grand added a comment -

        Patch.

        Show
        Adrien Grand added a comment - Patch.
        Hide
        Shai Erera added a comment -

        good catch. But why do you throw NPE? the rest of the setXYZ methods respect null by setting the default. I don't like that setMP is not consistent with the rest of the setters.

        Show
        Shai Erera added a comment - good catch. But why do you throw NPE? the rest of the setXYZ methods respect null by setting the default. I don't like that setMP is not consistent with the rest of the setters.
        Hide
        Adrien Grand added a comment -

        Good point. I copied the behavior of setCodec which throws a NPE although you are right that most methods seem to set the default value...

        Show
        Adrien Grand added a comment - Good point. I copied the behavior of setCodec which throws a NPE although you are right that most methods seem to set the default value...
        Hide
        Shai Erera added a comment -

        I think that setCodec should also be modified then, to something like that:

        if (codec == null) {
            codec = Codec.getDefault();
            if (codec == null) {
              throw new NullPointerException();
            }
        }
        

        Just like the ctor does. And the NPE should include some message too (both in the ctor and setter).

        Show
        Shai Erera added a comment - I think that setCodec should also be modified then, to something like that: if (codec == null ) { codec = Codec.getDefault(); if (codec == null ) { throw new NullPointerException(); } } Just like the ctor does. And the NPE should include some message too (both in the ctor and setter).
        Hide
        Adrien Grand added a comment -

        I'm not sure I like the fact that passing null to setXXX actually sets the default value, what do other committers think?

        Show
        Adrien Grand added a comment - I'm not sure I like the fact that passing null to setXXX actually sets the default value, what do other committers think?
        Hide
        Robert Muir added a comment -

        it seems more likely that someone would pass null due to a program mistake, rather than
        wanting the defaults.

        Show
        Robert Muir added a comment - it seems more likely that someone would pass null due to a program mistake, rather than wanting the defaults.
        Hide
        Adrien Grand added a comment -

        My point is that if someone wants to use the default value, all he has to do is to never call the setter? Moreover users can't pass null to methods that expect primitive types (such as setMaxBufferedDocs) so throwing an exception when encountering null would be more consistent?

        Show
        Adrien Grand added a comment - My point is that if someone wants to use the default value, all he has to do is to never call the setter? Moreover users can't pass null to methods that expect primitive types (such as setMaxBufferedDocs) so throwing an exception when encountering null would be more consistent?
        Hide
        Shai Erera added a comment -

        We need to be consistent across the setters. We throw IllegalArg in the other setters (which take primitives), so maybe throw that and not NPE?

        Show
        Shai Erera added a comment - We need to be consistent across the setters. We throw IllegalArg in the other setters (which take primitives), so maybe throw that and not NPE?
        Hide
        Adrien Grand added a comment -

        We throw IllegalArg in the other setters (which take primitives), so maybe throw that and not NPE?

        +1 I'll update the patch.

        Show
        Adrien Grand added a comment - We throw IllegalArg in the other setters (which take primitives), so maybe throw that and not NPE? +1 I'll update the patch.
        Hide
        Adrien Grand added a comment -

        Updated patch. IndexWriterConfig.setXXX methods now throw an IllegalArgumentException when passed null instead of setting the default value. Tests pass.

        Show
        Adrien Grand added a comment - Updated patch. IndexWriterConfig.setXXX methods now throw an IllegalArgumentException when passed null instead of setting the default value. Tests pass.
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Adrien Grand
        http://svn.apache.org/viewvc?view=revision&revision=1456564

        LUCENE-4833: Make IndexWriterConfig setters throw an exception when passed null if null is not a valid value.

        Show
        Commit Tag Bot added a comment - [trunk commit] Adrien Grand http://svn.apache.org/viewvc?view=revision&revision=1456564 LUCENE-4833 : Make IndexWriterConfig setters throw an exception when passed null if null is not a valid value.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Adrien Grand
        http://svn.apache.org/viewvc?view=revision&revision=1456567

        LUCENE-4833: Make IndexWriterConfig setters throw an exception when passed null if null is not a valid value (merged from r1456564).

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Adrien Grand http://svn.apache.org/viewvc?view=revision&revision=1456567 LUCENE-4833 : Make IndexWriterConfig setters throw an exception when passed null if null is not a valid value (merged from r1456564).

          People

          • Assignee:
            Adrien Grand
            Reporter:
            Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development