Solr
  1. Solr
  2. SOLR-2255

local params are not parsed in facet.pivot parameter

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.1
    • Component/s: None
    • Labels:
      None

      Description

      ...&facet=true&facet.pivot=

      {!ex=category}category_id,subcategory_id&fq={!tag=category}category_id=42

      generates the following error : "undefined field {!ex=category}

      category_id"

      If you filter on subcategory_id, the facet.pivot result will contain only results from this subcategory. It's a loss of function since you can't alter this behavior with local params.

        Activity

        Hide
        David Smiley added a comment -

        Attached is a patch developed against trunk. It'll probably apply cleanly against 4x but I haven't tried yet.

        Adding this feature was awkward because of the nature of the PivotFacetHelper and SimpleFacet classes. The core of the problem is that PivotFacetHelper needs to invoke SimpleFacet.parseParams() somehow but that method is package access. And the variables it sets are package access as well. It is invoked indirectly via getFacetFieldCounts() but PivotFacetHelper sidesteps that method and calls getTermCounts() so that the pivot code can split on commas. PivotFacetHelper was oddly (IMO) declared as a thread-safe class (it has no state) and there is a shared instance of it in FacetComponent but its not clear why; it's a trivial class. It has an extension hook getFacetImplementation() with a default impl that simply instantiates a new SimpleFacet instance. Yet I found no case of this method being overridden, and it doesn't seem like an extension point worth keeping. It's not clear that the SimpleFacet behemoth was designed to be substituted/extended.

        Whatever I did would be somewhat disruptive to the relationship between these classes so I took a stab at it any way. In my patch, PivotFacetHelper extends SimpleFacet and is instantiated similarly in FacetComponent. I removed the dubious getFacetImplementation() method in favor of simply referencing the current instance "this". This also made it possible to remove some parameters (which was getting a little out of control) since they are inherited fields now. I changed package access to protected to parseParams() and the fields it sets. I also decided to do a refactor rename there of "docs" to "docsOrig" and "base" to "docs" so that it's more clear what's going on there. It's not clear to me if references in this class to docsOrig is an undiscovered bug or not.

        The test demonstrates a filter query exclusion as well as "key" to add the data to the response using a different name. I noticed there is a "threads" performance tuning option that should now work too.

        Show
        David Smiley added a comment - Attached is a patch developed against trunk. It'll probably apply cleanly against 4x but I haven't tried yet. Adding this feature was awkward because of the nature of the PivotFacetHelper and SimpleFacet classes. The core of the problem is that PivotFacetHelper needs to invoke SimpleFacet.parseParams() somehow but that method is package access. And the variables it sets are package access as well. It is invoked indirectly via getFacetFieldCounts() but PivotFacetHelper sidesteps that method and calls getTermCounts() so that the pivot code can split on commas. PivotFacetHelper was oddly (IMO) declared as a thread-safe class (it has no state) and there is a shared instance of it in FacetComponent but its not clear why; it's a trivial class. It has an extension hook getFacetImplementation() with a default impl that simply instantiates a new SimpleFacet instance. Yet I found no case of this method being overridden, and it doesn't seem like an extension point worth keeping. It's not clear that the SimpleFacet behemoth was designed to be substituted/extended. Whatever I did would be somewhat disruptive to the relationship between these classes so I took a stab at it any way. In my patch, PivotFacetHelper extends SimpleFacet and is instantiated similarly in FacetComponent. I removed the dubious getFacetImplementation() method in favor of simply referencing the current instance "this". This also made it possible to remove some parameters (which was getting a little out of control) since they are inherited fields now. I changed package access to protected to parseParams() and the fields it sets. I also decided to do a refactor rename there of "docs" to "docsOrig" and "base" to "docs" so that it's more clear what's going on there. It's not clear to me if references in this class to docsOrig is an undiscovered bug or not. The test demonstrates a filter query exclusion as well as "key" to add the data to the response using a different name. I noticed there is a "threads" performance tuning option that should now work too.
        Hide
        David Smiley added a comment -

        This is a small update to the patch to update it for a recent change. FYI this works on 4x too.

        Any comments from the likes of Yonik, Hoss, Ryan, or Erik (those involved in the underlying code)?

        Show
        David Smiley added a comment - This is a small update to the patch to update it for a recent change. FYI this works on 4x too. Any comments from the likes of Yonik, Hoss, Ryan, or Erik (those involved in the underlying code)?
        Hide
        Yonik Seeley added a comment -

        Looks fine to me. PivotFacet was originally written as a separate component I think (which I was sort of against) - not sure if that explains any of the reasoning behind what the code looks like.

        Show
        Yonik Seeley added a comment - Looks fine to me. PivotFacet was originally written as a separate component I think (which I was sort of against) - not sure if that explains any of the reasoning behind what the code looks like.
        Hide
        David Smiley added a comment -

        Thanks for your examination Yonik. I plan to commit this to the 4x branch Monday, and it should eventually show up in v4.1. For the changes.txt entry I'll say this:

        SOLR-2255: Enhanced pivot faceting to use local-params in the same way that regular field value faceting can. This means support for excluding a filter query, using a different output key, and specifying 'threads' to do facet.method=fcs concurrently. PivotFacetHelper now extends SimpleFacet and the getFacetImplementation() extension hook was removed. (dsmiley)

        Show
        David Smiley added a comment - Thanks for your examination Yonik. I plan to commit this to the 4x branch Monday, and it should eventually show up in v4.1. For the changes.txt entry I'll say this: SOLR-2255 : Enhanced pivot faceting to use local-params in the same way that regular field value faceting can. This means support for excluding a filter query, using a different output key, and specifying 'threads' to do facet.method=fcs concurrently. PivotFacetHelper now extends SimpleFacet and the getFacetImplementation() extension hook was removed. (dsmiley)
        Hide
        Yonik Seeley added a comment -

        The patch had minimal changes to SimpleFacets (making private protected and one variable name change). I'd be comfortable with this being committed to the 4.0 branch also.

        Show
        Yonik Seeley added a comment - The patch had minimal changes to SimpleFacets (making private protected and one variable name change). I'd be comfortable with this being committed to the 4.0 branch also.
        Hide
        David Smiley added a comment -

        Yonik, so are you proposing the whole thing be committed to 4.0 or just the SimpleFacets changes?

        If the whole patch doesn't make it into 4.0, I propose that a deprecation warning to PivotFacetHelper.getFacetImplementation() being added to 4.0.

        Show
        David Smiley added a comment - Yonik, so are you proposing the whole thing be committed to 4.0 or just the SimpleFacets changes? If the whole patch doesn't make it into 4.0, I propose that a deprecation warning to PivotFacetHelper.getFacetImplementation() being added to 4.0.
        Hide
        Yonik Seeley added a comment -

        Yes, the whole thing.

        Show
        Yonik Seeley added a comment - Yes, the whole thing.
        Hide
        David Smiley added a comment -

        Too late for 4.0, but I did get a @Deprecated annotation in there this morning. It'll make it due to the respin.

        I committed to trunk & 4.1.

        Show
        David Smiley added a comment - Too late for 4.0, but I did get a @Deprecated annotation in there this morning. It'll make it due to the respin. I committed to trunk & 4.1.
        Hide
        Yonik Seeley added a comment -

        David, do you not feel confident about this for 4.0? IMO, the missing support for the params that every other faceting method uses can definitely be considered a bug, and still eligible for 4.0 if you feel comfortable with the changes.

        Show
        Yonik Seeley added a comment - David, do you not feel confident about this for 4.0? IMO, the missing support for the params that every other faceting method uses can definitely be considered a bug, and still eligible for 4.0 if you feel comfortable with the changes.
        Hide
        David Smiley added a comment -

        It was filed as a bug but I considered it a lack of a feature, and I changed the issue accordingly. It's debatable. I think the changes are relatively safe to the Solr codebase, but I have a suspicion that Ryan or Erik added that getFacetImplementation() that I removed and simply didn't notice my comments here to speak up about its merits. But more importantly I want to be very judicious about what goes into a release branch.

        Show
        David Smiley added a comment - It was filed as a bug but I considered it a lack of a feature, and I changed the issue accordingly. It's debatable. I think the changes are relatively safe to the Solr codebase, but I have a suspicion that Ryan or Erik added that getFacetImplementation() that I removed and simply didn't notice my comments here to speak up about its merits. But more importantly I want to be very judicious about what goes into a release branch.
        Hide
        Michael McCandless added a comment -

        But more importantly I want to be very judicious about what goes into a release branch.

        +1

        Show
        Michael McCandless added a comment - But more importantly I want to be very judicious about what goes into a release branch. +1
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] David Wayne Smiley
        http://svn.apache.org/viewvc?view=revision&revision=1389685

        SOLR-2255 local-param support for pivot faceting

        Show
        Commit Tag Bot added a comment - [branch_4x commit] David Wayne Smiley http://svn.apache.org/viewvc?view=revision&revision=1389685 SOLR-2255 local-param support for pivot faceting

          People

          • Assignee:
            David Smiley
            Reporter:
            Julien Lirochon
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development