-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Extensible Completion Postings Formats #111494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 30 commits
a76cb4d
3a317a8
f62a7a7
ee4604d
c664739
4343eea
35eaba5
a088ae9
322d176
a88ead0
4fcb416
c9eed64
fc94a32
4a80fe3
da464b7
070d451
f243893
377759c
13b926a
bbf06c9
40fb72f
943047d
c91c23d
d1f4403
ade1546
a80e744
86d0f7c
56e2c5c
8858d39
e4659c2
f8a0c51
616dee8
35f66e8
2f16043
70f2143
23186f6
ae2a0af
2ab57cb
add1089
3f6a72e
a5a60ee
7d1d145
93db5b2
5a90785
3abc777
3e80ca9
c3da22f
ba4a91c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
pr: 111494 | ||
summary: Extensible Completion Postings Formats | ||
area: "Suggesters" | ||
type: enhancement | ||
issues: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
import org.elasticsearch.index.codec.bloomfilter.ES87BloomFilterPostingsFormat; | ||
import org.elasticsearch.index.codec.postings.ES812PostingsFormat; | ||
import org.elasticsearch.index.codec.tsdb.ES87TSDBDocValuesFormat; | ||
import org.elasticsearch.index.mapper.CompletionFieldMapper; | ||
import org.elasticsearch.index.mapper.IdFieldMapper; | ||
import org.elasticsearch.index.mapper.Mapper; | ||
import org.elasticsearch.index.mapper.MapperService; | ||
|
@@ -53,9 +54,9 @@ public PostingsFormat getPostingsFormatForField(String field) { | |
|
||
private PostingsFormat internalGetPostingsFormatForField(String field) { | ||
if (mapperService != null) { | ||
final PostingsFormat format = mapperService.mappingLookup().getPostingsFormat(field); | ||
if (format != null) { | ||
return format; | ||
Mapper mapper = mapperService.mappingLookup().getMapper(field); | ||
if (mapper instanceof CompletionFieldMapper) { | ||
return CompletionFieldMapper.postingsFormat(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rjernst asked me to clean this up to work similarly to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to following up with moving the contents of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
// return our own posting format using PFOR | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
package org.elasticsearch.internal; | ||
|
||
import org.apache.lucene.search.suggest.document.CompletionPostingsFormat; | ||
|
||
/** | ||
* Allows plugging-in the Completions Postings Format. | ||
*/ | ||
public interface CompletionsPostingsFormatExtension { | ||
javanna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Returns the name of the {@link CompletionPostingsFormat} that Elasticsearch should use. Should return null if the extension | ||
* is not enabled. | ||
*/ | ||
String getFormatName(); | ||
|
||
/** | ||
* Sets whether this extension is enabled. If the extension is not enabled, {@link #getFormatName()} should return null. | ||
* <p> | ||
* This allows all nodes to be upgraded to a version that supports the extension before it is enabled. | ||
*/ | ||
void setExtensionEnabled(boolean isExtensionEnabled); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you help me understand why we need to add
org.elasticsearch.serverless.codec
here as well and below, and why we need to addorg.elasticsearch.stateless
here too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup,
org.elasticsearch.serverless.codec
andorg.elasticsearch.stateless
are separate java modules. Each requires access to theCompletionsPostingsFormatExtension
found in theorg.elasticsearch.internal
java module.@rjernst mind checking this as well to ensure this is correct? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe
org.elasticsearch.stateless
should not be needed here, but codec is so that it can implement the internal spi.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rjernst. I'll remove the export to
stateless
now that we are no longer using the Feature (for which this was previously required).