[
https://issues.apache.org/jira/browse/LUCENE-4985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13721923#comment-13721923
]
Shai Erera commented on LUCENE-4985:
------------------------------------
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 :).
> Make it easier to mix different kinds of FacetRequests
> ------------------------------------------------------
>
> Key: LUCENE-4985
> URL: https://issues.apache.org/jira/browse/LUCENE-4985
> Project: Lucene - Core
> Issue Type: Improvement
> Components: modules/facet
> Reporter: Michael McCandless
> Fix For: 5.0, 4.5
>
>
> Spinoff from LUCENE-4980, where we added a strange class called
> RangeFacetsAccumulatorWrapper, which takes an incoming FSP, splits out the
> FacetRequests into range and non-range, delegates to two accumulators for
> each set, and then zips the results back together in order.
> Somehow we should generalize this class and make it work with
> SortedSetDocValuesAccumulator as well.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
