[
https://issues.apache.org/jira/browse/SOLR-14467?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17112695#comment-17112695
]
Chris M. Hostetter commented on SOLR-14467:
-------------------------------------------
{quote}... unsurprisingly, most (all?) seem to ignore slotContext ...
{quote}
Correct. slotContext was introduced when the {{relatedness()}} stat was added.
Prior to that, all "stats" were simple aggregations of document field values –
they cared only about the individual documents, not the "set" of documents that
make up the bucket.
{{relatedness()}} introduced the need for knowing "context" about the current
bucket/slot for the purpose of computing statistical results that couldn't be
aggregated from the individual document values.
The {{SlotContext}} API was added as a "hook" to capture the information
currently needed by {{relatedness()}} (the {{Query}}) and to provide an
extention point for the future if/when any additional info that might be needed
by other "advanced" stats about the context of the bucket. The {{collect()}}
API was extended to take in a {{IntFunction<SlotContext>}} (as opposed to just
{{SlotContext}}) so that there would be no overhead for "simple" aggregations
that only care about the individual documents and their field values – ex:
computing "sum(price)" under a "terms" facet on the "category" doesn't result
in building a {{SlotContext}} (object or a {{TermQuery}}) for evey facet
bucket, because the "sum" aggretaion never invokes the {{IntFunction}}.
{quote}I'm curious to know what you make of the approach in the attached patch.
{quote}
My Initial impressions:
* i don't like the idea of adding yet another collect method signature to
specify the "accumulation" slot independently of the "collection" slot ... i
think that if that info really is useful it should just be added as a new
method callback in the {{SlotContext}}
* I _really_ don't like adding {{instanceof}} checks to {{RelatednessAgg}}
that make hardocded assumptions about what impls will/won't ever exist or
support allBuckets
** reading {{allBucketsSlot}} once and assuming it will never change is also a
brittle assumption that defies the design of the Resizer API – having just
fixed SOLR-14492, I'm pretty sure this will break with DVHASH.
** Again: I think it would be better if this info was conveyed via the
SlotContext, using some Enum or flag
* trackAllBucketsTermOrds looks like it's broken if/when a Resizer is used?
... i don't see any resize calls for it (again: pretty sure this will break
with DVHASH)
In general i feel like this patch jumps through a lot of hoops to try and
"support" {{allBuckets}} on {{relatedness()}} – to the point of (IIUC?) a lot
of new bookkeeping about when we've already accumulated this slot, but not for
the same contextSlotOrd, in which case we merge it (in a way that i'm not sure
at a glance is correct? .. it might be, but i think i saw a place where we
overwrite a slot value with a new BUcketData w/o checking if it's non-null?)
.... but none of this really answers the question of whether this
accumulated/merged {{relatedness()}} calculation is *meaningful* in the
allBucket context?
That's really the question that should be asked before doing any work to make
the code more complicated: Is there any semantic *meaning* behind returning a
{{relatedness()}} score for the {{allBuckets:true}} situation?
I would argue the answer is "No" ...
{{relatedness()}} is inherently a computation of how the population of the
_*SET*_ of documents in a bucket intersects with the population of the
"foreground" and "background" sets of documents. {{allBuckets}} is fundamental
about merging the a*ccumulated results* of many different buckets _even when
the sets of documents in those buckets overlap_ due to the buckets
corresponding with multivalued field.
I said before...
{quote}My gut says we should just using the base DocSet/domain for the entire
facet as the slotContext in SpecialSlotAcc ...
{quote}
... but the more i think about it the more i feel like that would also be
missleading, A user that explicitly requested {{allBuckets: true}} knowing that
the resulting sub-stat calculations will be different then just computing stats
on the "base" set, would then be misslead by {{relatedness()}} values that
don't factor in counting the same document more then once because they occur in
multiple buckets ... but likewise adding special logic to "double count"
documents based on how many buckets they are in seems like it would violate the
intent of {{relatedness()}} since the calculations are currently focused solely
on the _set_ of documents, and don't consider things like "these buckets are
based on term values, how many times does this term appear in each document?"
I think the safe thing to do (for now) is say " {{relatedness()}} is
meaningless in {{allBuckets}} ." ... in a way that fixes the Server errors, and
leave open the possibility of revisting the idea later.
----
Here's what i would propose we do...
* add {{public boolean isAllBucket() \{ return false;}}} to SlotContext
** {{SpecialSlotAcc}} should override this to return true
* {{SKGSlotAcc.processSlot()}} should start checking for {{isAllBucket()}}
** when true, asign to that slot a {{new AllBucketsSpecialBucketData(agg)}}
and return immediately (no need to do any calculations)
** this would be a new {{BucketData}} subclass that overrides {{externalize}}
so that the "final" bucket returned to the end user is null (or maybe an empty
map? ... not sure what would be better)
*** probably need to return something special in the {{isShard}} situation and
update FacetMerger to recognize thisand return the correct BucketData instance
on merging?
* Add a note in the ref-gude explaining that {{relatedness()}} scores are
meaningless in the context of {{allBucket}} so we don't compute them, and
explain what users should expect in the result...
The end result being that people can still use {{relatedness()}} as a sub-stat
of a {{terms}} facet that uses the {{allBuckets: true}} option (ie: if they are
interesting computing _other_ stats across {{allBuckets}} ) but the
{{relatedness()}} value that gets put in the {{allBuckets}} output will be
"empty"
thoughts?
> inconsistent server errors combining relatedness() with allBuckets:true
> -----------------------------------------------------------------------
>
> Key: SOLR-14467
> URL: https://issues.apache.org/jira/browse/SOLR-14467
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Components: Facet Module
> Reporter: Chris M. Hostetter
> Priority: Major
> Attachments: SOLR-14467.patch, SOLR-14467_test.patch
>
>
> While working on randomized testing for SOLR-13132 i discovered a variety of
> different ways that JSON Faceting's "allBuckets" option can fail when
> combined with the "relatedness()" function.
> I haven't found a trivial way to manual reproduce this, but i have been able
> to trigger the failures with a trivial patch to {{TestCloudJSONFacetSKG}}
> which i will attach.
> Based on the nature of the failures it looks like it may have something to do
> with multiple segments of different sizes, and or resizing the SlotAccs ?
> The relatedness() function doesn't have much (any?) existing tests in place
> that leverage "allBuckets" so this is probably a bug that has always existed
> -- it's possible it may be excessively cumbersome to fix and we might
> nee/wnat to just document that incompatibility and add some code to try and
> detect if the user combines these options and if so fail with a 400 error?
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
