T3214 mappings exclude vectors#4758
Conversation
Merge PR to keep BBC Images Main in step with GNM Main - 2026-05-26
…om the ES index if not required
…denseVectorField mappings have been excluded
| (implicit ex: ExecutionContext, logMarker: LogMarker): Future[SearchResults] = { | ||
| if (!includeDenseVectorMappings) { | ||
| logger.warn(logMarker, "knnSearch called but includeDenseVectorMappings=false, returning empty results") | ||
| return Future.successful(SearchResults(Nil, total = 0, extraCounts = None)) |
There was a problem hiding this comment.
I think return would be redundant here since in Scala the last expression in a method is implicitly returned, so:
if (!includeDenseVectorMappings) {
logger.warn(...)
Future.successful(SearchResults(Nil, total = 0, extraCounts = None))
}
Should be fine
There was a problem hiding this comment.
This is usually true, but in this case this line is not the last expression in the method, so it is not implicitly returned. Either the remaining expressions in the method need to be inside an else block, or we need to use the return keyword (in general I'd recommend avoiding the return keyword as it does not always behave as you might expect, see https://tpolecat.github.io/2014/05/09/return.html. In this case I think it'd technically be fine here, but it's usually better to avoid it out of principle, to avoid needing to think too hard about whether it will definitely do what you want it to)
…into t3214-mappings-exclude-vectors
|
Have added else clauses to make early return pattern explicit and also resolved conflicts |
andrew-nowak
added
the
maintenance
|
Seen on auth, metadata-editor, cropper, collections, kahuna (created by @AndyKilmory and merged by @andrew-nowak 9 minutes and 44 seconds ago) Please check your changes! |
|
Seen on thrall, leases, usage (created by @AndyKilmory and merged by @andrew-nowak 9 minutes and 51 seconds ago) Please check your changes! |
|
Seen on media-api (created by @AndyKilmory and merged by @andrew-nowak 9 minutes and 55 seconds ago) Please check your changes! |
|
Seen on image-loader (created by @AndyKilmory and merged by @andrew-nowak 10 minutes ago) Please check your changes! |
|
Seen on image-loader (created by @AndyKilmory and merged by @andrew-nowak 10 minutes and 1 second ago) Please check your changes! |
3 participants
What does this change?
This adds a new configuration parameter - es.includeDenseVectorMappings - which can take values 'true' or 'false' (default value if not included = true) and will control whether the DenseVectorField used for adding embedding vector data to the Elasticsearch/Opensearch index will be included or not. False will exclude the mapping structures from the index and so allow continued use with Opensearch (post fork) indexes without modification until a longer term solution can be established.
The config parameter should be added to Thrall and Media-Api services to fully resolve all the issues.
In addition the parameter has been used to add in checks to the knnSearch and hybridSearch to return empty results sets when the embedding data has been excluded from the index.
How should a reviewer test this change?
Ensure that the ES/Opensearch index will operate as expected and the api and thrall services run as expected with both settings for the parameter - true and false.
Who should look at this?
Tested? Documented?