Solr
  1. Solr
  2. SOLR-5541

Allow QueryElevationComponent to accept elevateIds and excludeIds as http parameters

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.6
    • Fix Version/s: 4.7
    • Labels:
      None

      Description

      The QueryElevationComponent currently uses an xml file to map query strings to elevateIds and excludeIds.

      This ticket adds the ability to pass in elevateIds and excludeIds through two new http parameters "elevateIds" and "excludeIds".

      This will allow more sophisticated business logic to be used in selecting which ids to elevate/exclude.

      Proposed syntax:

      http://localhost:8983/solr/elevate?q=*:*&elevatedIds=3,4&excludeIds=6,8

      The elevateIds and excludeIds point to the unique document Id.

      1. SOLR-5541.patch
        4 kB
        Joel Bernstein
      2. SOLR-5541.patch
        3 kB
        Joel Bernstein
      3. SOLR-5541.patch
        3 kB
        Joel Bernstein
      4. SOLR-5541.patch
        3 kB
        Joel Bernstein

        Activity

        Hide
        Rahul Babulal added a comment -

        FYI...,

        There is a typo in the URL in the JIRA as well as the documentation,

        The param elevatedIds, should be elevateIds

        http://localhost:8983/solr/elevate?q=*:*&elevatedIds=3,4&excludeIds=6,8
        
        Show
        Rahul Babulal added a comment - FYI..., There is a typo in the URL in the JIRA as well as the documentation, The param elevatedIds, should be elevateIds http: //localhost:8983/solr/elevate?q=*:*&elevatedIds=3,4&excludeIds=6,8
        Hide
        ASF subversion and git services added a comment -

        Commit 1565526 from Joel Bernstein in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1565526 ]

        SOLR-5541: Use StrUtils.splitSmart to handle escape chars

        Show
        ASF subversion and git services added a comment - Commit 1565526 from Joel Bernstein in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1565526 ] SOLR-5541 : Use StrUtils.splitSmart to handle escape chars
        Hide
        ASF subversion and git services added a comment -

        Commit 1565520 from Joel Bernstein in branch 'dev/trunk'
        [ https://svn.apache.org/r1565520 ]

        SOLR-5541: Use StrUtils.splitSmart to handle escape chars

        Show
        ASF subversion and git services added a comment - Commit 1565520 from Joel Bernstein in branch 'dev/trunk' [ https://svn.apache.org/r1565520 ] SOLR-5541 : Use StrUtils.splitSmart to handle escape chars
        Hide
        Joel Bernstein added a comment -

        Looks like splitSmart will give us \ escapes. I'll slide that in.

        Show
        Joel Bernstein added a comment - Looks like splitSmart will give us \ escapes. I'll slide that in.
        Hide
        Yonik Seeley added a comment -

        The canonical way to do this in Solr is a StrUtils.splitSmart variant (the second one that doesn't do quotes I imagine)

        Show
        Yonik Seeley added a comment - The canonical way to do this in Solr is a StrUtils.splitSmart variant (the second one that doesn't do quotes I imagine)
        Hide
        Jan Høydahl added a comment -

        Great feature.

        Valid ids may contain commas. Should not this feature provide a way to elevate/exclude such docs?
        Either allow escaping, i.e. one\,id,another\,id, or allow configuring separator, i.e. elevate.sep=;.

        Show
        Jan Høydahl added a comment - Great feature. Valid ids may contain commas. Should not this feature provide a way to elevate/exclude such docs? Either allow escaping, i.e. one\,id,another\,id , or allow configuring separator, i.e. elevate.sep=; .
        Hide
        ASF subversion and git services added a comment -

        Commit 1556923 from Joel Bernstein in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1556923 ]

        SOLR-5541: Allow QueryElevationComponent to accept elevateIds and excludeIds as http parameters

        Show
        ASF subversion and git services added a comment - Commit 1556923 from Joel Bernstein in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1556923 ] SOLR-5541 : Allow QueryElevationComponent to accept elevateIds and excludeIds as http parameters
        Hide
        ASF subversion and git services added a comment -

        Commit 1556903 from Joel Bernstein in branch 'dev/trunk'
        [ https://svn.apache.org/r1556903 ]

        SOLR-5541: Allow QueryElevationComponent to accept elevateIds and excludeIds as http parameters

        Show
        ASF subversion and git services added a comment - Commit 1556903 from Joel Bernstein in branch 'dev/trunk' [ https://svn.apache.org/r1556903 ] SOLR-5541 : Allow QueryElevationComponent to accept elevateIds and excludeIds as http parameters
        Hide
        Joel Bernstein added a comment - - edited

        Added a new patch. In the initial patches I was piggy backing a convenience method that was used by the test cases to manually set the elevateIds and excludeIds. This method was adding to an in memory cache that holds all the queries that were in the elevation xml file.

        This approach would cause a memory leak when doing large scale query elavation, which this patch is designed to support.

        This patch stops piggy backing that convenience method and adds a new method that doesn't interact with the elevation cache.

        Show
        Joel Bernstein added a comment - - edited Added a new patch. In the initial patches I was piggy backing a convenience method that was used by the test cases to manually set the elevateIds and excludeIds. This method was adding to an in memory cache that holds all the queries that were in the elevation xml file. This approach would cause a memory leak when doing large scale query elavation, which this patch is designed to support. This patch stops piggy backing that convenience method and adds a new method that doesn't interact with the elevation cache.
        Hide
        Joel Bernstein added a comment -

        Thanks Mark, I'll fix that up.

        Show
        Joel Bernstein added a comment - Thanks Mark, I'll fix that up.
        Hide
        Mark Miller added a comment -

        +1

        One comment:

        + assertQ("All six should make it", req

        Should update the copy/paste assert comment - only 5 should make it because b is excluded.

        Show
        Mark Miller added a comment - +1 One comment: + assertQ("All six should make it", req Should update the copy/paste assert comment - only 5 should make it because b is excluded.
        Hide
        Joel Bernstein added a comment -

        Added test case

        Show
        Joel Bernstein added a comment - Added test case
        Hide
        Joel Bernstein added a comment -

        Wondering if having two separate elevation components would be confusing?

        Looking through the code it looks like the XML config loading happens on demand so it wouldn't get triggered by accident. Also the component needs to be switched on so it seems like it would be safe in the default list.

        Show
        Joel Bernstein added a comment - Wondering if having two separate elevation components would be confusing? Looking through the code it looks like the XML config loading happens on demand so it wouldn't get triggered by accident. Also the component needs to be switched on so it seems like it would be safe in the default list.
        Hide
        Hoss Man added a comment -

        Joel: This smells like the kind of thing that could easily be it's own Component (using a common base class with the existing QueryElevationComponent containing code refactored out of QEC) .. and since this new type of elevation wouldn't require any explicit XML configuration, it could concievably be be wired into the default list of components in SearchHandler.

        thoughts?

        Show
        Hoss Man added a comment - Joel: This smells like the kind of thing that could easily be it's own Component (using a common base class with the existing QueryElevationComponent containing code refactored out of QEC) .. and since this new type of elevation wouldn't require any explicit XML configuration, it could concievably be be wired into the default list of components in SearchHandler. thoughts?
        Hide
        Joel Bernstein added a comment - - edited

        Initial patch, built from trunk

        Show
        Joel Bernstein added a comment - - edited Initial patch, built from trunk

          People

          • Assignee:
            Joel Bernstein
            Reporter:
            Joel Bernstein
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development