Extensible Completion Postings Formats by JVerwolf · Pull Request #111494 · elastic/elasticsearch

JVerwolf · 2024-07-31T19:41:48Z

This PR allows the completion posting format loaded by ES to be extensible.

Lucene uses PostingsFormat.forName(String) to load a postings format identified by the String arg. This PR provides an extension that allows alternative names to be provided at the point we load postings format in ES at write-time. This name is then written to the shard info. When the shard is read at search-time, Lucene looks for postings formats with the same type on the classpath (as declared though module info via a provides directive), then picks the one with the same name.

elasticsearchmachine · 2024-07-31T19:42:11Z

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

elasticsearchmachine · 2024-07-31T19:42:11Z

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

JVerwolf · 2024-07-31T19:47:48Z

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

+            Mapper mapper = mapperService.mappingLookup().getMapper(field);
+            if (mapper instanceof CompletionFieldMapper) {
+                return CompletionFieldMapper.postingsFormat();


@rjernst asked me to clean this up to work similarly to getKnnVectorsFormatForField below. Now, the CompletionFieldMapper is solely responsible for exposing it's own postings format.

I find that this is indeed easier to follow. I have one minor suggestion for improvement: would it make sense to move the loading of the codec name etc. also to this class? Given it's just static code, I wonder if we can have it all in a single place. The main reason why I would do so is that we hardcode Competion912 , and I am anxious that changes may happen in Lucene and we may forget to update this string. It is probably easier to miss if it's directly in PerFieldFormatSupplier. We can also make this change later, it's not a big deal. This is a pre-existing problem and the code looks better with your change already.

+1 to following up with moving the contents of CompletionFieldMapper.postingsFormat() into here.

I suppose I have some confusion as the the purview of each class with respect to who ultimately is the source of truth for the format. To me it seems like the mapper should own it, though I see mixed examples of ownership here. I however agree it's cleaner to just move it here, so I'll do that.

server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java

server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java

…ent/make-fst-on-heap-configurable

elasticsearchmachine · 2024-07-31T22:13:46Z

Hi @JVerwolf, I've created a changelog YAML for you.

JVerwolf · 2024-08-01T19:33:37Z

@elasticmachine run elasticsearch-ci/validate-changelogs

…ent/make-fst-on-heap-configurable

benwtrent

Do we have a rolling upgrade test that will exercise this code ?

We need something that indexes with old format.

Indexes with a mixed cluster and merges some segments.

Indexes with new and merges some segments.

JVerwolf · 2024-08-09T18:18:55Z

Thanks for the review @benwtrent ! Good points, though IIUC, I think this applies more to code that will use this PR (that will provided another codec), rather than to this code, per se. Given that I do have a PR (in a private repo) that uses this, I'll work out how to add rolling upgrade tests there. WDYT?

…ent/make-fst-on-heap-configurable

javanna

This looks good, I left a couple of small comments.

javanna · 2024-10-21T20:09:38Z

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

+            Mapper mapper = mapperService.mappingLookup().getMapper(field);
+            if (mapper instanceof CompletionFieldMapper) {
+                return CompletionFieldMapper.postingsFormat();


I find that this is indeed easier to follow. I have one minor suggestion for improvement: would it make sense to move the loading of the codec name etc. also to this class? Given it's just static code, I wonder if we can have it all in a single place. The main reason why I would do so is that we hardcode Competion912 , and I am anxious that changes may happen in Lucene and we may forget to update this string. It is probably easier to miss if it's directly in PerFieldFormatSupplier. We can also make this change later, it's not a big deal. This is a pre-existing problem and the code looks better with your change already.

server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java

server/src/main/java/org/elasticsearch/internal/CompletionsPostingsFormatExtension.java

javanna · 2024-10-21T20:40:32Z

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

-            org.elasticsearch.serverless.constants;
+            org.elasticsearch.serverless.constants,
+            org.elasticsearch.serverless.codec,
+            org.elasticsearch.stateless;


could you help me understand why we need to add org.elasticsearch.serverless.codec here as well and below, and why we need to add org.elasticsearch.stateless here too?

Yup, org.elasticsearch.serverless.codec and org.elasticsearch.stateless are separate java modules. Each requires access to the CompletionsPostingsFormatExtension found in the org.elasticsearch.internal java module.

@rjernst mind checking this as well to ensure this is correct? Thanks!

I believe org.elasticsearch.stateless should not be needed here, but codec is so that it can implement the internal spi.

Thanks @rjernst. I'll remove the export to stateless now that we are no longer using the Feature (for which this was previously required).

…ent/make-fst-on-heap-configurable

rjernst

LGTM

rjernst · 2024-10-30T22:09:28Z

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

-            org.elasticsearch.serverless.constants;
+            org.elasticsearch.serverless.constants,
+            org.elasticsearch.serverless.codec,
+            org.elasticsearch.stateless;


I believe org.elasticsearch.stateless should not be needed here, but codec is so that it can implement the internal spi.

rjernst · 2024-10-30T22:10:57Z

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

+            Mapper mapper = mapperService.mappingLookup().getMapper(field);
+            if (mapper instanceof CompletionFieldMapper) {
+                return CompletionFieldMapper.postingsFormat();


+1 to following up with moving the contents of CompletionFieldMapper.postingsFormat() into here.

…ent/make-fst-on-heap-configurable

javanna

LGTM!

…ent/make-fst-on-heap-configurable

JVerwolf · 2024-11-28T17:11:07Z

@elasticmachine run elasticsearch-ci/part-3

A while ago we introduced a completion postings format extension to eventually be able to customize how completion FSTs are loaded. See elastic#111494. We have never leveraged this extension, and meanwhile Lucene is moving to always load FSTs off-heap, and no longer allow on-heap. See apache/lucene#14364 . This commit removes the SPI extension as it is no longer needed.

A while ago we introduced a completion postings format extension to eventually be able to customize how completion FSTs are loaded. See #111494. We have never leveraged this extension, and meanwhile Lucene is moving to always load FSTs off-heap, and no longer allow on-heap. See apache/lucene#14364 . This commit removes the SPI extension as it is no longer needed.

A while ago we introduced a completion postings format extension to eventually be able to customize how completion FSTs are loaded. See elastic#111494. We have never leveraged this extension, and meanwhile Lucene is moving to always load FSTs off-heap, and no longer allow on-heap. See apache/lucene#14364 . This commit removes the SPI extension as it is no longer needed.

JVerwolf added 2 commits July 31, 2024 11:44

Make completion postings format extensible

a76cb4d

Spotless

3a317a8

JVerwolf added :Search Foundations/Mapping Index mappings, including merging and defining field types :Search/Search Search-related issues that do not fall into other categories labels Jul 31, 2024

elasticsearchmachine added the v8.16.0 label Jul 31, 2024

elasticsearchmachine added Team:Search Meta label for search team Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Jul 31, 2024

Spotless

f62a7a7

JVerwolf commented Jul 31, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java Outdated Show resolved Hide resolved

JVerwolf commented Jul 31, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/MappingLookup.java Show resolved Hide resolved

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

ee4604d

…ent/make-fst-on-heap-configurable

JVerwolf requested review from jpountz and rjernst July 31, 2024 21:25

JVerwolf added the >enhancement label Jul 31, 2024

Update docs/changelog/111494.yaml

c664739

JVerwolf added 5 commits August 2, 2024 09:50

Fix docs

4343eea

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

35eaba5

…ent/make-fst-on-heap-configurable

Make completion postings format extensible

a088ae9

Spotless

322d176

Spotless

a88ead0

benwtrent reviewed Aug 3, 2024

View reviewed changes

JVerwolf added 3 commits August 9, 2024 14:56

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

4fcb416

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

c9eed64

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

fc94a32

…ent/make-fst-on-heap-configurable

javanna reviewed Oct 21, 2024

View reviewed changes

JVerwolf added 4 commits October 22, 2024 06:50

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

f8a0c51

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

616dee8

…ent/make-fst-on-heap-configurable

PR Feedback: remove feature

35f66e8

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

2f16043

…ent/make-fst-on-heap-configurable

rjernst approved these changes Oct 30, 2024

View reviewed changes

JVerwolf added 2 commits October 31, 2024 13:37

PR Feedback: remove export and move format

70f2143

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

23186f6

…ent/make-fst-on-heap-configurable

javanna approved these changes Nov 1, 2024

View reviewed changes

JVerwolf added 9 commits November 4, 2024 10:03

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

ae2a0af

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

2ab57cb

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

add1089

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

3f6a72e

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

a5a60ee

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

7d1d145

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

93db5b2

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

5a90785

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

3abc777

…ent/make-fst-on-heap-configurable

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Nov 14, 2024

JVerwolf added 3 commits November 18, 2024 14:53

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

3e80ca9

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

c3da22f

…ent/make-fst-on-heap-configurable

Merge branch 'main' of github.com:elastic/elasticsearch into enhancem…

ba4a91c

…ent/make-fst-on-heap-configurable

JVerwolf merged commit 8350ff2 into elastic:main Nov 28, 2024

javanna mentioned this pull request Mar 19, 2025

Remove completion postings format extension #125253

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible Completion Postings Formats#111494

Extensible Completion Postings Formats#111494
JVerwolf merged 48 commits intoelastic:mainfrom
JVerwolf:enhancement/make-fst-on-heap-configurable

JVerwolf commented Jul 31, 2024

elasticsearchmachine commented Jul 31, 2024

elasticsearchmachine commented Jul 31, 2024

JVerwolf Jul 31, 2024

javanna Oct 21, 2024

rjernst Oct 30, 2024

JVerwolf Oct 31, 2024

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 31, 2024

JVerwolf commented Aug 1, 2024

benwtrent left a comment

JVerwolf commented Aug 9, 2024 •

edited

Loading

javanna left a comment

javanna Oct 21, 2024

Uh oh!

Uh oh!

javanna Oct 21, 2024

JVerwolf Oct 29, 2024

rjernst Oct 30, 2024

JVerwolf Oct 31, 2024 •

edited

Loading

rjernst left a comment

rjernst Oct 30, 2024

rjernst Oct 30, 2024

javanna left a comment

JVerwolf commented Nov 28, 2024

Labels

6 participants

Conversation

JVerwolf commented Jul 31, 2024

elasticsearchmachine commented Jul 31, 2024

elasticsearchmachine commented Jul 31, 2024

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

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 31, 2024

JVerwolf commented Aug 1, 2024

benwtrent left a comment

Choose a reason for hiding this comment

JVerwolf commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JVerwolf Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

JVerwolf commented Nov 28, 2024

Labels

6 participants

JVerwolf commented Aug 9, 2024 •

edited

Loading

JVerwolf Oct 31, 2024 •

edited

Loading