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

Make grouping code use response builder needDocList

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4
    • Component/s: None
    • Labels:
      None

      Description

      Right now the grouping code does this to check if it needs to generate a docList for grouped results:

      if (rb.doHighlights || rb.isDebug() || params.getBool(MoreLikeThisParams.MLT, false) ){
      ...
      }
      

      this is ugly because any new component that needs a doclist, from grouped results, will need to modify QueryComponent to add a check to this if. Ideally this should just use the rb.isNeedDocList() flag...

      Coincidentally this boolean is really never used at for non-grouped results it always gets generated..

      1. SOLR-5616.patch
        3 kB
        Keith Laban
      2. SOLR-5616.patch
        3 kB
        Erick Erickson
      3. SOLR-5616.patch
        6 kB
        Steven Bower

        Activity

        Hide
        sbower Steven Bower added a comment -

        Here is a patch that makes this change. It's against trunk but should easily patch onto older versions. Ideally this would get onto a 4.x release..

        Show
        sbower Steven Bower added a comment - Here is a patch that makes this change. It's against trunk but should easily patch onto older versions. Ideally this would get onto a 4.x release..
        Hide
        erickerickson Erick Erickson added a comment -

        The same patch, except it doesn't re-arrange imports.

        Nice catch, changing the QueryComponent when you add a new component sure is ugly.

        My only question is whether the sense of the test in QueryComponent is preserved or whether this line (479):

        if (rb.isNeedDocList()) {
        

        should be

        if (rb.isNeedDocList() || rb.isDebug()) {
        

        The test over in DebugComponent isn't quite the same:

          if(rb.isDebugTrack() && rb.isDistrib) {
        
        Show
        erickerickson Erick Erickson added a comment - The same patch, except it doesn't re-arrange imports. Nice catch, changing the QueryComponent when you add a new component sure is ugly. My only question is whether the sense of the test in QueryComponent is preserved or whether this line (479): if (rb.isNeedDocList()) { should be if (rb.isNeedDocList() || rb.isDebug()) { The test over in DebugComponent isn't quite the same: if (rb.isDebugTrack() && rb.isDistrib) {
        Hide
        k317h Keith Laban added a comment -

        Attached new, addressing Erick Erickson comments from way back when. test suite passes when running locally

        Show
        k317h Keith Laban added a comment - Attached new, addressing Erick Erickson comments from way back when. test suite passes when running locally
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit fecdec6c85f6180f00e870ca8ec14058d30a1fae in lucene-solr's branch refs/heads/master from Dennis Gove
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fecdec6 ]

        SOLR-5616: Simplifies grouping code to use ResponseBuilder.needDocList() to determine if it needs to generate a doc list for grouped results.

        Show
        jira-bot ASF subversion and git services added a comment - Commit fecdec6c85f6180f00e870ca8ec14058d30a1fae in lucene-solr's branch refs/heads/master from Dennis Gove [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fecdec6 ] SOLR-5616 : Simplifies grouping code to use ResponseBuilder.needDocList() to determine if it needs to generate a doc list for grouped results.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit b1ce385302d055e53e51f364d88482cf7e24ad6f in lucene-solr's branch refs/heads/branch_6x from Dennis Gove
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b1ce385 ]

        SOLR-5616: Simplifies grouping code to use ResponseBuilder.needDocList() to determine if it needs to generate a doc list for grouped results.

        Show
        jira-bot ASF subversion and git services added a comment - Commit b1ce385302d055e53e51f364d88482cf7e24ad6f in lucene-solr's branch refs/heads/branch_6x from Dennis Gove [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b1ce385 ] SOLR-5616 : Simplifies grouping code to use ResponseBuilder.needDocList() to determine if it needs to generate a doc list for grouped results.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 5a93f83276eaa15ea58a2969f64c85834571ea78 in lucene-solr's branch refs/heads/branch_6x from Yonik Seeley
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5a93f83 ]

        SOLR-5616: fix unused import

        Show
        jira-bot ASF subversion and git services added a comment - Commit 5a93f83276eaa15ea58a2969f64c85834571ea78 in lucene-solr's branch refs/heads/branch_6x from Yonik Seeley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5a93f83 ] SOLR-5616 : fix unused import

          People

          • Assignee:
            dpgove Dennis Gove
            Reporter:
            sbower Steven Bower
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development