You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "romseygeek (via GitHub)" <gi...@apache.org> on 2026/02/20 16:13:49 UTC
[PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
romseygeek opened a new pull request, #15741:
URL: https://github.com/apache/lucene/pull/15741
We inadvertently removed a branch where a segment without a sort field would create a postings-based competitive state that generated an empty iterator, meaning that searches against such segments would waste time checking every document rather than early terminating. This re-adds that logic, introducing a simpler skip-all-docs implementation and adding explicit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #15741:
URL: https://github.com/apache/lucene/pull/15741#discussion_r2833988403
##########
lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java:
##########
@@ -511,6 +516,18 @@ private abstract static class CompetitiveState {
abstract void update(int minOrd, int maxOrd) throws IOException;
}
+ private static class EmptyCompetitiveState extends CompetitiveState {
+
+ EmptyCompetitiveState(LeafReaderContext context) {
+ super(context);
+ }
Review Comment:
I think here, we should set `iterator.update(DocIdSetIterator.empty())` from the get go right? The competitivestate sets it to `all` initially.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
Posted by "romseygeek (via GitHub)" <gi...@apache.org>.
romseygeek commented on code in PR #15741:
URL: https://github.com/apache/lucene/pull/15741#discussion_r2834335250
##########
lucene/core/src/test/org/apache/lucene/search/TestSortOptimization.java:
##########
@@ -1071,6 +1071,100 @@ private void testStringSortOptimizationWithMissingValues(
dir.close();
}
+ public void testStringSortOptimizationFieldMissingInSegmentBasedPostings() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(
+ (field, value) -> new KeywordField(field, value, Field.Store.NO));
+ }
+
+ public void testStringSortOptimizationFieldMissingInSegmentBasedDVSkipper() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(SortedDocValuesField::indexedField);
+ }
+
+ /**
+ * Test that when a segment doesn't contain the sort field at all (fieldInfo == null), the
+ * optimization still works. All docs in such a segment have missing values, and when missing
+ * values are non-competitive the entire segment should be skippable.
+ */
+ private void testStringSortOptimizationFieldMissingInSegment(
+ BiFunction<String, BytesRef, IndexableField> fieldsBuilder) throws IOException {
+ final Directory dir = newDirectory();
+ final IndexWriter writer =
+ new IndexWriter(dir, new IndexWriterConfig().setMergePolicy(newLogMergePolicy()));
Review Comment:
We create one segment with 20 docs that have the value, then flush, and then add a whole bunch of documents with no values. I don't think it would be possible to get no segments that have no values? Unless it decided to merge everything down to a single segment, I suppose. Yeah, ok, maybe a NoMergePolicy is better here if we need a deterministic index geometry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
Posted by "romseygeek (via GitHub)" <gi...@apache.org>.
romseygeek commented on code in PR #15741:
URL: https://github.com/apache/lucene/pull/15741#discussion_r2834012380
##########
lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java:
##########
@@ -511,6 +516,18 @@ private abstract static class CompetitiveState {
abstract void update(int minOrd, int maxOrd) throws IOException;
}
+ private static class EmptyCompetitiveState extends CompetitiveState {
+
+ EmptyCompetitiveState(LeafReaderContext context) {
+ super(context);
+ }
Review Comment:
It needs to be set to `all` initially as it may get used before the hit queue is filled. It gets set to empty as soon as we have collected enough documents to start updating bottom values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #15741:
URL: https://github.com/apache/lucene/pull/15741#discussion_r2834064711
##########
lucene/core/src/test/org/apache/lucene/search/TestSortOptimization.java:
##########
@@ -1071,6 +1071,72 @@ private void testStringSortOptimizationWithMissingValues(
dir.close();
}
+ public void testStringSortOptimizationFieldMissingInSegmentBasedPostings() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(
+ (field, value) -> new KeywordField(field, value, Field.Store.NO));
+ }
+
+ public void testStringSortOptimizationFieldMissingInSegmentBasedDVSkipper() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(SortedDocValuesField::indexedField);
+ }
+
+ /**
+ * Test that when a segment doesn't contain the sort field at all (fieldInfo == null), the
+ * optimization still works. All docs in such a segment have missing values, and when missing
+ * values are non-competitive the entire segment should be skippable.
+ */
Review Comment:
I think we should also do a test when its `missing_last` but reversed sorted and `missing_first` but naturally sorted. These would obviously NOT exit early (e.g. they aren't competitive).
##########
lucene/core/src/java/org/apache/lucene/search/comparators/TermOrdValComparator.java:
##########
@@ -511,6 +516,18 @@ private abstract static class CompetitiveState {
abstract void update(int minOrd, int maxOrd) throws IOException;
}
+ private static class EmptyCompetitiveState extends CompetitiveState {
+
+ EmptyCompetitiveState(LeafReaderContext context) {
+ super(context);
+ }
+
+ @Override
+ void update(int minOrd, int maxOrd) throws IOException {
+ iterator.update(DocIdSetIterator.empty());
+ }
Review Comment:
So, reading through all the logic, I honestly think the "empty" logic is handled directly in the `TermOrdValLeafComparator` and the iterators will see `0, 0` in the update. So, this state is only here to indicate that competitive iterating is possible and all the "missing" handling logic (e.g. can stop on asc, but cannot stop on desc) seems to be done elsewhere.
This is a nice little clarification and minor optimization even over what was done before :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
Posted by "romseygeek (via GitHub)" <gi...@apache.org>.
romseygeek commented on code in PR #15741:
URL: https://github.com/apache/lucene/pull/15741#discussion_r2834168657
##########
lucene/core/src/test/org/apache/lucene/search/TestSortOptimization.java:
##########
@@ -1071,6 +1071,72 @@ private void testStringSortOptimizationWithMissingValues(
dir.close();
}
+ public void testStringSortOptimizationFieldMissingInSegmentBasedPostings() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(
+ (field, value) -> new KeywordField(field, value, Field.Store.NO));
+ }
+
+ public void testStringSortOptimizationFieldMissingInSegmentBasedDVSkipper() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(SortedDocValuesField::indexedField);
+ }
+
+ /**
+ * Test that when a segment doesn't contain the sort field at all (fieldInfo == null), the
+ * optimization still works. All docs in such a segment have missing values, and when missing
+ * values are non-competitive the entire segment should be skippable.
+ */
Review Comment:
I tried to assert that no pruning happens, but it seems it can still do some pruning on the earlier segment. So instead I added test cases that run the query with pruning enabled and disabled and asserted that the results are the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
Posted by "martijnvg (via GitHub)" <gi...@apache.org>.
martijnvg commented on code in PR #15741:
URL: https://github.com/apache/lucene/pull/15741#discussion_r2834277480
##########
lucene/core/src/test/org/apache/lucene/search/TestSortOptimization.java:
##########
@@ -1071,6 +1071,100 @@ private void testStringSortOptimizationWithMissingValues(
dir.close();
}
+ public void testStringSortOptimizationFieldMissingInSegmentBasedPostings() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(
+ (field, value) -> new KeywordField(field, value, Field.Store.NO));
+ }
+
+ public void testStringSortOptimizationFieldMissingInSegmentBasedDVSkipper() throws IOException {
+ testStringSortOptimizationFieldMissingInSegment(SortedDocValuesField::indexedField);
+ }
+
+ /**
+ * Test that when a segment doesn't contain the sort field at all (fieldInfo == null), the
+ * optimization still works. All docs in such a segment have missing values, and when missing
+ * values are non-competitive the entire segment should be skippable.
+ */
+ private void testStringSortOptimizationFieldMissingInSegment(
+ BiFunction<String, BytesRef, IndexableField> fieldsBuilder) throws IOException {
+ final Directory dir = newDirectory();
+ final IndexWriter writer =
+ new IndexWriter(dir, new IndexWriterConfig().setMergePolicy(newLogMergePolicy()));
Review Comment:
I wonder whether this merge policy is good enough for this test case, so that we don't have rare runs where we somehow don't have an empty segment?
I think the `atLeast(10000)` should cover this in line 1105? But maybe there is a better way to enforce this. Like using the no merge policy?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Correctly handle sort pruning on segments where the sort field does not exist [lucene]
Posted by "romseygeek (via GitHub)" <gi...@apache.org>.
romseygeek merged PR #15741:
URL: https://github.com/apache/lucene/pull/15741
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org