Honor after value for skipping documents even if queue is not full for PagingFieldCollector

[gashutos]

Description

As mentioned in issue #12333, if after value is out of range for a field in a segment, PagingFieldCollector is not able to put any item in the FieldhitQueue. Due to which queueFull always stays false and it wont ever update PointValues based competitiveIterator to skip non-competitive hits.

I think, it makes sense to update competitiveIterator even if queue is not full and the query is after query because we have at least topValue in after query always. That itself should be capable enough to skip non-competitive hits.

It does not only help when after is out of range, also when after is within the range but filtering only based on the after value significantly reduces the number of hits to evaluate.

[gsmiller]

Seems reasonable to be able to skip due to deep paging before the queue fills. Nice find! It'd be great to see a test that reproduces this if possible. Not sure how hard that would be?

[gsmiller]

Do we need the lazy initialization? I thought topValueSet would already be set before the NumericLeafComparator gets constructed. Maybe I'm misunderstanding that?

[gashutos]

We dont know upfront at the time of construction (where currently initialization is done) that would we be needing maxValueAsBytes & minValuesAsBytes both. Like about the case, where we dont have any competitive hit collected in queue hence no bottom but has after value so the topValue.

[gsmiller]

Apologies in advance if I'm misunderstanding, but as the code is currently written, we also don't know if we'll ever need these arrays. If the queue never fills, we could unnecessarily have allocated one of them. I think we still have enough information upfront though to eagerly allocate these like we do today? Is it just a question of being eager vs. lazy with these?

[gashutos]

we also don't know if we'll ever need these arrays. If the queue never fills, we could unnecessarily have allocated one of them.

if queueFull is false, that mean we will be breaching this condition || (queueFull == false && topValueSet == false)) return; only if its after query where topValueSet is set to true. We dont allocate unnecessary here, i.e we initiliaze min/max values only if we are to call encodeTop or encodeBottom for that.

I gave explanation w.r.t code in current PR, but if you were talking in context of existing code, yes, we are allocating unnecessary in case queueFull is always false.

I think we still have enough information upfront though to eagerly allocate these like we do today? Is it just a question of being eager vs. lazy with these?

There is a problem if we follow the same approach, check this code,

} else {
        if (queueFull) { // bottom is avilable only when queue is full
          minValueAsBytes = minValueAsBytes == null ? new byte[bytesCount] : minValueAsBytes;
          encodeBottom(minValueAsBytes);
        }
        if (topValueSet) {
          maxValueAsBytes = maxValueAsBytes == null ? new byte[bytesCount] : maxValueAsBytes;
          encodeTop(maxValueAsBytes);
        }
      }

if queueFUll is always false & there is a topValueSet, we will have minValueAsBytes set as [0, 0, 0, 0, 0, 0, 0, 0, 0] instead null. This will result in incorrect minValueAsBytes in further calculations.

[gashutos]

Does that clarify ?

[gsmiller]

Oh, I see. Yes, that helps clarify. Thanks! The difference I missed is that we never start updating the competitive iterator until the queue fills in the current code, so it doesn't matter that these byte arrays are initialized as zeros, but now it does matter because null has meaning in the case of the competitive iterator update. OK got it. Thanks for walking me through it :)

[gsmiller]

Looks good. Thanks @gashutos! I'll get this merged.

[jpountz]

We just upgraded Elasticsearch to a Lucene snapshot that has this change, and this triggered major speedups on some queries. In my opinion, the PR title and description don't do justice to this change since it does not only help when after is out of range, also when after is within the range but filtering only based on the after value significantly reduces the number of hits to evaluate.

[gashutos]

We just upgraded Elasticsearch to a Lucene snapshot that has this change, and this triggered major speedups on some queries. In my opinion, the PR title and description don't do justice to this change since it does not only help when after is out of range, also when after is within the range but filtering only based on the after value significantly reduces the number of hits to evaluate.

@jpountz Agreed !

[jpountz]

@gashutos I think we should make users aware of this optimization, would you be up for opening another PR that adds a CHANGES entry?

[jpountz]

Let's also update the title/description of this PR?

[gashutos]

Sure, changed title/description, LMK if looks good.
CHANGES.txt PR #12368

[jpountz]

Looks great, thanks!