Mailing List Archive: Solr upgrade to Lucene 9.8.0 question

Solr upgrade to Lucene 9.8.0 question

Sep 27, 2023, 3:31 PM

Post #1 of 3 (95 views)

Hi,

I am working on getting Solr upgraded to Lucene 9.8 [0] and I wanted to
raise visibility on an issue I ran into.

I believe PR#12380 [1] introduced a change that calls `finish()` on the
LeafCollector [2]. The trouble is on Solr side we have a few collectors
that extend SimpleCollector. (to be more precise there is a
DelegatingCollector in between but that does not change things).
SimpleCollector returns `this` on the `getLeafCollector` call, so now there
are 2 calls to the `finish()` method on the same collector instance (one as
a leaf, one at the end).

One example I am working with is CollapsingQParserPlugin$OrdScoreCollector
[3] where I am seeing a few tests fail because calling `finish` twice will
mess up the results. I don't know yet if there are others.

My first question is to validate this analysis with someone that knows this
code (and perhaps the Solr code too), and ideally also take a quick look at
my fix [4].

Second is related to PR#12380. is it expected that the 'finish' method is
idempotent? per my tests this seems to be called twice now in some cases
and it will be the case for any implementation extending SimpleCollector.
also given that SimpleCollector's getLeafCollector method is final, there
is almost no room for passing some state wrt. this being a leaf vs not.

thanks,
alex

[0] https://github.com/apache/solr/pull/1958
[1] https://github.com/apache/lucene/pull/12380
[2]
https://github.com/apache/lucene/blob/releases/lucene/9.8.0/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L779
[3]
https://github.com/apache/solr/blob/f0fcd300c896b858ae83235ecdb0a109eaea5cea/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java#L594
[4]
https://github.com/apache/solr/commit/fc8a2ffe8951f31aa3b65fac2adc9eaa3fee6258

Re: Solr upgrade to Lucene 9.8.0 question [ In reply to ]

jpountz at gmail

Sep 28, 2023, 1:22 PM

Post #2 of 3 (93 views)

Permalink

Hi Alex,

I believe that your analysis is correct.

> is it expected that the 'finish' method is idempotent?

I don't expect `finish()` to be idempotent. It should not get called
multiple times per segment either, only once and when collection runs
successfully. Do you have a Lucene test case that reproduces this double
calling of finish()?

I'm sorry this change broke Solr. I remember that Solr had post-collection
hooks, which felt like another case for adding this new API, but I
overlooked that it could break Solr by introducing a clash given that Solr
uses SimpleCollector.

