Lucene - Core
  1. Lucene - Core
  2. LUCENE-4980

Can't use DrillSideways with both RangeFacetRequest and non-RangeFacetRequest

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, 5.0
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I tried to combine these two and there were several issues:

      • It's ... really tricky to manage the two different
        FacetAccumulators across that N FacetCollectors that DrillSideways
        creates ... to fix this I added a new MultiFacetsAccumulator that
        switches for you.
      • There was still one place in DS/DDQ that wasn't properly handling
        a non-Term drill-down.
      • There was a bug in the "collector method" for DrillSideways
        whereby if a given segment had no hits, it was skipped, which is
        incorrect because it must still be visited to tally up the
        sideways counts.
      • Separately I noticed that DrillSideways was doing too much work:
        it would count up drill-down counts and drill-sideways counts
        against the same dim (but then discard the drill-down counts in
        the end).
      1. LUCENE-4980.patch
        23 kB
        Michael McCandless
      2. LUCENE-4980.patch
        23 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Patch w/ test case + fixes.

        Show
        Michael McCandless added a comment - Patch w/ test case + fixes.
        Hide
        Shai Erera added a comment -

        I was confused by the name MultiFacetsAccumulator as I thought it takes something like a Map<FacetRequest,FacetsAccumulator>, but I see that it only distinguishes RangeAccumulator from others. So I'm worried about someone gets confused about the name and use it incorrectly. I don't have a better name in mind though ... RangeAndRegularFacetsAccumulator?

        What if RangeAccumulator did that under the covers? I.e. instead of rejecting non-RangeFacetRequest, it created FA over all such requests? Multi is quite simple though, so I like it .. maybe FacetAccumulatorRangeWrapper? I think as long as we keep the word Range in the name, it's less likely users will get confused.

        Minor comments about the class: (a) can you rename 'a' and 'ra'? (b) why do you need to hold onto fspOrig? Is it because FA.searchParams isn't final?

        Show
        Shai Erera added a comment - I was confused by the name MultiFacetsAccumulator as I thought it takes something like a Map<FacetRequest,FacetsAccumulator>, but I see that it only distinguishes RangeAccumulator from others. So I'm worried about someone gets confused about the name and use it incorrectly. I don't have a better name in mind though ... RangeAndRegularFacetsAccumulator? What if RangeAccumulator did that under the covers? I.e. instead of rejecting non-RangeFacetRequest, it created FA over all such requests? Multi is quite simple though, so I like it .. maybe FacetAccumulatorRangeWrapper? I think as long as we keep the word Range in the name, it's less likely users will get confused. Minor comments about the class: (a) can you rename 'a' and 'ra'? (b) why do you need to hold onto fspOrig? Is it because FA.searchParams isn't final?
        Hide
        Michael McCandless added a comment -

        What if RangeAccumulator did that under the covers?

        Well ... I have a TODO to also support SortedSetDocValuesAccumulator. So I'm not quite sure what to name it / where to put it.

        Another option here is to commit this class only under src/test ... it's technically only needed right now by the test case to expose the bugs ... but then I'm using the class in the Jira search app, because I need to use DrillSideways with range and non-range facets, and without it things get very messy. So we need to fix something here, but we can do it in a separate issue after fixing these bugs.

        Minor comments about the class: (a) can you rename 'a' and 'ra'?

        Will do ...

        (b) why do you need to hold onto fspOrig? Is it because FA.searchParams isn't final?

        I need fspOrig in accumulator() to un-collate the wrapped List<FacetResult> back in the same order as the original requests ...

        Show
        Michael McCandless added a comment - What if RangeAccumulator did that under the covers? Well ... I have a TODO to also support SortedSetDocValuesAccumulator. So I'm not quite sure what to name it / where to put it. Another option here is to commit this class only under src/test ... it's technically only needed right now by the test case to expose the bugs ... but then I'm using the class in the Jira search app, because I need to use DrillSideways with range and non-range facets, and without it things get very messy. So we need to fix something here, but we can do it in a separate issue after fixing these bugs. Minor comments about the class: (a) can you rename 'a' and 'ra'? Will do ... (b) why do you need to hold onto fspOrig? Is it because FA.searchParams isn't final? I need fspOrig in accumulator() to un-collate the wrapped List<FacetResult> back in the same order as the original requests ...
        Hide
        Michael McCandless added a comment -

        New patch, folding in Shai's feedback.

        I just renamed MFA -> RangeFacetsAccumulatorWrapper for now ... in a followon issue we can generalize this better (eg to include SortedSetDV facets too).

        Show
        Michael McCandless added a comment - New patch, folding in Shai's feedback. I just renamed MFA -> RangeFacetsAccumulatorWrapper for now ... in a followon issue we can generalize this better (eg to include SortedSetDV facets too).
        Hide
        Shai Erera added a comment -

        Thanks Mike. I think the TODO is a bit trickier than what it writes ... you cannot decide to create SortedSetDVAccumulator based on a FacetRequest. App needs to create it only if it indexed facets with SSDVFacetFields, right? This might be possible if we had an index schema, of somehow expressed that information in FacetIndexingParams.

        So it's up to you to remove the TODO, but I don't think you can actually handle it, currently, without app specifically telling you that it's ok.

        Show
        Shai Erera added a comment - Thanks Mike. I think the TODO is a bit trickier than what it writes ... you cannot decide to create SortedSetDVAccumulator based on a FacetRequest. App needs to create it only if it indexed facets with SSDVFacetFields, right? This might be possible if we had an index schema, of somehow expressed that information in FacetIndexingParams. So it's up to you to remove the TODO, but I don't think you can actually handle it, currently, without app specifically telling you that it's ok.
        Hide
        Michael McCandless added a comment -

        I think the TODO is a bit trickier than what it writes ... you cannot decide to create SortedSetDVAccumulator based on a FacetRequest.

        Hmm true. Maybe we need a separate SSDVFacetRequest? Not sure ...

        I don't think I'll remove the TODO: I think it's still important that (somehow) we are able to send an FSP requiring different FacetsAccumulators and something figures out which ones to instantiate, and then collates the FacetResults back in order, somehow ... I will fixup the TODO to reflect its difficulty though

        Show
        Michael McCandless added a comment - I think the TODO is a bit trickier than what it writes ... you cannot decide to create SortedSetDVAccumulator based on a FacetRequest. Hmm true. Maybe we need a separate SSDVFacetRequest? Not sure ... I don't think I'll remove the TODO: I think it's still important that (somehow) we are able to send an FSP requiring different FacetsAccumulators and something figures out which ones to instantiate, and then collates the FacetResults back in order, somehow ... I will fixup the TODO to reflect its difficulty though
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development