Mailing List Archive: LeafCollector#finish idempotency?

LeafCollector#finish idempotency?

Oct 9, 2023, 8:37 AM

Post #1 of 4 (63 views)

Hey folks-

I'm curious if anyone has thoughts around idempotency concerns related to
the LeafCollector#finish API added in GH#12380
<https://github.com/apache/lucene/pull/12380>. My expectation would be that
LeafCollector implementations should be able to assume #finish will only
get called once. In fact, it looks like FacetsCollector is already making
that assumption.

Is this inline with other folks' expectations? If so, I'm going to, 1)
address a small bug related to drill-sideways that results #finish being
called multiple times on one of the collectors, and 2) propose some
additional javadoc on LeafCollector#finish clarifying this.

Make sense?

Cheers,
-Greg

Re: LeafCollector#finish idempotency? [ In reply to ]

jpountz at gmail

Oct 9, 2023, 8:46 AM

Post #2 of 4 (63 views)

Permalink

Hi Greg,

I agree that LeafCollector implementations should be able to assume that
finish() only gets called once. The test framework already makes this
assumption:
https://github.com/apache/lucene/blob/dfff1e635805ffc61dd6029a8060e2635bfcbdb9/lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingLeafCollector.java#L95-L100
.

On Mon, Oct 9, 2023 at 5:38?PM Greg Miller <gsmiller@gmail.com> wrote:

> Hey folks-
>
> I'm curious if anyone has thoughts around idempotency concerns related to
> the LeafCollector#finish API added in GH#12380
> <https://github.com/apache/lucene/pull/12380>. My expectation would be
> that LeafCollector implementations should be able to assume #finish will
> only get called once. In fact, it looks like FacetsCollector is already
> making that assumption.
>
> Is this inline with other folks' expectations? If so, I'm going to, 1)
> address a small bug related to drill-sideways that results #finish being
> called multiple times on one of the collectors, and 2) propose some
> additional javadoc on LeafCollector#finish clarifying this.
>
> Make sense?
>
> Cheers,
> -Greg
>

--
Adrien

Re: LeafCollector#finish idempotency? [ In reply to ]

mdrob at mdrob

Oct 9, 2023, 9:44 AM

Post #3 of 4 (63 views)

Permalink

Not sure if you saw this, Greg, but Alex ran into a similar question
recently from Solr.
https://lists.apache.org/thread/1gs3nsv1mcns1czdtdnqyz84f31tqm2x

On Mon, Oct 9, 2023 at 10:47?AM Adrien Grand <jpountz@gmail.com> wrote:

> Hi Greg,
>
> I agree that LeafCollector implementations should be able to assume that
> finish() only gets called once. The test framework already makes this
> assumption:
> https://github.com/apache/lucene/blob/dfff1e635805ffc61dd6029a8060e2635bfcbdb9/lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingLeafCollector.java#L95-L100
> .
>
> On Mon, Oct 9, 2023 at 5:38?PM Greg Miller <gsmiller@gmail.com> wrote:
>
>> Hey folks-
>>
>> I'm curious if anyone has thoughts around idempotency concerns related to
>> the LeafCollector#finish API added in GH#12380
>> <https://github.com/apache/lucene/pull/12380>. My expectation would be
>> that LeafCollector implementations should be able to assume #finish will
>> only get called once. In fact, it looks like FacetsCollector is already
>> making that assumption.
>>
>> Is this inline with other folks' expectations? If so, I'm going to, 1)
>> address a small bug related to drill-sideways that results #finish being
>> called multiple times on one of the collectors, and 2) propose some
>> additional javadoc on LeafCollector#finish clarifying this.
>>
>> Make sense?
>>
>> Cheers,
>> -Greg
>>
>
>
> --
> Adrien
>

Re: LeafCollector#finish idempotency? [ In reply to ]

gsmiller at gmail

Oct 9, 2023, 11:32 AM

Post #4 of 4 (63 views)

Permalink

Thanks Adrien & Mike! I hadn't seen the Solr email thread, but that's a
good reference (as is the AssertingLeafCollector implementation). I've
opened two follow-up PRs as a result:

1. Fix a buried drill-sideways bug where #finish can be called more than
once: https://github.com/apache/lucene/pull/12642
2. Add a small note to LeafCollector#finish javadoc:
https://github.com/apache/lucene/pull/12643

Cheers,
-Greg

On Mon, Oct 9, 2023 at 1:45?PM Mike Drob <mdrob@mdrob.com> wrote:

> Not sure if you saw this, Greg, but Alex ran into a similar question
> recently from Solr.
> https://lists.apache.org/thread/1gs3nsv1mcns1czdtdnqyz84f31tqm2x
>
> On Mon, Oct 9, 2023 at 10:47?AM Adrien Grand <jpountz@gmail.com> wrote:
>
>> Hi Greg,
>>
>> I agree that LeafCollector implementations should be able to assume that
>> finish() only gets called once. The test framework already makes this
>> assumption:
>> https://github.com/apache/lucene/blob/dfff1e635805ffc61dd6029a8060e2635bfcbdb9/lucene/test-framework/src/java/org/apache/lucene/tests/search/AssertingLeafCollector.java#L95-L100
>> .
>>
>> On Mon, Oct 9, 2023 at 5:38?PM Greg Miller <gsmiller@gmail.com> wrote:
>>
>>> Hey folks-
>>>
>>> I'm curious if anyone has thoughts around idempotency concerns related
>>> to the LeafCollector#finish API added in GH#12380
>>> <https://github.com/apache/lucene/pull/12380>. My expectation would be
>>> that LeafCollector implementations should be able to assume #finish will
>>> only get called once. In fact, it looks like FacetsCollector is already
>>> making that assumption.
>>>
>>> Is this inline with other folks' expectations? If so, I'm going to, 1)
>>> address a small bug related to drill-sideways that results #finish being
>>> called multiple times on one of the collectors, and 2) propose some
>>> additional javadoc on LeafCollector#finish clarifying this.
>>>
>>> Make sense?
>>>
>>> Cheers,
>>> -Greg
>>>
>>
>>
>> --
>> Adrien
>>
>