Didn't expect I'd run into it so quickly, but there are issues with adding createFacetsAccumulator to FacetRequest. Here are some details:
- RangeAccumulator only requires List<FacetRequest>
- TaxonomyFacetsAccumulator requires: List<FacetRequest>, FacetIndexingParams, TaxonomyReader and IndexReader
- SortedSetDVAccumulator requires: List<FacetRequest>, SortedSetDVReaderState
At first I made:
- FacetsAccumulator.create(FacetSearchParams, TaxonomyReader, IndexReader)
- FacetSearchParams covers List<FacetRequest> and FacetIndexingParams
- FacetRequest.createFacestAccumulator(FacetSearchParams, TxonomyReader, IndexReader).
But this is problematic on several fronts:
- A FacetRequest does not need List<FacetRequest> in createFacetsAccumulator(), only FacetIndexingParams
- Not all requests need FacetIndexingParams, TaxonomyReader and IndexReader
- While declaring parameters that aren't needed is not a big deal, it does mean that someone will need to pass e.g. taxoReader=null and hope for the best. Especially if someone uses only RangeAccumulator and SortedSetDVAccumulator.
- Some requests may need more than that, e.g. SortedSetDVReaderState.
So what's the best solution? It would be best if FacetRequest is self-descriptive so that apps can really just call FacetsAccumulator.create().
If we make FacetRequest take all the parameters it needs in the ctor, we sort of get that. A request is fully self-descriptive and FR.createFacetsAccumulator does not need any arguments. However, each new CountFacetRequest() will now need to pass many parameters, which is annoying and makes the code less readable, IMO.
I was thinking perhaps we can extend FacetSearchParams to include additional parameters. So ctor takes only the facet requests, and you can optionally set FacetIndexingParams, IndexReader, TaxonomyReader, SortedSetDVReaderState. There are minor issues that I think can be resolved easily:
- If in the future we'll create an accumulator which requires a different setting, we'll need to add that to FSP. That's easy.
- An app may not know which parameters to set on FSP in the first place – sort of easy to fix (on app side) cause it will hit a hard exception, probably NPE or some sort, immediately.
- This does show a slight problem though, as you need to create your FRs and then "remember" to set the right parameters on FSP.
There is another problem with FR.createFacetsAccumulator – the accumulators already take a List<FacetRequest> when created, however at the single FR level, it doesn't know the "global picture" and cannot pass all the requests up front. I was thinking that perhaps it can return a FacetAccumulatorBuilder which has API for addFacetRequest() and build(). FacetAccumulator.create() will call FR.createFacetAccumulatorBuilder() and will group all the requests that return the same builder, and then call builder.build() to get the proper FacetAccumulator instance. This is an orthogonal problem BTW to the fact that this builder will also need to know e.g. the SortedSetDVReaderState up front...
So ... making FR completely self-descriptive by taking all the parameters needed to execute it solves most of the problems – you have to resolve the needed parameters at ctor-time. But, it makes the code uglier. Moving parameters to FacetSearchParams keeps the FR ctors clean, but might be trappy for an app. I'll think about it more, but if someone has an idea how to tackle this ... don't be shy .