lucene_snapshot: Update to new Lucene 10.3 postings format #128240

ChrisHegarty · 2025-05-21T11:55:51Z

There are a few different things going on in this PR, all of which are required to get the lucene_snspahot branch building again, but the most substantial is the update to the new Lucene 10.3 postings format, Lucene103PostingsFormat.

Additionally, changes are required because of the removal of deprecated methods in IOContext, as well as the override of hints for direct IO.

elasticsearchmachine · 2025-05-21T11:56:15Z

Pinging @elastic/es-search (Team:Search)

ChrisHegarty · 2025-05-21T11:56:36Z

server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java

@@ -318,7 +318,10 @@ public void testRangeQuery() throws IOException {
            LongPoint.newRangeQuery("field", instant1, instant2),
            SortedNumericDocValuesField.newSlowRangeQuery("field", instant1, instant2)
        );
+        // TODO: temporarily disable - the range query rewrites to a MatchNoDocsQuery


I'm going to follow up on this.

Interesting, shout if you need help on this! this is probably due to apache/lucene#14609 ?

I guess that this case can no longer be tested the way that we do, because we have no data in the index have we?

Yeah, again I think that the assertions are too strict, and in fact assert an implementation detail. Likely just need to adjust what is expected ( once I spend a little more time on it ).

ChrisHegarty · 2025-05-21T11:56:52Z

server/src/test/java/org/elasticsearch/search/query/QueryPhaseTests.java

