Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7493

Support of TotalHitCountCollector for FacetCollector.search api if numdocs passed as zero.

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: master (7.0), 6.3
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Hi,

      I want to do drill down search using FacetCollection below is the code

      FacetsCollector facetCollector = new FacetsCollector();
      TopDocs topDocs = FacetsCollector.search(st.searcher, filterQuery, limit, facetCollector);

      I just want facet information so I pass limit as zero but I get error "numHits must be > 0; please use TotalHitCountCollector if you just need the total hit count".

      For FacetCollector there is no way to initialize 'TotalHitCountCollector'. Internally it always create either 'TopFieldCollector' or 'TopScoreDocCollector' which does not allow limit as 0.

      So if limit should be zero then there should be a way that 'TotalHitCountCollector' should be initialized.

      Better way would be to provide an api which takes query and collector as inputs just like 'drillSideways.search(filterQuery, totalHitCountCollector)'.

      1. LUCENE-7493-Fail.patch
        4 kB
        Mahesh
      2. LUCENE-7493-Pass.patch
        4 kB
        Mahesh

        Activity

        Hide
        mikemccand Michael McCandless added a comment -

        Thank you Mahesh ... maybe you could make a test case in a patch showing the exception when you pass limit=0? I think the fix should be simple enough, basically the code you posted on the mailing list (once we debug it!)...

        Show
        mikemccand Michael McCandless added a comment - Thank you Mahesh ... maybe you could make a test case in a patch showing the exception when you pass limit=0? I think the fix should be simple enough, basically the code you posted on the mailing list (once we debug it!)...
        Hide
        maahi333 Mahesh added a comment -

        This is the working copy of code that I have right now.

        FacetsCollector facetCollector = new FacetsCollector();
        TopDocs topDocs = null;
        TotalHitCountCollector totalHitCountCollector = null;
        if (limit == 0)

        { totalHitCountCollector = new TotalHitCountCollector(); indexSearcher.search(query, MultiCollector.wrap(totalHitCountCollector, facetCollector)); topDocs = new TopDocs(totalHitCountCollector.getTotalHits(), new ScoreDoc[0], Float.NaN); }

        else
        topDocs = FacetsCollector.search(st.searcher, filterQuery, first + limit, facetCollector);

        Show
        maahi333 Mahesh added a comment - This is the working copy of code that I have right now. FacetsCollector facetCollector = new FacetsCollector(); TopDocs topDocs = null; TotalHitCountCollector totalHitCountCollector = null; if (limit == 0) { totalHitCountCollector = new TotalHitCountCollector(); indexSearcher.search(query, MultiCollector.wrap(totalHitCountCollector, facetCollector)); topDocs = new TopDocs(totalHitCountCollector.getTotalHits(), new ScoreDoc[0], Float.NaN); } else topDocs = FacetsCollector.search(st.searcher, filterQuery, first + limit, facetCollector);
        Hide
        maahi333 Mahesh added a comment -

        Should I add test case as an attachment in this bug?

        Show
        maahi333 Mahesh added a comment - Should I add test case as an attachment in this bug?
        Hide
        mikemccand Michael McCandless added a comment -

        Should I add test case as an attachment in this bug?

        Yes please!

        You can e.g. just add a new test method, e.g. testZeroLimit, to one of the existing test cases under lucene/facet/src/test/... e.g. TestDrillDownQuery, run it using ant test -Dtestcase=TestDrillDownQuery -Dtests.method=testZeroLimit and it should fail at first. Then fold in your changes above, and run the test case again, and it should pass.

        Then use git diff HEAD to make a patch file that you attach back on this issue.

        Show
        mikemccand Michael McCandless added a comment - Should I add test case as an attachment in this bug? Yes please! You can e.g. just add a new test method, e.g. testZeroLimit , to one of the existing test cases under lucene/facet/src/test/... e.g. TestDrillDownQuery , run it using ant test -Dtestcase=TestDrillDownQuery -Dtests.method=testZeroLimit and it should fail at first. Then fold in your changes above, and run the test case again, and it should pass. Then use git diff HEAD to make a patch file that you attach back on this issue.
        Hide
        maahi333 Mahesh added a comment -

        Patch file to reproduce the issue.

        Show
        maahi333 Mahesh added a comment - Patch file to reproduce the issue.
        Hide
        maahi333 Mahesh added a comment -

        Thanks Michael McCandless for helping out. Followed your steps and attached patch file. Hope its what you expect.

        Show
        maahi333 Mahesh added a comment - Thanks Michael McCandless for helping out. Followed your steps and attached patch file. Hope its what you expect.
        Hide
        mikemccand Michael McCandless added a comment -

        Thanks Mahesh, but I was hoping for a test that fails, i.e. you should change that test case to pass limit=0 to FacetCollector.search such that when you run the test, it hits an exception.

        Once we have that, then you can also go and fix FacetCollector.search internally with your fix (to use wrapped TotalHitCountCollector when limit is 0), at which point the test should pass.

        Show
        mikemccand Michael McCandless added a comment - Thanks Mahesh , but I was hoping for a test that fails , i.e. you should change that test case to pass limit=0 to FacetCollector.search such that when you run the test, it hits an exception. Once we have that, then you can also go and fix FacetCollector.search internally with your fix (to use wrapped TotalHitCountCollector when limit is 0), at which point the test should pass.
        Hide
        maahi333 Mahesh added a comment -

        Hi Michael McCandless,

        Added patch for failure and success case.

        Show
        maahi333 Mahesh added a comment - Hi Michael McCandless, Added patch for failure and success case.
        Hide
        mikemccand Michael McCandless added a comment -

        Thank you Mahesh.

        Hmm but there is a problem with your change inside FacetsCollector: I think in the limit=0 case you will get no facet results, because you make a new FacetCollector rather than using the Collector passed in by the user?

        Can you improve the test to confirm that you do get facet results with limit=0, which should fail, and then fix your changes in FacetsCollector and then the test should pass?

        Show
        mikemccand Michael McCandless added a comment - Thank you Mahesh . Hmm but there is a problem with your change inside FacetsCollector : I think in the limit=0 case you will get no facet results, because you make a new FacetCollector rather than using the Collector passed in by the user? Can you improve the test to confirm that you do get facet results with limit=0, which should fail, and then fix your changes in FacetsCollector and then the test should pass?
        Hide
        maahi333 Mahesh added a comment -

        Hey Michael McCandless thanks for pointing out.

        With new code there is weird behavior. Code fix and test executes as expected like before fix assertion fails and after fix test passes but the problem that I had is that sometimes test will not pass in all cases with error ' java.lang.IllegalArgumentException: dimension "b" was not indexed into field "$facets'. This is happening randomly with no fixed step to reproduce and I am not sure why .

        Show
        maahi333 Mahesh added a comment - Hey Michael McCandless thanks for pointing out. With new code there is weird behavior. Code fix and test executes as expected like before fix assertion fails and after fix test passes but the problem that I had is that sometimes test will not pass in all cases with error ' java.lang.IllegalArgumentException: dimension "b" was not indexed into field "$facets'. This is happening randomly with no fixed step to reproduce and I am not sure why .
        Hide
        mikemccand Michael McCandless added a comment -

        OK that failure is confusing ... you need to use this line instead to get the facets:

            Facets facets = getTaxonomyFacetCounts(taxo, config, facetCollector, config.getDimConfig("b").indexFieldName);
        

        Try that and see if the test always passes?

        This is needed because this test's BeforeClass randomly assigns the b field to a different index field name ...

        Show
        mikemccand Michael McCandless added a comment - OK that failure is confusing ... you need to use this line instead to get the facets: Facets facets = getTaxonomyFacetCounts(taxo, config, facetCollector, config.getDimConfig("b").indexFieldName); Try that and see if the test always passes? This is needed because this test's BeforeClass randomly assigns the b field to a different index field name ...
        Hide
        maahi333 Mahesh added a comment -

        Thanks Michael McCandless.

        The above code works and test passes always now. Attached are the patch files,

        Show
        maahi333 Mahesh added a comment - Thanks Michael McCandless. The above code works and test passes always now. Attached are the patch files,
        Hide
        mikemccand Michael McCandless added a comment -

        Excellent, the new patch looks great Mahesh! I'll make some minor code style fixes and commit. Thank you!

        Show
        mikemccand Michael McCandless added a comment - Excellent, the new patch looks great Mahesh ! I'll make some minor code style fixes and commit. Thank you!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 739981b6c8e6ccd60279216b320d8a25d06c70e9 in lucene-solr's branch refs/heads/master from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=739981b ]

        LUCENE-7493: FacetCollector.search now accepts limit=0, for getting facets but not search hits

        Show
        jira-bot ASF subversion and git services added a comment - Commit 739981b6c8e6ccd60279216b320d8a25d06c70e9 in lucene-solr's branch refs/heads/master from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=739981b ] LUCENE-7493 : FacetCollector.search now accepts limit=0, for getting facets but not search hits
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 2b32f5d0db532eeee723f707f0d704d7e6f6ac33 in lucene-solr's branch refs/heads/branch_6x from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2b32f5d ]

        LUCENE-7493: FacetCollector.search now accepts limit=0, for getting facets but not search hits

        Show
        jira-bot ASF subversion and git services added a comment - Commit 2b32f5d0db532eeee723f707f0d704d7e6f6ac33 in lucene-solr's branch refs/heads/branch_6x from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2b32f5d ] LUCENE-7493 : FacetCollector.search now accepts limit=0, for getting facets but not search hits
        Hide
        mikemccand Michael McCandless added a comment -

        Thank you Mahesh.

        Show
        mikemccand Michael McCandless added a comment - Thank you Mahesh .
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Closing after 6.3.0 release.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Closing after 6.3.0 release.

          People

          • Assignee:
            Unassigned
            Reporter:
            maahi333 Mahesh
          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development