Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2
    • Component/s: search
    • Labels:
      None

      Description

      This tracks the creation of a patch that adds the n-gram/edge n-gram tokenizing functionality that was initially part of SOLR-81 (spell checking). This was taken out b/c the lucene SpellChecker class removed this dependency. None-the-less, I think this is useful functionality and the addition is trivial. How does everyone feel about such an addition?

      1. SOLR-81-ngram.patch
        5 kB
        Adam Hiatt
      2. SOLR-199-n-gram.patch
        5 kB
        Adam Hiatt

        Activity

        Hoss Man made changes -
        Fix Version/s 1.2 [ 12312235 ]
        Hide
        Hoss Man added a comment -

        This bug was modified as part of a bulk update using the criteria...

        • Marked ("Resolved" or "Closed") and "Fixed"
        • Had no "Fix Version" versions
        • Was listed in the CHANGES.txt for 1.2

        The Fix Version for all 39 issues found was set to 1.2, email notification
        was suppressed to prevent excessive email.

        For a list of all the issues modified, search jira comments for this
        (hopefully) unique string: 20080415hossman2

        Show
        Hoss Man added a comment - This bug was modified as part of a bulk update using the criteria... Marked ("Resolved" or "Closed") and "Fixed" Had no "Fix Version" versions Was listed in the CHANGES.txt for 1.2 The Fix Version for all 39 issues found was set to 1.2, email notification was suppressed to prevent excessive email. For a list of all the issues modified, search jira comments for this (hopefully) unique string: 20080415hossman2
        Yonik Seeley made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Hide
        Yonik Seeley added a comment -

        Thanks Adam, I just committed this.

        Show
        Yonik Seeley added a comment - Thanks Adam, I just committed this.
        Adam Hiatt made changes -
        Attachment SOLR-199-n-gram.patch [ 12356578 ]
        Hide
        Adam Hiatt added a comment -

        This is the new patch, not just cut out of SOLR-81...

        I removed references to the Base class and fixed the edge n-gram bug.

        Show
        Adam Hiatt added a comment - This is the new patch, not just cut out of SOLR-81 ... I removed references to the Base class and fixed the edge n-gram bug.
        Hide
        Adam Hiatt added a comment -

        I'll make those changes. I agree that we don't want bloated base classes.

        > NGramTokenizerFactory is refering to constants from, and constructing an instance of, EdgeNGramTokenizer
        Are you saying that this worries you b/c it is referenced in the example schema and will thus break without the lucene-analyzers package? I do agree that this example should probably be taken out for the time being (at the least).

        Show
        Adam Hiatt added a comment - I'll make those changes. I agree that we don't want bloated base classes. > NGramTokenizerFactory is refering to constants from, and constructing an instance of, EdgeNGramTokenizer Are you saying that this worries you b/c it is referenced in the example schema and will thus break without the lucene-analyzers package? I do agree that this example should probably be taken out for the time being (at the least).
        Hide
        Hoss Man added a comment -

        NGramTokenizerFactory is refering to constants from, and constructing an instance of, EdgeNGramTokenizer

        I'm also not crazy about some of the utilities being added to BaseTokenizerFactory .. at a minimum they need better names (like getStringArg) but i'm not really clear on what this is suppose to mean at all...

        protected int getInt(String name, int defaultVal, boolean useDefault)

        ...if i don't want to use the default, then what am i suppose to pass as the defaultVal?

        how about if we don't make any changes to BaseTokenizerFactory and just let subclasses that want convenience methods for dealing with args use MapSolrParams and the methods it supports?

        Show
        Hoss Man added a comment - NGramTokenizerFactory is refering to constants from, and constructing an instance of, EdgeNGramTokenizer I'm also not crazy about some of the utilities being added to BaseTokenizerFactory .. at a minimum they need better names (like getStringArg) but i'm not really clear on what this is suppose to mean at all... protected int getInt(String name, int defaultVal, boolean useDefault) ...if i don't want to use the default, then what am i suppose to pass as the defaultVal? how about if we don't make any changes to BaseTokenizerFactory and just let subclasses that want convenience methods for dealing with args use MapSolrParams and the methods it supports?
        Hide
        Otis Gospodnetic added a comment -

        +1 for getting this stuff into Solr then. I imagine the patch is mostly what was in some of the SOLR-81 patches.
        I think I saw Yonik mentioning cleaning up either solrconfig or schema, so that's something to keep in mind when applying this patch.

        Show
        Otis Gospodnetic added a comment - +1 for getting this stuff into Solr then. I imagine the patch is mostly what was in some of the SOLR-81 patches. I think I saw Yonik mentioning cleaning up either solrconfig or schema, so that's something to keep in mind when applying this patch.
        Hide
        Adam Hiatt added a comment -

        Quoted:
        Adam, I think Yonik is just saying that the n-gram stuff I added to Lucene's contrib/analyzers was added after 2.1 was released, so we'd need a version of that jar from the trunk at this time. I see mentions of Solr 1.2, so perhaps we can grab the 2.2-dev version of that jar and add it to Solr starting with release 1.2?

        Understood. I talked with Yonik and he mentioned possibly upgrading to a lucene 2.2-dev in the future. I'm not sure he intended that to happen in time for solr 1.2 however. I suppose if it came to it, we could probably use the analyzers 2.2-dev with 2.1 core. I'm guessing the API was stable, but I'm not sure if we want to complicate things that much.

        Quoted:
        Question: How will the spellchecker you are writing or considering writing going to be different/better than the one in contrib/spellchecker?

        The initial use case was actually to support autocomplete functionality. IE using the start n-gramming functionality to build tokens that we can match term fragments upon.

        However, I do still plan to write a native Solr spell checker based on this same patch sometime in the future. The major improvements with a native system are several fold. First, it allows for truly native use of a Solr-configurable lucene index. Second, we will be able to take advantage of native Solr caching. Third, we will be able to boost on arbitrary aspects. For example, take the misspelling 'ipad' and the indexed terms 'ipod' and 'ipaq'. Both the indexed terms are the same edit distance away from the misspelling. They also have the same number of 2 grams (though not 3 grams). If find that 'ipod' is the more valuable term we can boost slightly based on its popularity and draw out ahead. The final big win is the ability to spell check on individual input tokens. For example, assume that we have the term 'ipod' indexed in our spell checker, but not the term 'apple ipod' and the misspelling 'apple ipdo' is entered. The overlap between 'ipod' and 'apple ipdo' is slight enough to not warrant a suggestion. However if we tokenize on white space and spell correct on each token we would be able to catch the 'ipdo' misspelling. I'm sure there are other use cases, but those are the ones that I've identified.

        Show
        Adam Hiatt added a comment - Quoted: Adam, I think Yonik is just saying that the n-gram stuff I added to Lucene's contrib/analyzers was added after 2.1 was released, so we'd need a version of that jar from the trunk at this time. I see mentions of Solr 1.2, so perhaps we can grab the 2.2-dev version of that jar and add it to Solr starting with release 1.2? Understood. I talked with Yonik and he mentioned possibly upgrading to a lucene 2.2-dev in the future. I'm not sure he intended that to happen in time for solr 1.2 however. I suppose if it came to it, we could probably use the analyzers 2.2-dev with 2.1 core. I'm guessing the API was stable, but I'm not sure if we want to complicate things that much. Quoted: Question: How will the spellchecker you are writing or considering writing going to be different/better than the one in contrib/spellchecker? The initial use case was actually to support autocomplete functionality. IE using the start n-gramming functionality to build tokens that we can match term fragments upon. However, I do still plan to write a native Solr spell checker based on this same patch sometime in the future. The major improvements with a native system are several fold. First, it allows for truly native use of a Solr-configurable lucene index. Second, we will be able to take advantage of native Solr caching. Third, we will be able to boost on arbitrary aspects. For example, take the misspelling 'ipad' and the indexed terms 'ipod' and 'ipaq'. Both the indexed terms are the same edit distance away from the misspelling. They also have the same number of 2 grams (though not 3 grams). If find that 'ipod' is the more valuable term we can boost slightly based on its popularity and draw out ahead. The final big win is the ability to spell check on individual input tokens. For example, assume that we have the term 'ipod' indexed in our spell checker, but not the term 'apple ipod' and the misspelling 'apple ipdo' is entered. The overlap between 'ipod' and 'apple ipdo' is slight enough to not warrant a suggestion. However if we tokenize on white space and spell correct on each token we would be able to catch the 'ipdo' misspelling. I'm sure there are other use cases, but those are the ones that I've identified.
        Hide
        Otis Gospodnetic added a comment -

        Adam, I think Yonik is just saying that the n-gram stuff I added to Lucene's contrib/analyzers was added after 2.1 was released, so we'd need a version of that jar from the trunk at this time. I see mentions of Solr 1.2, so perhaps we can grab the 2.2-dev version of that jar and add it to Solr starting with release 1.2?

        Question: How will the spellchecker you are writing or considering writing going to be different/better than the one in contrib/spellchecker?

        Show
        Otis Gospodnetic added a comment - Adam, I think Yonik is just saying that the n-gram stuff I added to Lucene's contrib/analyzers was added after 2.1 was released, so we'd need a version of that jar from the trunk at this time. I see mentions of Solr 1.2, so perhaps we can grab the 2.2-dev version of that jar and add it to Solr starting with release 1.2? Question: How will the spellchecker you are writing or considering writing going to be different/better than the one in contrib/spellchecker?
        Hide
        Adam Hiatt added a comment -

        Were you imagining that it just be included and that the users must use include a lucene 2.2+ analyzers jar themselves?

        Show
        Adam Hiatt added a comment - Were you imagining that it just be included and that the users must use include a lucene 2.2+ analyzers jar themselves?
        Hide
        Yonik Seeley added a comment -

        This patch does rely on analyzers.jar from lucene contrib. No reason that jar shouldn't be part of Solr, but it needs a version after Lucene 2.1

        Show
        Yonik Seeley added a comment - This patch does rely on analyzers.jar from lucene contrib. No reason that jar shouldn't be part of Solr, but it needs a version after Lucene 2.1
        Hide
        Yonik Seeley added a comment -

        Since there is no impact or even memory overhead if unused, and just a teeny bit of disk overhead, this patch looks fine to me.

        Show
        Yonik Seeley added a comment - Since there is no impact or even memory overhead if unused, and just a teeny bit of disk overhead, this patch looks fine to me.
        Hide
        Adam Hiatt added a comment -

        I do in fact have a need. I am creating an edge n-gram index to provide auto-complete functionality. This doesn't require the regular n-gram factory, but I put it in the patch as well. Furthermore, I am also planning to create a alternative spellchecker that does what you wanted to do initially (ie have a single spellchecking index). This would of course require n-gramming functionality.

        Show
        Adam Hiatt added a comment - I do in fact have a need. I am creating an edge n-gram index to provide auto-complete functionality. This doesn't require the regular n-gram factory, but I put it in the patch as well. Furthermore, I am also planning to create a alternative spellchecker that does what you wanted to do initially (ie have a single spellchecking index). This would of course require n-gramming functionality.
        Hide
        Otis Gospodnetic added a comment -

        While I was the one who started that n-gram approach, and while I'm saving code and config changes from that work "just in case", I'm not sure why we'd add it to Solr at this point, if nothing is going to use it. I'd say keep the patch here, so one can easily put that back into Solr when a need arises, but don't commit it unless there is some immediate need. Do you have something that needs the n-gram stuff in Solr?

        Show
        Otis Gospodnetic added a comment - While I was the one who started that n-gram approach, and while I'm saving code and config changes from that work "just in case", I'm not sure why we'd add it to Solr at this point, if nothing is going to use it. I'd say keep the patch here, so one can easily put that back into Solr when a need arises, but don't commit it unless there is some immediate need. Do you have something that needs the n-gram stuff in Solr?
        Adam Hiatt made changes -
        Field Original Value New Value
        Attachment SOLR-81-ngram.patch [ 12354473 ]
        Hide
        Adam Hiatt added a comment -

        Here is the patch.

        Show
        Adam Hiatt added a comment - Here is the patch.
        Adam Hiatt created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Adam Hiatt
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development