@@ -952,8 +959,9 @@ public void testMaxScoreQueryVisitor() {

    // assert score docs are in order and their number is as expected
    private static void assertSortResults(TopDocs topDocs, long totalNumDocs, boolean isDoubleSort) {
-        assertEquals(TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO, topDocs.totalHits.relation());
-        assertThat(topDocs.totalHits.value(), lessThan(totalNumDocs)); // we collected less docs than total number
+        // TODO: fix java.lang.AssertionError: expected:<GREATER_THAN_OR_EQUAL_TO> but was:<EQUAL_TO>


I'm going to follow up on this.

Is there anything to do other than update the assertions here?

yeah, I think that the outcome here will be to adjust the assertions - I just wanna spend a little more time on it before doing that, so as to satisfy myself complete! :-)

ChrisHegarty · 2025-05-21T11:57:40Z

server/src/main/java/org/elasticsearch/index/codec/PerFieldFormatSupplier.java

@@ -35,12 +36,12 @@
 */
 public class PerFieldFormatSupplier {
    public static final FeatureFlag USE_LUCENE101_POSTINGS_FORMAT = new FeatureFlag("use_lucene101_postings_format");
+    public static final FeatureFlag USE_LUCENE103_POSTINGS_FORMAT = new FeatureFlag("use_lucene103_postings_format");


This will be removed once #128236 is merged.

thecoop · 2025-05-21T12:26:36Z

...main/java/org/elasticsearch/index/codec/vectors/es818/DirectIOLucene99FlatVectorsFormat.java

    @Override
    public FlatVectorsReader fieldsReader(SegmentReadState state) throws IOException {
        SegmentReadState directIOState = new SegmentReadState(
            state.directory,
            state.segmentInfo,
            state.fieldInfos,
-            DIRECT_IO_CONTEXT,
+            state.context.withHints(DirectIOHint.INSTANCE),


I used a specific implementation of IOContext so the hints weren't overridden by calls to withHints within Lucene99FlatVectorsReader. I should have commented that.

I was thinking that we should not be changing the context if it was a merge or flush. Should withHints be additive rather than replace? Or we otherwise build a new hint set from the existing hints along with directIO ?

Ah yes, we have just discovered that other directory implementations can be used. So DirectIOHint should probably be added to all the others - we can modify our implementation here to add DirectIO to any withHints that are specified

javanna

I left some comments, thanks @ChrisHegarty !

javanna · 2025-05-21T13:16:25Z

server/src/main/java/module-info.java

@@ -461,7 +461,8 @@
            org.elasticsearch.index.codec.Elasticsearch814Codec,
            org.elasticsearch.index.codec.Elasticsearch816Codec,
            org.elasticsearch.index.codec.Elasticsearch900Codec,
-            org.elasticsearch.index.codec.Elasticsearch900Lucene101Codec;
+            org.elasticsearch.index.codec.Elasticsearch900Lucene101Codec,
+            org.elasticsearch.index.codec.Elasticsearch902Lucene103Codec;


that makes me wonder why I used 900 instead of 90 back when I added it in the first place. 9.2.0 could have been 92 as well and perhaps 902 makes it look like a bugfix release which it is not.

Oh good point. So this should be Elasticsearch92Lucene103Codec, then right ? I'll update it.

I think it would be better yea. pardon the inconsistency with 900 that we will have to live with.

javanna · 2025-05-21T13:21:10Z

server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java

@@ -318,7 +318,10 @@ public void testRangeQuery() throws IOException {
            LongPoint.newRangeQuery("field", instant1, instant2),
            SortedNumericDocValuesField.newSlowRangeQuery("field", instant1, instant2)
        );
+        // TODO: temporarily disable - the range query rewrites to a MatchNoDocsQuery


Interesting, shout if you need help on this! this is probably due to apache/lucene#14609 ?

javanna · 2025-05-21T13:25:18Z

server/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java

@@ -318,7 +318,10 @@ public void testRangeQuery() throws IOException {
            LongPoint.newRangeQuery("field", instant1, instant2),
            SortedNumericDocValuesField.newSlowRangeQuery("field", instant1, instant2)
        );
+        // TODO: temporarily disable - the range query rewrites to a MatchNoDocsQuery


I guess that this case can no longer be tested the way that we do, because we have no data in the index have we?

javanna · 2025-05-21T13:26:16Z

server/src/test/java/org/elasticsearch/search/query/QueryPhaseTests.java

@@ -952,8 +959,9 @@ public void testMaxScoreQueryVisitor() {

    // assert score docs are in order and their number is as expected
    private static void assertSortResults(TopDocs topDocs, long totalNumDocs, boolean isDoubleSort) {
-        assertEquals(TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO, topDocs.totalHits.relation());
-        assertThat(topDocs.totalHits.value(), lessThan(totalNumDocs)); // we collected less docs than total number
+        // TODO: fix java.lang.AssertionError: expected:<GREATER_THAN_OR_EQUAL_TO> but was:<EQUAL_TO>


Is there anything to do other than update the assertions here?

javanna · 2025-05-21T13:26:41Z

server/src/test/java/org/elasticsearch/search/query/QueryPhaseTests.java

@@ -817,6 +817,13 @@ public void testNumericSortOptimization() throws Exception {

        Query q = LongPoint.newRangeQuery(fieldNameLong, startLongValue, startLongValue + numDocs);

+        // 0. test assertion - the query rewritten to a match all - https://github.com/apache/lucene/pull/14609/
+        // TODO: reflow total hits expectations


this refers to the other TODO below or is it something else?

javanna · 2025-05-21T13:27:22Z

server/src/main/java/org/elasticsearch/index/codec/postings/Lucene90BlockTreeTermsWriter.java

@@ -0,0 +1,1191 @@
+/*


Are we forking this from Lucene? Why so?

Unfortunately we have to copy (not fork per se). Since the writer has been moved to backward compat test in Lucene - so is not in any shipping Lucene jar :-(

And, why do we need it again? Sorry if it's a dumb question, it is not obvious to me.

javanna · 2025-05-21T13:27:45Z

server/src/main/java/org/elasticsearch/index/codec/postings/CompressionAlgorithm.java

+import java.io.IOException;
+
+/** Compression algorithm used for suffixes of a block of terms. */
+public enum CompressionAlgorithm {


this class is somehow surprising, what makes it required to fork on our end?

Same as above.

Let's adjust the access modifier in Lucene to avoid this needless copying - apache/lucene#14695

lucene_snapshot: Update to new Lucene 10.3 postings format

60b9f59

ChrisHegarty added >non-issue :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team labels May 21, 2025

ChrisHegarty commented May 21, 2025

View reviewed changes

thecoop reviewed May 21, 2025

View reviewed changes

javanna reviewed May 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lucene_snapshot: Update to new Lucene 10.3 postings format #128240

lucene_snapshot: Update to new Lucene 10.3 postings format #128240

ChrisHegarty commented May 21, 2025

elasticsearchmachine commented May 21, 2025

ChrisHegarty May 21, 2025

javanna May 21, 2025

javanna May 21, 2025

ChrisHegarty May 21, 2025

ChrisHegarty May 21, 2025

javanna May 21, 2025

ChrisHegarty May 21, 2025

ChrisHegarty May 21, 2025

thecoop May 21, 2025 •

edited

Loading

ChrisHegarty May 21, 2025

thecoop May 21, 2025 •

edited

Loading

javanna left a comment

javanna May 21, 2025

ChrisHegarty May 21, 2025

javanna May 21, 2025

javanna May 21, 2025

javanna May 21, 2025

javanna May 21, 2025

javanna May 21, 2025

javanna May 21, 2025

ChrisHegarty May 21, 2025

javanna May 21, 2025

javanna May 21, 2025

ChrisHegarty May 21, 2025

ChrisHegarty May 21, 2025

lucene_snapshot: Update to new Lucene 10.3 postings format #128240

Are you sure you want to change the base?

lucene_snapshot: Update to new Lucene 10.3 postings format #128240

Conversation

ChrisHegarty commented May 21, 2025

elasticsearchmachine commented May 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecoop May 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecoop May 21, 2025 • edited Loading

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecoop May 21, 2025 •

edited

Loading

thecoop May 21, 2025 •

edited

Loading