-
Notifications
You must be signed in to change notification settings - Fork 25.6k
refactor(test): add sparse vector pruning tests #132264
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
refactor(test): add sparse vector pruning tests #132264
Conversation
…omaios/elasticsearch into increase_sparse_pruning_test_coverage
| NO_PRUNING, // No pruning applied - all tokens preserved | ||
| DEFAULT_PRUNING, // Default pruning configuration | ||
| STRICT_PRUNING // Stricter pruning with higher thresholds | ||
| } |
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 started of with these 3 scenarios for index and query configs as well, but quickly realised that those have 2 core dimensions. prune and pruningConfig, which for index you need to have prune=true to set a pruningConfig but that's not the case for queries, where the user can send any combination. This lead to the creation of separate enums for query and index scenarios.
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/search-eng (Team:SearchOrg) |
|
Pinging @elastic/search-relevance (Team:Search - Relevance) |
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Show resolved
Hide resolved
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.
Nice cleanup! I left some comments about where we have opportunities to streamline even further :)
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Outdated
Show resolved
Hide resolved
…omaios/elasticsearch into increase_sparse_pruning_test_coverage
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 iterating!
server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
Show resolved
Hide resolved
|
I configured the PR to backport these changes to 9.1 & 8.19. We'll need to do some adjustments to the index version checks in the 8.19 backport, but it shouldn't be too bad. |
Thanks for all the support throughout!
Sounds good! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
(cherry picked from commit 6cd330c) # Conflicts: # server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
(cherry picked from commit 6cd330c) # Conflicts: # server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
|
Oops we have some failures: Investigating |
| performTypeQueryFinalizationTest(mapperService, null, null, false); | ||
| } | ||
| if (shouldPrune == null) { | ||
| shouldPrune = indexVersion.onOrAfter(SPARSE_VECTOR_PRUNING_INDEX_OPTIONS_SUPPORT); |
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.
One thing I overlooked here is that we also need to handle 8.x index versions that support default token pruning. This check needs to change to something like:
shouldPrune = indexVersion.between(IndexVersions.SPARSE_VECTOR_PRUNING_INDEX_OPTIONS_SUPPORT_BACKPORT_8_X, IndexVersions.UPGRADE_TO_LUCENE_10_0_0) || indexVersion.onOrAfter(SPARSE_VECTOR_PRUNING_INDEX_OPTIONS_SUPPORT)
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.
Good catch. Will add it, thanks!
This PR
SparseVectorFieldMapperTestsby adding documents with various frequency tokens in the mock index and accompanying query vector set so that we can test 3 pruning scenarios (NO_PRUNING, DEFAULT_PRUNING, STRICT_PRUNING) more effectively.