[
https://issues.apache.org/jira/browse/LUCENE-6808?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Hoss Man updated LUCENE-6808:
-----------------------------
Attachment: LUCENE-6808.patch
Weird, i wrote this comment yesterday, but aparently something failed when my
browser tried to submit it...
{panel}
bq. The intention of the FieldComparator.value API is that you would use it to
look up a value after all collection is complete, e.g. to fill the fields to
create the FieldDoc for this hit, and so the comparator would naturally not be
changing it anymore.
ok, but that's not clear/enforced anywhere in the API itself -- if you are only
suppose to fetch back all the slots after the search is complete, why support
fetching the individual slots (and compareValues()) at all?
bq. But I don't really understand what SOLR-6168 is trying to do ... maybe
there is another way to do it w/o abusing this API?
In a nutshell the CollapseQParser is another style of "group collapsing" where
it picks a single best representative doc for each group on the fly as docs are
collected -- currently it does this only based on a single field via DocValues,
SOLR-6168 is about making this work with any arbitrary "Sort" object. The code
in the patch (in SOLR-6168) is analogous to the type of logic that
FieldValueHitQueue does, but instead of tracking the "N best docs" (according
to the sort) in a single priority queue (which re-orders on the fly), it tracks
the *1* best doc for every "group" as it makes a single pass collection uses
value() + compareValues to know when a "better" doc has been found for that
group.
(The most trivial implementation would be to use a FieldValueHitQueue of size
'1' for each group, but this approach unwinds the logic a track the values
directly. When i was first looking at this approach, i initially considered
using a FieldComparator "slot" for each group, but the number of groups isn't
neccessarly known in advance and there's no way to "grow" a FieldComparator)
It doesn't feel like an abuse of the FieldComparator API to do this ... every
other Comparator works fine as is. As i mentioned I could always put hacks in
the caller code (ColapseQParser) to do instanceof/casting/deepCopy when the
FieldComparator returns BytesRefs -- but fixing Term(Ord)ValComparator.value to
return "safe" objects seemed saner.
---
Forgetting SOLR-6168, even if you asssume that all callers should know that the
result of FieldComparator.value is only usable afte the end of a search, the
BytesRefs returned by Term(Ord)ValComparator.value don't really seem to be safe
for a caller to hang on to (for later use searchAfter, etc...) in any situation
where the FieldComparator itself might get re-used -- ie: re-using a
FieldValueHitQueue (or Collector that uses FieldValueHitQueue) in a subsequent
search.
what protects the user from those values being changed out from under them in
that situation?
{panel}
...but beyond that, this morning a slightly diff impl occurred to me that
should still address my Comparator re-use concerns w/o the "extra per-hit
garbage" mike was worded about in the more common case...
{code}
public BytesRef value(int slot) {
// null out the corrisponding Builder to prevent future changes to
returned BytesRef instance
// in cases where the user makes additional slot changes after calling
value
tempBRs[slot] = null;
return values[slot];
}
{code}
...any concerns with this mike? A new BytesRefBuilder instance will be created
in tempBRs\[slot\] if and only if the FieldComparator gets re-used after the
call to value(slot).
> Term(Ord)ValComparator.value return BytesRefs that are re-used internally
> -------------------------------------------------------------------------
>
> Key: LUCENE-6808
> URL: https://issues.apache.org/jira/browse/LUCENE-6808
> Project: Lucene - Core
> Issue Type: Bug
> Reporter: Hoss Man
> Attachments: LUCENE-6808.patch, LUCENE-6808.patch
>
>
> While working on SOLR-6168, which involves some non-trivial usage of
> {{FieldComparator}}, I discovered some weird bugs anytime I was using
> TermOrdValComparator. I ultimately tracked this down to the fact that the
> {{BytesRef}} instances returned by {{TermOrdValComparator.value(int slot)}}
> are backed by {{BytesRefBuilder}} instances that the Comparator hangs on to
> and re-uses -- so the values a caller gets back from
> {{FieldComparator.value(slot)}} might be changed out from under it before it
> has a chance to use that value in something like
> {{FieldComparator.compareValues(first,second)}}.
> The general approach when dealing with BytesRef instances (as i understand
> it) is that the caller is responsible for making a copy if it wants to hang
> on to it -- but in this case that would violate the generic API of
> FieldComparator -- callers would have to pay attention to when a
> {{FieldComparator}} is a
> {{FieldComparator<BytesRef>}} and do casting to copy the BytesRef.
> It seems like the right solution is for {{TermOrdValComparator.value(slot)}}
> (and {{TermValComparator.value(slot)}} which has a similar BytesRef usage) to
> return {{BytesRef.deepCopyOf(values\[slot\])}}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
