Lucene - Core
  1. Lucene - Core
  2. LUCENE-6464

Allow possibility to group contexts in AnalyzingInfixSuggester.loockup()

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.1
    • Fix Version/s: 5.2, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This is an enhancement to LUCENE-6050
      LUCENE-6050 added

      lookup(CharSequence key, Map<BytesRef, BooleanClause.Occur> contextInfo, int num, boolean allTermsRequired, boolean doHighlight)
      

      which allowed to do something like

      (A OR B AND C OR D ...)

      In our use-case, we realise that we need grouping i.e
      (A OR B) AND (C OR D) AND (...)

      In other words, we need the intersection of multiple contexts.

      The attached patch allows to pass in a varargs of map, each one representing the each group. Looks a bit heavy IMHO.

      This is an initial patch.

      The question to Michael McCandless and jane chang is:
      is it better to expose a FilteredQuery/Query and let the user build their own query instead of passing a map?

      Exposing a filteredQuery will probably give the best flexibility to the end-users.

      1. LUCENE-6464.patch
        11 kB
        Arcadius Ahouansou

        Issue Links

          Activity

          Hide
          Arcadius Ahouansou added a comment -

          initial patch

          Show
          Arcadius Ahouansou added a comment - initial patch
          Hide
          Michael McCandless added a comment -

          Arcadius Ahouansou +1 to just accept a BooleanQuery for the context? But can we keep the current method (that takes Map<key,Occur>) and just add a new expert method taking an arbitrary BooleanQuery?

          Show
          Michael McCandless added a comment - Arcadius Ahouansou +1 to just accept a BooleanQuery for the context? But can we keep the current method (that takes Map<key,Occur>) and just add a new expert method taking an arbitrary BooleanQuery?
          Hide
          Arcadius Ahouansou added a comment -

          added lookup with BooleanQuery as a filter

          Show
          Arcadius Ahouansou added a comment - added lookup with BooleanQuery as a filter
          Hide
          Arcadius Ahouansou added a comment -

          Added lookup method with flexible filtering by BooleanQuery

          Show
          Arcadius Ahouansou added a comment - Added lookup method with flexible filtering by BooleanQuery
          Hide
          Arcadius Ahouansou added a comment -

          Hello Michael McCandless
          Please have a look at the new patch when you have the chance.

          I have now added

          public List<LookupResult> lookup(CharSequence key, BooleanQuery contextQuery, int num, boolean allTermsRequired, boolean doHighlight)
          
          Show
          Arcadius Ahouansou added a comment - Hello Michael McCandless Please have a look at the new patch when you have the chance. I have now added public List<LookupResult> lookup(CharSequence key, BooleanQuery contextQuery, int num, boolean allTermsRequired, boolean doHighlight)
          Hide
          Michael McCandless added a comment -

          Thanks Arcadius Ahouansou, this looks great!

          Maybe rename .addToQuery -> .addContextToQuery?

          In the javadoc for the new uber-expert lookup method, can you link to #addContextToQuery as the way to build up the contextQuery arg?

          Show
          Michael McCandless added a comment - Thanks Arcadius Ahouansou , this looks great! Maybe rename .addToQuery -> .addContextToQuery? In the javadoc for the new uber-expert lookup method, can you link to #addContextToQuery as the way to build up the contextQuery arg?
          Hide
          Arcadius Ahouansou added a comment -

          Thanks Michael McCandless for the review.
          Will do those minor changes today.

          Please, have you also noticed the TODO and comments about the usage of

          MatchAllDocsQuery()

          that I have introduced?

          Thanks.

          Show
          Arcadius Ahouansou added a comment - Thanks Michael McCandless for the review. Will do those minor changes today. Please, have you also noticed the TODO and comments about the usage of MatchAllDocsQuery() that I have introduced? Thanks.
          Hide
          Michael McCandless added a comment -

          Please, have you also noticed the TODO

          Oh I missed that, thanks for pointing it out.

          Maybe we should fix the lowest level lookup method to do this "all MUST_NOT" check? I.e., it would handle taking the clauses of the sub-query and moving them up to the main query? This is also cleaner for users, i.e. they should not be expected to know about this "all MUST_NOT" case. And then toQuery doesn't need to handle it ...

          Anyway, I think the : should be better (faster) than doing context:, because :* doesn't visit any postings. Also, the user may not have indexed any contexts ...

          Show
          Michael McCandless added a comment - Please, have you also noticed the TODO Oh I missed that, thanks for pointing it out. Maybe we should fix the lowest level lookup method to do this "all MUST_NOT" check? I.e., it would handle taking the clauses of the sub-query and moving them up to the main query? This is also cleaner for users, i.e. they should not be expected to know about this "all MUST_NOT" case. And then toQuery doesn't need to handle it ... Anyway, I think the : should be better (faster) than doing context: , because :* doesn't visit any postings. Also, the user may not have indexed any contexts ...
          Hide
          Arcadius Ahouansou added a comment -

          Updated patch.

          • Renamed addToQuery()->addContextToQuery()
          • Added reference to addContextToQuery() in the javadoc of the lookup method that takes a contextQuery as parameter
          • Removed the usage of MatchAllDocsQuery() and moved the all MUST_NOT logic from toQuery() into the main lookup() method
          Show
          Arcadius Ahouansou added a comment - Updated patch. Renamed addToQuery()->addContextToQuery() Added reference to addContextToQuery() in the javadoc of the lookup method that takes a contextQuery as parameter Removed the usage of MatchAllDocsQuery() and moved the all MUST_NOT logic from toQuery() into the main lookup() method
          Hide
          Michael McCandless added a comment -

          Thanks Arcadius Ahouansou, new patch looks great, I'll commit shortly!

          Show
          Michael McCandless added a comment - Thanks Arcadius Ahouansou , new patch looks great, I'll commit shortly!
          Hide
          ASF subversion and git services added a comment -

          Commit 1679579 from Michael McCandless in branch 'dev/trunk'
          [ https://svn.apache.org/r1679579 ]

          LUCENE-6464: add expert AnalyzingInfixSuggester.lookup method that accepts arbitrary BooleanQuery to filter contexts

          Show
          ASF subversion and git services added a comment - Commit 1679579 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1679579 ] LUCENE-6464 : add expert AnalyzingInfixSuggester.lookup method that accepts arbitrary BooleanQuery to filter contexts
          Hide
          ASF subversion and git services added a comment -

          Commit 1679586 from Michael McCandless in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1679586 ]

          LUCENE-6464: add expert AnalyzingInfixSuggester.lookup method that accepts arbitrary BooleanQuery to filter contexts

          Show
          ASF subversion and git services added a comment - Commit 1679586 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1679586 ] LUCENE-6464 : add expert AnalyzingInfixSuggester.lookup method that accepts arbitrary BooleanQuery to filter contexts
          Hide
          Michael McCandless added a comment -
          Show
          Michael McCandless added a comment - Thanks Arcadius Ahouansou !
          Hide
          Arcadius Ahouansou added a comment -

          Thank you very much Michael McCandless for finding the time to help fix this issue

          Show
          Arcadius Ahouansou added a comment - Thank you very much Michael McCandless for finding the time to help fix this issue
          Hide
          Anshum Gupta added a comment -

          Bulk close for 5.2.0.

          Show
          Anshum Gupta added a comment - Bulk close for 5.2.0.

            People

            • Assignee:
              Unassigned
              Reporter:
              Arcadius Ahouansou
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development