Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1, 6.0
    • Component/s: modules/facet
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      FacetIndexingParams lets you configure few things such as OrdinalPolicy, PathPolicy and partitionSize. However, in order to set them you must extend DefaultFacetIndexingParams and override fixedXY(), as the respective getters are final.

      I'd like to do the following:

      • Turn FacetIndexingParams and FacetSearchParams into concrete classes, rather than interfaces. The reason they are interfaces because one app once wants to have one class which implements both. Since FSP holds FIP, I don't think that's needed.
      • Add setters to FacetIndexingParams for the relevant configuration parameters, rather than forcing someone to extend the class. Extensions should really be for special cases, which we haven't identified so far (except overriding these settings), hence why there's only DefaultFIP/FSP.
      • Add some javadocs...
      • I'm sure, w/ my pedantic and perfectionist nature, that more thing will be done once I get to it .
      1. LUCENE-4621.patch
        166 kB
        Shai Erera
      2. LUCENE-4621.patch
        117 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        Patch does the following:

        • FacetIndexingParams is now a concrete class rather than interface, and folds in DefaultFIP.
        • The entire IndexingParams chain is now immutable. You can pass the CategoryListParams at construction time, but for overriding the rest of the parameters, you need to extend one of the classes in the chain.
        • Added FIP.ALL_PARENTS as a singleton. If we'll have a decent solution for NO_PARENTS, we should add such singleton too. I figure that what users would like to change would be the OrdinalPolicy, so this constant is a conveniency for not allocating FIP().

        The chain of indexing params is now:

        • FacetIndexingParams: either use a default CategoryListParams or set your own.
          • PerDimensionIndexingParams: allows you to specify a mapping from a CategoryPath to CategoryListParams.
            • EnhancementsIndexingParams: for indexing category associations. Note, even before this change that class extended PerDimensionIP.

        FacetIP has a scary equals() and hashCode() implementations. Not sure why do we need them, i.e. do we put them in a map? Anyway, this should not block the commit.

        All tests pass, I think it's ready.

        Show
        Shai Erera added a comment - Patch does the following: FacetIndexingParams is now a concrete class rather than interface, and folds in DefaultFIP. The entire IndexingParams chain is now immutable. You can pass the CategoryListParams at construction time, but for overriding the rest of the parameters, you need to extend one of the classes in the chain. Added FIP.ALL_PARENTS as a singleton. If we'll have a decent solution for NO_PARENTS, we should add such singleton too. I figure that what users would like to change would be the OrdinalPolicy, so this constant is a conveniency for not allocating FIP(). The chain of indexing params is now: FacetIndexingParams: either use a default CategoryListParams or set your own. PerDimensionIndexingParams: allows you to specify a mapping from a CategoryPath to CategoryListParams. EnhancementsIndexingParams: for indexing category associations. Note, even before this change that class extended PerDimensionIP. FacetIP has a scary equals() and hashCode() implementations. Not sure why do we need them, i.e. do we put them in a map? Anyway, this should not block the commit. All tests pass, I think it's ready.
        Hide
        Shai Erera added a comment -

        FacetSearchParams could also be made immutable, by eliminating addFacetRequest and setClCache. I saw that it might be an issue with the tests, so I'll check if this can be done cleanly. Basically I think it's doable, I'll do it now.

        Show
        Shai Erera added a comment - FacetSearchParams could also be made immutable, by eliminating addFacetRequest and setClCache. I saw that it might be an issue with the tests, so I'll check if this can be done cleanly. Basically I think it's doable, I'll do it now.
        Hide
        Shai Erera added a comment -

        Updated patch makes FacetSearchParams immutable too. Now all FacetRequests must be specified at construction. For convenience I added these ctors:

        • FSP(FacetRequest...) - default indexing params, useful for one or two requests.
        • FSP(List<FacetRequest>) - default indexing params, useful when you need to build a list of requests, because e.g. you don't know how many you'll get.
        • FSP(List<FacetRequest>, FacetIP> - if you want to use custom indexing params, you need to construct a list.

        You can no longer set CategoryListCache, you need to override. But with all the changes happening (cutting over to DV), I suspect that this method will be nuked completely, soon.

        FacetSP do not allow empty requests. That caused trouble in PartitionUtils because it exposed methods that take FSP, but only delegate to FIP methods. I nuked the FSP methods, whoever uses partitions should understand that he can call fsp.getFIP(). The problem was that some places just called PartitionUtils w/ an empty FSP, just because you could initialize one.

        I think this is ready. Would be glad if someone took a quick look at FSP, FIP and its chain, see if you think it can be improved somehow.

        Show
        Shai Erera added a comment - Updated patch makes FacetSearchParams immutable too. Now all FacetRequests must be specified at construction. For convenience I added these ctors: FSP(FacetRequest...) - default indexing params, useful for one or two requests. FSP(List<FacetRequest>) - default indexing params, useful when you need to build a list of requests, because e.g. you don't know how many you'll get. FSP(List<FacetRequest>, FacetIP> - if you want to use custom indexing params, you need to construct a list. You can no longer set CategoryListCache, you need to override. But with all the changes happening (cutting over to DV), I suspect that this method will be nuked completely, soon. FacetSP do not allow empty requests. That caused trouble in PartitionUtils because it exposed methods that take FSP, but only delegate to FIP methods. I nuked the FSP methods, whoever uses partitions should understand that he can call fsp.getFIP(). The problem was that some places just called PartitionUtils w/ an empty FSP, just because you could initialize one. I think this is ready. Would be glad if someone took a quick look at FSP, FIP and its chain, see if you think it can be improved somehow.
        Hide
        Sivan Yogev added a comment -

        Patch looks good. One question - there is a ctor of FIP whose Javadoc states: "You should use this constructor only if you intend to override any of the getters". Shouldn't it be protected?

        Show
        Sivan Yogev added a comment - Patch looks good. One question - there is a ctor of FIP whose Javadoc states: "You should use this constructor only if you intend to override any of the getters". Shouldn't it be protected?
        Hide
        Shai Erera added a comment -

        Thanks for the review !

        That comment is meant more to direct user's attention to the constant. If I'd make it protected, then you couldn't do:

        FacetIndexingParams fip = new FacetIndexParams() {
          @Override
          public int getPartitionSize() { return size; }
        }
        

        This patter is easy and used in tests, as well as I used it in some code that I wrote. It doesn't force you to actually declare a class.

        Show
        Shai Erera added a comment - Thanks for the review ! That comment is meant more to direct user's attention to the constant. If I'd make it protected, then you couldn't do: FacetIndexingParams fip = new FacetIndexParams() { @Override public int getPartitionSize() { return size; } } This patter is easy and used in tests, as well as I used it in some code that I wrote. It doesn't force you to actually declare a class.
        Hide
        Sivan Yogev added a comment -

        OK, ready to commit from my side.

        Show
        Sivan Yogev added a comment - OK, ready to commit from my side.
        Hide
        Michael McCandless added a comment -

        This patch looks like a great cleanup!

        Show
        Michael McCandless added a comment - This patch looks like a great cleanup!
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] Shai Erera
        http://svn.apache.org/viewvc?view=revision&revision=1421256

        LUCENE-4621: FacetIndexing/SearchParams house cleaning

        Show
        Commit Tag Bot added a comment - [trunk commit] Shai Erera http://svn.apache.org/viewvc?view=revision&revision=1421256 LUCENE-4621 : FacetIndexing/SearchParams house cleaning
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Shai Erera
        http://svn.apache.org/viewvc?view=revision&revision=1421262

        LUCENE-4621: FacetIndexing/SearchParams house cleaning

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Shai Erera http://svn.apache.org/viewvc?view=revision&revision=1421262 LUCENE-4621 : FacetIndexing/SearchParams house cleaning
        Hide
        Shai Erera added a comment -

        Committed to trunk and 4x.

        Show
        Shai Erera added a comment - Committed to trunk and 4x.

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development