Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.9, 4.9.1, 4.10, 4.10.1
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:

      Description

      When using AnalyzingInfixLookupFactory suggestions always return with the match term as highlighted and 'allTermsRequired' is always set to true.

      We should be able to configure those.

      Steps to reproduce -

      schema additions

      <searchComponent name="suggest" class="solr.SuggestComponent">
          <lst name="suggester">
            <str name="name">mySuggester</str>
            <str name="lookupImpl">AnalyzingInfixLookupFactory</str>
            <str name="dictionaryImpl">DocumentDictionaryFactory</str> 
            <str name="field">suggestField</str>
            <str name="weightField">weight</str>
            <str name="suggestAnalyzerFieldType">textSuggest</str>
          </lst>
        </searchComponent>
      
        <requestHandler name="/suggest" class="solr.SearchHandler" startup="lazy">
          <lst name="defaults">
            <str name="suggest">true</str>
            <str name="suggest.count">10</str>
          </lst>
          <arr name="components">
            <str>suggest</str>
          </arr>
        </requestHandler>
      

      solrconfig changes -

      <fieldType class="solr.TextField" name="textSuggest" positionIncrementGap="100">
         <analyzer>
            <tokenizer class="solr.StandardTokenizerFactory"/>
            <filter class="solr.StandardFilterFactory"/>
            <filter class="solr.LowerCaseFilterFactory"/>
         </analyzer>
        </fieldType>
      
      
         <field name="suggestField" type="textSuggest" indexed="true" stored="true"/>
      

      Add 3 documents -

      curl http://localhost:8983/solr/update/json?commit=true -H 'Content-type:application/json' -d '
      [ {"id" : "1", "suggestField" : "bass fishing"}, {"id" : "2", "suggestField" : "sea bass"}, {"id" : "3", "suggestField" : "sea bass fishing"} ]
      '
      

      Query -

      http://localhost:8983/solr/collection1/suggest?suggest.build=true&suggest.dictionary=mySuggester&q=bass&wt=json&indent=on
      

      Response

      {
        "responseHeader":{
          "status":0,
          "QTime":25},
        "command":"build",
        "suggest":{"mySuggester":{
            "bass":{
              "numFound":3,
              "suggestions":[{
                  "term":"<b>bass</b> fishing",
                  "weight":0,
                  "payload":""},
                {
                  "term":"sea <b>bass</b>",
                  "weight":0,
                  "payload":""},
                {
                  "term":"sea <b>bass</b> fishing",
                  "weight":0,
                  "payload":""}]}}}}
      

      The problem is in SolrSuggester Line 200 where we say lookup.lookup()

      This constructor does not take allTermsRequired and doHighlight since it's only tuneable to AnalyzingInfixSuggester and not the other lookup implementations.

      If different Lookup implementations have different params as their constructors, these sort of issues will always keep happening. Maybe we should not keep it generic and do instanceof checks and set params accordingly?

      1. SOLR-6648.patch
        14 kB
        Boon Low
      2. SOLR-6648-v4.10.3.patch
        4 kB
        Boon Low

        Issue Links

          Activity

          Hide
          Boon Low added a comment -

          Suggestion highlighting and "allTermsRequired" options are currently hardwired in Lucene - see true, true in the lookup method in AnalyzingInfixSuggester.java below:

          public List<LookupResult> lookup(CharSequence key, Set<BytesRef> contexts, boolean onlyMorePopular, int num) throws IOException {
              return lookup(key, contexts, num, true, true);
          }
          
          /** Lookup, without any context. */
          public List<LookupResult> lookup(CharSequence key, int num, boolean allTermsRequired, boolean doHighlight) throws IOException {
              return lookup(key, null, num, allTermsRequired, doHighlight);
            }
          

          The lookup method (proper) does support the options, e.g. creating the appropriate Boolean lookup clauses based on "allTermsRequired": BooleanClause.Occur.MUST vs. BooleanClause.Occur.SHOULD.

          Show
          Boon Low added a comment - Suggestion highlighting and "allTermsRequired" options are currently hardwired in Lucene - see true , true in the lookup method in AnalyzingInfixSuggester.java below: public List<LookupResult> lookup(CharSequence key, Set<BytesRef> contexts, boolean onlyMorePopular, int num) throws IOException { return lookup(key, contexts, num, true , true ); } /** Lookup, without any context. */ public List<LookupResult> lookup(CharSequence key, int num, boolean allTermsRequired, boolean doHighlight) throws IOException { return lookup(key, null , num, allTermsRequired, doHighlight); } The lookup method (proper) does support the options, e.g. creating the appropriate Boolean lookup clauses based on "allTermsRequired": BooleanClause.Occur.MUST vs. BooleanClause.Occur.SHOULD.
          Hide
          Boon Low added a comment - - edited

          I have created a patche for this, in Lucene and Solr, so that highlighting and the Boolean matching clause can be configured in solrconfig.xml, for BlendedInfixSuggester and AnalyzingInfixSuggester:

             <lst name="suggester">
                  <str name="name">..</str>
                  <str name="lookupImpl">BlendedInfixLookupFactory</str>
                  <str name="dictionaryImpl">DocumentDictionaryFactory</str>
                   ...
                  <str name="allTermsRequired">false</str>
                  <str name="highlight">true</str>
               </lst>
          

          If not configured, both 'highlighting' and 'allTermsRequired' default to *true.

          Show
          Boon Low added a comment - - edited I have created a patche for this, in Lucene and Solr, so that highlighting and the Boolean matching clause can be configured in solrconfig.xml, for BlendedInfixSuggester and AnalyzingInfixSuggester : <lst name= "suggester" > <str name= "name" > .. </str> <str name= "lookupImpl" > BlendedInfixLookupFactory </str> <str name= "dictionaryImpl" > DocumentDictionaryFactory </str> ... <str name= "allTermsRequired" > false </str> <str name= "highlight" > true </str> </lst> If not configured, both 'highlighting' and 'allTermsRequired' default to *true.
          Hide
          Boon Low added a comment -

          Patch submitted. Also added a couple of unit tests for allTermsRequired=false.

          Since the patch involves Lucene suggesters (AnalyzingInfixSuggester, BlendedInfixSuggester) and the corresponding factories in Solr, do we need to open a Lucene issue?

          Please change the title of this issue to something cf. "Infix suggesters highlighting and allTermsRequired configuration" to reflect to the scope of patch.

          Show
          Boon Low added a comment - Patch submitted. Also added a couple of unit tests for allTermsRequired=false. Since the patch involves Lucene suggesters (AnalyzingInfixSuggester, BlendedInfixSuggester) and the corresponding factories in Solr, do we need to open a Lucene issue? Please change the title of this issue to something cf. "Infix suggesters highlighting and allTermsRequired configuration" to reflect to the scope of patch.
          Hide
          Tomás Fernández Löbbe added a comment -

          I think this makes sense. It sounds like it would be more common for people to always want suggestions with/without highlight and with/without "allTermsRequired" than the need to specify it per request.
          I think it would be better to split the Lucene part to a LUCENE jira.

          Show
          Tomás Fernández Löbbe added a comment - I think this makes sense. It sounds like it would be more common for people to always want suggestions with/without highlight and with/without "allTermsRequired" than the need to specify it per request. I think it would be better to split the Lucene part to a LUCENE jira.
          Hide
          Boon Low added a comment - - edited

          I have opened LUCENE-6149 and submitted a Lucene patch to the issue.

          Show
          Boon Low added a comment - - edited I have opened LUCENE-6149 and submitted a Lucene patch to the issue.
          Hide
          Boon Low added a comment -

          patch updated: Solr-only patch, w.r.t. trunk 05/01/15

          Show
          Boon Low added a comment - patch updated: Solr-only patch, w.r.t. trunk 05/01/15
          Hide
          Boon Low added a comment -

          patch updated in accordance with LUCENE-6149 and for v.4.10.3

          Show
          Boon Low added a comment - patch updated in accordance with LUCENE-6149 and for v.4.10.3
          Hide
          Tomás Fernández Löbbe added a comment -

          Thanks for the patch Boon Low, would you add a test case?

          Show
          Tomás Fernández Löbbe added a comment - Thanks for the patch Boon Low , would you add a test case?
          Hide
          Boon Low added a comment -

          Here is a patch w.r.t. trunk 08/01/15

          Show
          Boon Low added a comment - Here is a patch w.r.t. trunk 08/01/15
          Hide
          Boon Low added a comment -

          Hi Tomás, will look into the test case at the weekend, my lucene-solr trunk wouldn't compile for some reason. 'ivy:retrieve' dependencies problems..

          Show
          Boon Low added a comment - Hi Tomás, will look into the test case at the weekend, my lucene-solr trunk wouldn't compile for some reason. 'ivy:retrieve' dependencies problems..
          Hide
          Boon Low added a comment - - edited

          Hey Tomás, at last here is a new patch (w.r.t. trunk 14/01/15) containing unit tests. Instead of creating a new test case, I have updated TestAnalyzeInfixSuggestions' single and multiple tests with additional tests based on the new SolrSuggester (cf. Suggester) in default allTermsRequired, highlight config settings (true), plus 2 new tests for allTermsRequired=false, highlight=false scenarios.

          Show
          Boon Low added a comment - - edited Hey Tomás, at last here is a new patch (w.r.t. trunk 14/01/15) containing unit tests. Instead of creating a new test case, I have updated TestAnalyzeInfixSuggestions ' single and multiple tests with additional tests based on the new SolrSuggester (cf. Suggester) in default allTermsRequired, highlight config settings (true), plus 2 new tests for allTermsRequired=false , highlight=false scenarios.
          Hide
          Tomás Fernández Löbbe added a comment -

          Sorry for the delay Boon Low, the patch looks good. I'll commit shortly

          Show
          Tomás Fernández Löbbe added a comment - Sorry for the delay Boon Low , the patch looks good. I'll commit shortly
          Hide
          ASF subversion and git services added a comment -

          Commit 1657655 from Tomás Fernández Löbbe in branch 'dev/trunk'
          [ https://svn.apache.org/r1657655 ]

          SOLR-6648: Add support for highlight and allTermsRequired configuration in AnalyzingInfix and BlendedInfix Solr suggesters

          Show
          ASF subversion and git services added a comment - Commit 1657655 from Tomás Fernández Löbbe in branch 'dev/trunk' [ https://svn.apache.org/r1657655 ] SOLR-6648 : Add support for highlight and allTermsRequired configuration in AnalyzingInfix and BlendedInfix Solr suggesters
          Hide
          ASF subversion and git services added a comment -

          Commit 1657671 from Tomás Fernández Löbbe in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1657671 ]

          SOLR-6648: Add support for highlight and allTermsRequired configuration in AnalyzingInfix and BlendedInfix Solr suggesters

          Show
          ASF subversion and git services added a comment - Commit 1657671 from Tomás Fernández Löbbe in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1657671 ] SOLR-6648 : Add support for highlight and allTermsRequired configuration in AnalyzingInfix and BlendedInfix Solr suggesters
          Hide
          Tomás Fernández Löbbe added a comment -

          Thanks Boon and Varun!

          Show
          Tomás Fernández Löbbe added a comment - Thanks Boon and Varun!
          Hide
          Boon Low added a comment -

          Glad to see this committed into the code base. Thanks Tomás and Varun.

          Show
          Boon Low added a comment - Glad to see this committed into the code base. Thanks Tomás and Varun.
          Hide
          Timothy Potter added a comment -

          Bulk close after 5.1 release

          Show
          Timothy Potter added a comment - Bulk close after 5.1 release
          Hide
          Arcadius Ahouansou added a comment -

          Would be nice if highlight was a query-time parameter where clients could choose whether they want it or not.

          Show
          Arcadius Ahouansou added a comment - Would be nice if highlight was a query-time parameter where clients could choose whether they want it or not.

            People

            • Assignee:
              Tomás Fernández Löbbe
              Reporter:
              Varun Thacker
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development