[
https://issues.apache.org/jira/browse/LUCENE-6808?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14901752#comment-14901752
]
Michael McCandless commented on LUCENE-6808:
--------------------------------------------
bq. OK, in that case I just did a thorough code review to investigate any
issues.
Thank you for the thorough code review / explanation ...
But my concern wasn't that we can convince ourselves today that setting to null
(this change) is OK, it's that the code, to future devs looking at it, looks
bad/buggy because a getter really should not have side effects. This class is
already scary enough, I don't think we should make it worse ...
bq. So the fix prevents the trap that Hoss found
I don't think this is a trap: I think it's abusing the API.
bq. I think I just found another place where we fell into that same trap
Hmm, where?
> 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: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