Maybe we should think of deprecating SimpleCollector in Lucene and
recommend going with Collector directly. SimpleCollector is mostly a
backward compatibility layer with the old (9 years old
<https://github.com/apache/lucene/issues/6590>) collector API, we already
moved some of Lucene's main collectors to the new API, e.g.
TopScoreDocCollector and TopFieldCollector. Let's move other collectors
too, e.g. FacetsCollector and friends?

On Thu, Sep 28, 2023 at 12:31?AM Alex Deparvu <stillalex@apache.org> wrote:

> Hi,
>
> I am working on getting Solr upgraded to Lucene 9.8 [0] and I wanted to
> raise visibility on an issue I ran into.
>
> I believe PR#12380 [1] introduced a change that calls `finish()` on the
> LeafCollector [2]. The trouble is on Solr side we have a few collectors
> that extend SimpleCollector. (to be more precise there is a
> DelegatingCollector in between but that does not change things).
> SimpleCollector returns `this` on the `getLeafCollector` call, so now
> there are 2 calls to the `finish()` method on the same collector instance
> (one as a leaf, one at the end).
>
> One example I am working with is CollapsingQParserPlugin$OrdScoreCollector
> [3] where I am seeing a few tests fail because calling `finish` twice will
> mess up the results. I don't know yet if there are others.
>
> My first question is to validate this analysis with someone that knows
> this code (and perhaps the Solr code too), and ideally also take a quick
> look at my fix [4].
>
> Second is related to PR#12380. is it expected that the 'finish' method is
> idempotent? per my tests this seems to be called twice now in some cases
> and it will be the case for any implementation extending SimpleCollector.
> also given that SimpleCollector's getLeafCollector method is final, there
> is almost no room for passing some state wrt. this being a leaf vs not.
>
>
> thanks,
> alex
>
>
> [0] https://github.com/apache/solr/pull/1958
> [1] https://github.com/apache/lucene/pull/12380
> [2]
> https://github.com/apache/lucene/blob/releases/lucene/9.8.0/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L779
> [3]
> https://github.com/apache/solr/blob/f0fcd300c896b858ae83235ecdb0a109eaea5cea/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java#L594
> [4]
> https://github.com/apache/solr/commit/fc8a2ffe8951f31aa3b65fac2adc9eaa3fee6258
>
>

--
Adrien

Re: Solr upgrade to Lucene 9.8.0 question [ In reply to ]

stillalex at apache

Sep 28, 2023, 2:44 PM

Post #3 of 3 (93 views)

Permalink

Hi Adrien,

Thank you for your reply. At your suggestion I set out to put together a
Lucene only test and realised what was actually going on...

PR#12380 introduced a _new_ method called `void finish() throws
IOException` which clashed with Solr's existing finish method on
the DelegatingCollector (extending the SimpleCollector) [0]. this
introduced the second 'finish' call, one on Lucene side for the leaf
collectors and one on Solr side for completing the DelegatingCollector. the
new method fit so well with the existing method that I did not even see it
clashing. Renaming the existing method solves all of this weirdness.
Apologies for the noise, I was deep down the rabbit hole.

thanks,
alex

[0]
https://github.com/apache/solr/blob/f0fcd300c896b858ae83235ecdb0a109eaea5cea/solr/core/src/java/org/apache/solr/search/DelegatingCollector.java#L83

On Thu, Sep 28, 2023 at 1:23?PM Adrien Grand <jpountz@gmail.com> wrote:

> Hi Alex,
>
> I believe that your analysis is correct.
>
> > is it expected that the 'finish' method is idempotent?
>
> I don't expect `finish()` to be idempotent. It should not get called
> multiple times per segment either, only once and when collection runs
> successfully. Do you have a Lucene test case that reproduces this double
> calling of finish()?
>
> I'm sorry this change broke Solr. I remember that Solr had post-collection
> hooks, which felt like another case for adding this new API, but I
> overlooked that it could break Solr by introducing a clash given that Solr
> uses SimpleCollector.
>
> Maybe we should think of deprecating SimpleCollector in Lucene and
> recommend going with Collector directly. SimpleCollector is mostly a
> backward compatibility layer with the old (9 years old
> <https://github.com/apache/lucene/issues/6590>) collector API, we already
> moved some of Lucene's main collectors to the new API, e.g.
> TopScoreDocCollector and TopFieldCollector. Let's move other collectors
> too, e.g. FacetsCollector and friends?
>
>
>
> On Thu, Sep 28, 2023 at 12:31?AM Alex Deparvu <stillalex@apache.org>
> wrote:
>
>> Hi,
>>
>> I am working on getting Solr upgraded to Lucene 9.8 [0] and I wanted to
>> raise visibility on an issue I ran into.
>>
>> I believe PR#12380 [1] introduced a change that calls `finish()` on the
>> LeafCollector [2]. The trouble is on Solr side we have a few collectors
>> that extend SimpleCollector. (to be more precise there is a
>> DelegatingCollector in between but that does not change things).
>> SimpleCollector returns `this` on the `getLeafCollector` call, so now
>> there are 2 calls to the `finish()` method on the same collector instance
>> (one as a leaf, one at the end).
>>
>> One example I am working with is
>> CollapsingQParserPlugin$OrdScoreCollector [3] where I am seeing a few tests
>> fail because calling `finish` twice will mess up the results. I don't know
>> yet if there are others.
>>
>> My first question is to validate this analysis with someone that knows
>> this code (and perhaps the Solr code too), and ideally also take a quick
>> look at my fix [4].
>>
>> Second is related to PR#12380. is it expected that the 'finish' method is
>> idempotent? per my tests this seems to be called twice now in some cases
>> and it will be the case for any implementation extending SimpleCollector.
>> also given that SimpleCollector's getLeafCollector method is final, there
>> is almost no room for passing some state wrt. this being a leaf vs not.
>>
>>
>> thanks,
>> alex
>>
>>
>> [0] https://github.com/apache/solr/pull/1958
>> [1] https://github.com/apache/lucene/pull/12380
>> [2]
>> https://github.com/apache/lucene/blob/releases/lucene/9.8.0/lucene/core/src/java/org/apache/lucene/search/IndexSearcher.java#L779
>> [3]
>> https://github.com/apache/solr/blob/f0fcd300c896b858ae83235ecdb0a109eaea5cea/solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java#L594
>> [4]
>> https://github.com/apache/solr/commit/fc8a2ffe8951f31aa3b65fac2adc9eaa3fee6258
>>
>>
>
> --
> Adrien
>