Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-4452

Hunspell stemmer should not merge duplicate dictionary entries

    Details

      Description

      Hunspell dictionaries are on the form

      lucene/ABC
      mahout/X
      

      Each word is listed once with is base form, and the flags after the / define allowed prefixes and suffixes.

      In HunspellDictionary's parsing logic, if the same base word should appear multiple times in the file, the flags from the duplicate entry are added to the flags from the existing entry.

      However, HunSpellStemFilterFactory allows for a comma-separated list of dictionary files to be passed in, the idea being that you can have your own custom extensions and not need to modify the "standard" ones which may change upstream once in a while. This feature now works only for NEW words, not for overriding existing entries from the first dictionary.

      Would like to change this behavior, so that the last line read overwrites any previous one. This will both fix the custom dictionary issue and also fix unintentional wrong original dictionaries, where someone added a word definition at the end without realizing there was another already.

      For the en_UK.dic there are no duplicates. For en_US.dic there is one duplicate, so I argue this behavior is a bug and not a feature dictionary authors depend upon.

      1. SOLR-4452.patch
        4 kB
        Jan Høydahl
      2. SOLR-4452.patch
        0.7 kB
        Jan Høydahl
      3. SOLR-4452-caseinsensitivemerge.patch
        2 kB
        Jan Høydahl
      4. SOLR-4452-caseinsensitive-nomerge.patch
        3 kB
        Jan Høydahl

        Activity

        Hide
        steve_rowe Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        steve_rowe Steve Rowe added a comment - Bulk close resolved 4.4 issues
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1499180 from janhoy@apache.org
        [ https://svn.apache.org/r1499180 ]

        SOLR-4452: Fix test for case insensitive mode

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1499180 from janhoy@apache.org [ https://svn.apache.org/r1499180 ] SOLR-4452 : Fix test for case insensitive mode
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1499179 from janhoy@apache.org
        [ https://svn.apache.org/r1499179 ]

        SOLR-4452: Fix test for case insensitive mode

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1499179 from janhoy@apache.org [ https://svn.apache.org/r1499179 ] SOLR-4452 : Fix test for case insensitive mode
        Hide
        janhoy Jan Høydahl added a comment -

        Changed my mind. This patch treats lower/upper entries the same in caseInsensitive mode, i.e. the last one wins. So we modify the tests instead. Will commit shortly.

        Show
        janhoy Jan Høydahl added a comment - Changed my mind. This patch treats lower/upper entries the same in caseInsensitive mode, i.e. the last one wins. So we modify the tests instead. Will commit shortly.
        Hide
        janhoy Jan Høydahl added a comment -

        This extra patch takes care of this corner case where two lines are same word but different case.

        For case sensitive dictionary we will treat these as two distinct words. But for case insensitive dictionaries we'll merge them as before.

        Sounds good? Or would it be better to treat these also as duplicates and use only the last one? If so, we should modify the unit test instead.

        Show
        janhoy Jan Høydahl added a comment - This extra patch takes care of this corner case where two lines are same word but different case. For case sensitive dictionary we will treat these as two distinct words. But for case insensitive dictionaries we'll merge them as before. Sounds good? Or would it be better to treat these also as duplicates and use only the last one? If so, we should modify the unit test instead.
        Hide
        janhoy Jan Høydahl added a comment -

        Yup, the test for case insensitive dictionary assumes that entries are merged. Not sure what is the best action.

        Show
        janhoy Jan Høydahl added a comment - Yup, the test for case insensitive dictionary assumes that entries are merged. Not sure what is the best action.
        Hide
        rcmuir Robert Muir added a comment -

        This commit is causing test failures.

        Show
        rcmuir Robert Muir added a comment - This commit is causing test failures.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1499166 from janhoy@apache.org
        [ https://svn.apache.org/r1499166 ]

        SOLR-4452: Hunspell stemmer should not merge duplicate dictionary entries (merge from trunk)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1499166 from janhoy@apache.org [ https://svn.apache.org/r1499166 ] SOLR-4452 : Hunspell stemmer should not merge duplicate dictionary entries (merge from trunk)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1499164 from janhoy@apache.org
        [ https://svn.apache.org/r1499164 ]

        SOLR-4452: Hunspell stemmer should not merge duplicate dictionary entries

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1499164 from janhoy@apache.org [ https://svn.apache.org/r1499164 ] SOLR-4452 : Hunspell stemmer should not merge duplicate dictionary entries
        Hide
        janhoy Jan Høydahl added a comment -

        Think this is ready. Will commit in a day or two unless anyone proves that this is a needed "feature", not a bug.

        Show
        janhoy Jan Høydahl added a comment - Think this is ready. Will commit in a day or two unless anyone proves that this is a needed "feature", not a bug.
        Hide
        janhoy Jan Høydahl added a comment -

        New patch with testcase

        Show
        janhoy Jan Høydahl added a comment - New patch with testcase
        Hide
        janhoy Jan Høydahl added a comment -

        Initial patch

        Show
        janhoy Jan Høydahl added a comment - Initial patch

          People

          • Assignee:
            janhoy Jan Høydahl
            Reporter:
            janhoy Jan Høydahl
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development