SOLR-18363: remove EarlyTerminatingSortingCollector, use native TopFieldCollector - #4804
Conversation
…eldCollector Forces TopFieldCollectorManager's totalHitsThreshold down to `len` when segmentTerminateEarly=true, so TopFieldCollector does the same per-segment skip natively. A thin SegmentTerminatedEarlyObserver catches CollectionTerminatedException (only ever thrown from TopFieldCollector's sort-compatibility-gated path) to derive segmentTerminatedEarly precisely, without TopFieldCollector#isEarlyTerminated() (which also fires on routine hit-count-exceeded, unrelated to sort) or any package-private Lucene API. RankQuery + segmentTerminateEarly now logs "unsupported combination" explicitly instead of silently doing nothing (previously-untested combination, old wrapper-based code did attempt it). Also deprecates the public surface around this feature (the request param, response header, QueryCommand accessors) -- only the internal implementation class was ever marked deprecated before.
github-actions
Bot
added
documentation
|
@epugh does this need a changelog entry, or can you add |
|
I think it maybe should have a changelog, especially since there appears to be a use case that has slightly changed behvior? I don't work much on the Solr <--> Lucene integration layer, so not an expert on what's expected. |
|
Added — see |
| WARNING: Deprecated since 11.0. Its implementation relies on internal Lucene behavior that cannot | ||
| currently be replicated using Lucene's own public early-termination support (see SOLR-18363). | ||
|
|
There was a problem hiding this comment.
When deprecating, we should mention that minExactCount is the new parameter which controls when the search will be terminated early. This should be mentioned in the changelog as well.
There was a problem hiding this comment.
Added -- see the updated WARNING and changelog, both now point to minExactCount.
|
|
||
| final FieldDoc searchAfter = (null != cursor ? cursor.getSearchAfterFieldDoc() : null); | ||
| if (allowNativeSegmentTerminateEarly) { | ||
| minNumFound = 0; |
There was a problem hiding this comment.
shouldn't this be len?
There was a problem hiding this comment.
Confirmed equivalent -- TopFieldCollector's constructor clamps totalHitsThreshold to Math.max(totalHitsThreshold, numHits) internally (Lucene 10.4.0, TopFieldCollector.java:335), so 0 and len produce the same effective threshold. Switched to len.
Point the deprecation warning + changelog at minExactCount as the replacement, and use len instead of 0 for the forced totalHitsThreshold (Lucene clamps to max(totalHitsThreshold, numHits) internally, so it's equivalent, but len reads clearer).
…ly-termination # Conflicts: # solr/core/src/java/org/apache/solr/search/EarlyTerminatingSortingCollector.java
3 participants
https://issues.apache.org/jira/browse/SOLR-18363
Removes
EarlyTerminatingSortingCollectorin favor of nativeTopFieldCollectorearly termination, as the ticket asks.Forces
TopFieldCollectorManager'stotalHitsThresholddown tolenwhensegmentTerminateEarly=true-- Lucene does the same per-segment skip natively.TopFieldCollector#isEarlyTerminated()isn't the right signal to read back (it also fires on routine hit-count overflow, unrelated to sort), so a thinSegmentTerminatedEarlyObservercatchesCollectionTerminatedExceptiondirectly instead -- Lucene only throws that from the sort-compatible fast path. With this,TestSegmentSortingpasses with its original, unmodified assertions.One gap:
RankQuery+segmentTerminateEarly=true(untested before too) now logs "unsupported combination" instead of silently no-oping.Also deprecates the public surface (
SEGMENT_TERMINATE_EARLY, the response header,QueryCommand's accessors) -- only the internal class was deprecated before.AI-assisted (Claude Sonnet 5)