[
https://issues.apache.org/jira/browse/SOLR-13132?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17072269#comment-17072269
]
Chris M. Hostetter commented on SOLR-13132:
-------------------------------------------
{quote}The changes presented here now consist of: ...
{quote}
Yup yup ... having the jiras split out *definitely* makes it easier to review
them and understand what chagnes were made for what purposes.
I have been reviewing the "sweep" collection changes a bit more in depth
lately, but i've been distracted with a lot of other things, sorry about the
delay.
I'm still wrapping my head around a lot of it – I've mainly been focused on the
higher level changes (to the internal APIs/abstractions), and thinking about
what the corner cases are that might not be obvious in existing testing, and
wondering wether there may be different/better ways to accomplish the same
objective with a smaller "patch footprint" that affects less internal logic.
(i'm not saying there are, just trying to do due-dillagence).
I haven't really spent any time at all yet looking into the lower level changes
in {{FacetFieldProcessorByArrayDV}} and {{UnInvertedField}}, but I do have a
few questions / concerns that i'll go ahead and share now – note that I haven't
fully examinied / thought through all of these, they are starting points for
discussions, not requests that you make any sorts of specific changes...
Questions:
* the " {{// XXXXXX temporary!}} " comment in {{SKGSlotAcc}} suggests that
{{SKGSlotAcc}} can/should be removed once {{SweepSKGSlotAcc}} is adopted ...
but {{SweepSKGSlotAcc}} doesn't appear to be a "universal" replacement for
{{SKGSlotAcc}}. {{RelatednessAgg}} uses hueristics + the
{{DISABLE_SWEEP_COLLECTION}} param to decide whne/what to use ... is this
comment old / a mistake?
* the introduction of the {{CountSlotAccFactory}} interface seems like dead
code? ... the only instance i see is {{DEFAULT_COUNT_ACC_FACTORY}} and even
that is never used directly, just as the implicit default for the 2 arg version
of {{getSweepCountAcc()}} to call the 3 arg version ... why not eliminate the 3
arg version?
* do {{ReadOnlyCountSlotAcc}} and {{ReadOnlyCountSlotAccWrapper}} really need
to exist as new classes? why can't we just introduce {{ReadOnlyCountSlotAcc}}
as an interface implemented by {{CountSlotAcc}} that only offers the (read
only) "getter" methods we want callers to have access to? (and then only expose
this interface in the "sweep data strcutures" that the Sweep Accs have access
to) Is there a specific reason for the "wrapper" pattern here that i'm not
understanding?
** the fact that the wrapper just seems to throw UnsupportedOperationException
on methods that aren't read-only suggests it's just ment to help us find any
'miss-use' type bugs during testing, why not weed those bugs out at compile
time?
* why is {{FilterCtStruct}} a distinct data structure from
{{SweepCountAccStruct}}? ... if the {{Filter}} in a {{FilterCtStruct}} _always_
comes from calling {{docSet.getTopFilter()}} then can't that just be exposed
via a new {{SweepCountAccStruct.getFilter()}} method?
Concerns:
* It feels like there is some "brittleness" (in terms of future
changes/improvements in JSON Faceting internals) related to when/how/where
Sweep can be used – in particularly because both the FacetProcessor and the
Aggregation have to _support_ sweeping in the first place, but they never
actually "communicate" that info directly. {{RelatednessAgg}} has it's own
hueristics/assumptions for deciding (based on reflection checks against the
Processor) if/when to return a SweepAcc – but if/when future Processors (or
riskier: future Processor subclasses w/new "optimizations") come around that
heuristic might break. Then the SweepAcc kind of "goes in the backdoor" of the
Processor to setup the {{CountSlotAcc}} / {{SweepCountAccStruct}} instances it
wants/needs – but there is no guarantee that the Processor will even look at
those.
** at the very least, I suppose we could make
{{FacetFieldProcessor.getSweepCountAcc()}} default to returning 'null', making
subclasses that support sweeping override it, and Aggs that want to leverage
sweeping would use 'null' to recognize that sweeping isn't supported?
** but meanwhile: we also have the (empty) {{SweepAcc}} marker interface that
Processors can use in instanceof checks to take conditional action, but this
also seems brittle in it's own way: notable the main (only?) use at the moment
is {{collectAcc instanceof SweepAcc}}, but this doesn't account for the
possibility that {{collectAcc = new MultiAcc(fcontext, accs)}} in the case of a
{{singlePassSlotAccCollection}} ... a situation where IIUC sweep collection
over _multiple_ {{relatedness()}} calls (nested under the same term facet)
would make a lot of sense? ... IIUC in the current code that posssibility might
be broken break? both {{RelatednessAggs}} would return {{SweepSKGSlotAcc}}
impls, but {{countOnly}} would be false? ... haven't dug in enough to be sure
if the {{SweepAccs}} get used in that case
** related: AFAICT even if "sweep" collection is used, and the
{{CountSlotAccStruct}} get populated, the Processor still keeps the
{{SweepSKGSlotAcc}} around in it's {{collectAcc}} – resulting in useless No-Op
calls to {{SweepSKGSlotAcc.collect()}}
* perhaps a "cleaner" approach would be to re-factor/re-tool the {{SweepAcc}}
API concept such that:
** {{SlotAcc}} has some new method that Processors (which support sweeping)
can call if/when they want to know if that {{SlotAcc}} wants to use sweeping
*** by default this method is a No-Op, but things like {{SKGSlotAcc}} could
instead "do something" to initialize a (new type of) {{SweepAcc}} it wants to
be used during sweeping and return some kind of indicator that it no longer
needs to be used _as a collector_ – but the {{SlotAcc}} would still have the
choice (and could use special params like {{DISABLE_SWEEP_COLLECTION}} to
inform that choice)
** this new {{SlotAcc}} method would only be called by Processors that support
sweeping against their {{collectAcc}} (or delegated to via {{MultiAcc}}) and if
the return value indicates that the {{SlotAcc}} no longer needs then the
Processor would null out it's {{collectAcc}} (and/or {{MultiAcc}} would remove
that particular {{SlotAcc}} from it's list, and if all the nested {{SlotAccs}}
inside of a {{MultiAcc}} support sweeping, then the {{MultiAcc}} would return
the status that it should be removed)
*** this would also allow us to eliminate the need for the {{AggValueSources}}
(like {{RelatednessAgg}}) that want to use sweeping to have to know that they
need to treat {{numSlots == 1}} as special. Instead the code paths in the
processor that use {{numSlots==1}} would just never call this new method.
** And, perhaps: instead of the (existing) special case logic in the
processors regarding {{countAcc}}, maybe {{CountSlotAcc}} could/should/would
also implement this API to initialize the same sweep collection data structures
just like any other {{SlotAcc}} that supports sweeping ?
* whatever winds up happening with the "should i use sweeping?" hueristic in
{{RelatednessAgg}}, i think that instead of a {{"disable_sweep_collection"}}
option that defaults to {{false}} and is used _as an input to the hueristic_ we
should instead have a {{"sweep_collection"}} option that defaults to {{null}}
meaning "use hueristic" but if it's explicitly set to {{true}} or {{false}}
then the hueristic shouldn't be used at all – this is more in line with how
similar options work (like overrequest)
** it also makes equivilence testing easier, because instead of making
assumptions about the hueristic, we can just force both code paths in tests.
** of course, if we wind up completely removing the hueristic because the
logic moves to the Processor, then we can just make {{"sweep_collection"}}
default to {{true}}
* wherever/however the decision to use sweeping is made, the final outcome
should get put into the facet debug in some way, so that tests can assert
sweeping gets used when expected
* I think, in general, we should probably avoid new methods that return a
'new' array (as opposed to direct access to some sort of interal array) or null
– let's use List (or empty List) for these types of things
** in particular, {{getSweepFilters()}} can return null, but
{{FacetFieldProcessorByArrayDV}} doesn't do any null checking (i think this
only works right now because there's always at least one
{{SweepCountAccStruct}} for the "base" {{CountSlotAcc}} – but if it was just a
non-nullable List the code would be simpler and this wouldn't be a time bomb
waiting to go off the next time someone tweaks this code.
** I'm also suspicious of the way these new methods go out of their way to
ensure the element corrisponding to the "base" is the last element in the array
... that's a big code smell for me. I'm not clear yet on why it matters which
one is the 'base' if we're doing a sweep over all of them, but if it does then
that suggests we should be using a more robust data strcuture then just an
array/List to encapsulate the items
** or to revisit my earlier quandary about wether the new sweep support would
allow us to generalize the {{countAcc}} to be "just another sweep collector"
instead of a special case in hte processors: If it really is important to keep
track of the "base" {{SweepCountAccStruct}}, then maybe the
API/logic/data-structures around how an Agg that wants to do sweep collection
work should ultimately involve registering those info/data-structures (ie:
{{SweepCountAccStruct}} ) with a special (new) {{BaseCountSlotAcc}} for the
"base" set that would then encapsulate all of the information the (sweep
supporting) processors need realting to the the
DocSets/Filters/SweepAccs/etc...) to use when sweeping ?
*** ie: the new {{SlotAcc}} method i hypothosized above that
Processors/MultiAcc would call if they support Sweeping might be something like
{{public boolean replaceCollectionWithSweep(BaseCountSlotAcc)}} that could add
the {{SweepCountAccStruct}} directly to the (new) {{BaseCountSlotAcc}} (instead
of to the processor) and then return {{true}} indicating that the Processor (or
{{MultiAcc}} can remove this {{SlotAcc}} from {{collectAcc}} because the sweep
logic will now handle all collection.
> Improve JSON "terms" facet performance when sorted by relatedness
> ------------------------------------------------------------------
>
> Key: SOLR-13132
> URL: https://issues.apache.org/jira/browse/SOLR-13132
> Project: Solr
> Issue Type: Improvement
> Components: Facet Module
> Affects Versions: 7.4, master (9.0)
> Reporter: Michael Gibney
> Priority: Major
> Attachments: SOLR-13132-with-cache-01.patch,
> SOLR-13132-with-cache.patch, SOLR-13132.patch
>
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> When sorting buckets by {{relatedness}}, JSON "terms" facet must calculate
> {{relatedness}} for every term.
> The current implementation uses a standard uninverted approach (either
> {{docValues}} or {{UnInvertedField}}) to get facet counts over the domain
> base docSet, and then uses that initial pass as a pre-filter for a
> second-pass, inverted approach of fetching docSets for each relevant term
> (i.e., {{count > minCount}}?) and calculating intersection size of those sets
> with the domain base docSet.
> Over high-cardinality fields, the overhead of per-term docSet creation and
> set intersection operations increases request latency to the point where
> relatedness sort may not be usable in practice (for my use case, even after
> applying the patch for SOLR-13108, for a field with ~220k unique terms per
> core, QTime for high-cardinality domain docSets were, e.g.: cardinality
> 1816684=9000ms, cardinality 5032902=18000ms).
> The attached patch brings the above example QTimes down to a manageable
> ~300ms and ~250ms respectively. The approach calculates uninverted facet
> counts over domain base, foreground, and background docSets in parallel in a
> single pass. This allows us to take advantage of the efficiencies built into
> the standard uninverted {{FacetFieldProcessorByArray[DV|UIF]}}), and avoids
> the per-term docSet creation and set intersection overhead.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
