FacetIndexing/SearchParams house cleaning [LUCENE-4621]

[asfimport]

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 :).

---

Migrated from LUCENE-4621 by Shai Erera (@shaie), resolved Dec 13 2012 Attachments: LUCENE-4621.patch (versions: 2)

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

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.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

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.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

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.

[asfimport]

Sivan Yogev (migrated from JIRA)

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?

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

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.

[asfimport]

Sivan Yogev (migrated from JIRA)

OK, ready to commit from my side.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

This patch looks like a great cleanup!

[asfimport]

Commit Tag Bot (migrated from JIRA)

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

LUCENE-4621: FacetIndexing/SearchParams house cleaning

[asfimport]

Commit Tag Bot (migrated from JIRA)

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

LUCENE-4621: FacetIndexing/SearchParams house cleaning

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Committed to trunk and 4x.