-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Make bbq_hnsw the default index option for dense-vector fields with more than 384 dimensions #129825
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
Make bbq_hnsw the default index option for dense-vector fields with more than 384 dimensions #129825
Conversation
…ore than 384 dimensions
|
🔍 Preview links for changed docs: 🔔 The preview site may take up to 3 minutes to finish building. These links will become live once it completes. |
|
Pinging @elastic/es-docs (Team:Docs) |
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Setting it for review, but there are some test cases failing for |
...c/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/70_dense_vector_telemetry.yml
Show resolved
Hide resolved
benwtrent
left a comment
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.
We need some tests around default values and ensuring that dynamic dims still have the default of bbq hnsw set.
| // This is defined as updatable because it can be updated once, from [null] to a valid dim size, | ||
| // by a dynamic mapping update. Once it has been set, however, the value cannot be changed. | ||
| this.dims = new Parameter<>("dims", true, () -> null, (n, c, o) -> { |
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 have a sneaky suspicion that if dims are not set, but we then index a vector of 384 dims, and set the dims to 384, the index type will not be set to bbq_hnsw.
Can you write a test to confirm/deny this?
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.
Yeap, your suspicion was right :) Updating to use the builder's dimension setter when reading the float array., so that we can have access to that when setting up default value.
Added tests in DynamicMappingIT and DynamicMappingTests.
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.
@pmpailis there is still a bug.
PUT vectors
{
"mappings": {
"properties": {
"vector": {
"type": "dense_vector"
}
}
}
}
POST vectors/_doc/1
{
"vector": [0.1, 0.2, 0.3, 0.4, 0.5,..., 0.385]
}
GET vectors
See that the mapped type is not bbq_hnsw.
Its possible to create a dense_vector mapping with a null dims. Then later, when the doc is parsed, we update the mapping to apply that dim update.
I think in that same update, we should adjust the default index type to ensure its applied.
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 guess the tricky part is knowing if index_options were set by the user (specifically to the defaults, or whatever), or not set at all.
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.
oh, that's a nice catch. I've tried restoring the default value to null if dims are not present, and resetting this once we have a document in place. So now:
- if a user provides dims, then we take them into account from the beginning to setup a default value
- if a user does not provide any dimensions, then the mappings do not initialize any information for the index_options either, and we instead delay this decision for when we know dims as well
For example:
PUT vectors
{
"mappings": {
"properties": {
"vector": {
"type": "dense_vector"
}
}
}
}
GET _mappings
->
{
"vectors": {
"mappings": {
"properties": {
"vector": {
"type": "dense_vector",
"index": true,
"similarity": "cosine"
}
}
}
}
}
PUT vectors/_doc/1
{
"vector": [1.2, 3.4, ..., 8.9]
}
GET _mappings
->
{
"vectors": {
"mappings": {
"properties": {
"vector": {
"type": "dense_vector",
"dims": 1512,
"index": true,
"similarity": "cosine",
"index_options": {
"type": "bbq_hnsw",
"m": 16,
"ef_construction": 100,
"rescore_vector": {
"oversample": 3
}
}
}
}
}
}
}
Not overly familiar with mappings and how they're updated, so i might be missing something (or if this considered breaking change). Does the above make sense?
Added a take on this in ea7c8a2 and a test for the above scenario in DynamicMappingIT#testBBQDynamicMappingWhenFirstIngestingDoc
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.
(there is some code duplication that we could potentially avoid if we are to go with this)
…ing & adding tests
…ings have changed with previous major
| } | ||
|
|
||
| private DenseVectorIndexOptions defaultIndexOptions(boolean defaultInt8Hnsw, boolean defaultBBQHnsw) { | ||
| if (this.dims != null && this.dims.isConfigured() && elementType.getValue() == ElementType.FLOAT && this.indexed.getValue()) { |
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.
@benwtrent updated this so if dims is not provided, we postpone deciding which IndexOptions to create until we actually know the dimensions (in DenseVectorFieldMapper#parse)
|
buildkite test this please |
jimczi
left a comment
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.
LGTM
|
Thanks for the reviews @benwtrent , @jimczi and @tteofili ! @benwtrent I'll proceed with merging this PR, we can address any leftover comments you may have in a follow up :) |
…ore than 384 dimensions (elastic#129825)
In elastic#129825, we modified the dense_vector field type to delay setting index options until the field's dimensions are known. However, this introduced a discrepancy for indices created before that change, which would previously default to int8_hnsw even when dimensions were not set. This discrepancy leads to an assertion failure in mixed-version clusters, where the serialized mappings differ between nodes: ``` [2025-07-02T20:37:29,852][ERROR][o.e.b.ElasticsearchUncaughtExceptionHandler] [v9.0.4-2] fatal error in thread [elasticsearch[v9.0.4-2][clusterApplierService#updateTask][T#1]], exiting java.lang.AssertionError: provided source [{"_doc":{"properties":{"vector":{"type":"dense_vector","index":true,"similarity":"cosine"}}}}] differs from mapping [{"_doc":{"properties":{"vector":{"type":"dense_vector","index":true,"similarity":"cosine","index_options":{"type":"int8_hnsw","m":16,"ef_construction":100}}}}}] ``` This commit resolves the issue by ensuring that indices created before the change continue to default to int8_hnsw index options, even if dimensions remain unset.
#130540) In #129825, we modified the dense_vector field type to delay setting index options until the field's dimensions are known. However, this introduced a discrepancy for indices created before that change, which would previously default to int8_hnsw even when dimensions were not set. This discrepancy leads to an assertion failure in mixed-version clusters, where the serialized mappings differ between nodes: ``` [2025-07-02T20:37:29,852][ERROR][o.e.b.ElasticsearchUncaughtExceptionHandler] [v9.0.4-2] fatal error in thread [elasticsearch[v9.0.4-2][clusterApplierService#updateTask][T#1]], exiting java.lang.AssertionError: provided source [{"_doc":{"properties":{"vector":{"type":"dense_vector","index":true,"similarity":"cosine"}}}}] differs from mapping [{"_doc":{"properties":{"vector":{"type":"dense_vector","index":true,"similarity":"cosine","index_options":{"type":"int8_hnsw","m":16,"ef_construction":100}}}}}] ``` This commit resolves the issue by ensuring that indices created before the change continue to default to int8_hnsw index options, even if dimensions remain unset.
elastic#130540) In elastic#129825, we modified the dense_vector field type to delay setting index options until the field's dimensions are known. However, this introduced a discrepancy for indices created before that change, which would previously default to int8_hnsw even when dimensions were not set. This discrepancy leads to an assertion failure in mixed-version clusters, where the serialized mappings differ between nodes: ``` [2025-07-02T20:37:29,852][ERROR][o.e.b.ElasticsearchUncaughtExceptionHandler] [v9.0.4-2] fatal error in thread [elasticsearch[v9.0.4-2][clusterApplierService#updateTask][T#1]], exiting java.lang.AssertionError: provided source [{"_doc":{"properties":{"vector":{"type":"dense_vector","index":true,"similarity":"cosine"}}}}] differs from mapping [{"_doc":{"properties":{"vector":{"type":"dense_vector","index":true,"similarity":"cosine","index_options":{"type":"int8_hnsw","m":16,"ef_construction":100}}}}}] ``` This commit resolves the issue by ensuring that indices created before the change continue to default to int8_hnsw index options, even if dimensions remain unset.
After the recent benchmarking deemed it sensible to do so, this PR changes the default index options for
floatvector fields with>= 384dimensions tobbq_hnsw. Otherwise, we keep theint8_hnswthat was set previously in #106836