-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Allow update any index type to bbq_disk #131760
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
Allow update any index type to bbq_disk #131760
Conversation
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
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'm interested in other tests that we could do in YAML. I extended the ones we currently have to check that we can switch to bbq_disk.
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.
you might want to add "bbq_disk" (and perhaps "bbq_flat" and "bbq_hnsw"? with a random choice) to the updated cluster mapping in DenseVectorMappingUpdateIT.
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.
you might want to add "bbq_disk" (and perhaps "bbq_flat" and "bbq_hnsw"? with a random choice) to the updated cluster mapping in DenseVectorMappingUpdateIT
Thought about that - but IIUC the mapping is applied before the upgrade, so I'm not sure if the mapping will be supported by the older node 🤔
I believe we can create a separate IT test for updating the mapping and index / search, similar to what the YAML test does, but in a randomized manner. That would add coverage to the YAML tests and probably remove some of them.
I think that should be done as a separate PR - but happy to work on that if y'all think is worhtwhile 👍
| - method: POST | ||
| path: /_search | ||
| capabilities: [ optimized_scalar_quantization_bbq ] | ||
| capabilities: [ update_field_to_bbq_disk, dense_vector_updatable_bbq ] |
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 changed some capabilities, as they were referring to /_mapping instead of /_search - we were skipping some tests due to the capabilities not being present in _search.
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 with a minor comment re tests
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.
you might want to add "bbq_disk" (and perhaps "bbq_flat" and "bbq_hnsw"? with a random choice) to the updated cluster mapping in DenseVectorMappingUpdateIT.
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.
you made it look easy; lgtm
|
Added more comprehensive update tests in #131821 |
Allow updates from any index type to
bbq_disk.For the moment, no further restrictions have been applied to updating from
bbq_disktobbq_disk.Closes #131461