Skip to content

test: fix SparseVectorFieldMapperTests edge case #132937

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mromaios
Copy link
Contributor

@mromaios mromaios commented Aug 14, 2025

Closes #132810
Follow up from: #132264

This PR:

  • Fixes an edge case test failure for when the index contains 2 segments
  • Adds additional indexVersions to test (v_8x coverage)
  • Unmutes the SparseVectorFieldMapperTest
  • Improves randomisation for indexVersions

@mromaios mromaios requested a review from Mikep86 August 14, 2025 15:25
@mromaios mromaios self-assigned this Aug 14, 2025
@mromaios mromaios added >test Issues or PRs that are addressing/adding tests >test-failure Triaged test failures from CI :SearchOrg/Relevance Label for the Search (solution/org) Relevance team labels Aug 14, 2025
@elasticsearchmachine elasticsearchmachine added Team:SearchOrg Meta label for the Search Org (Enterprise Search) needs:risk Requires assignment of a risk label (low, medium, blocker) Team:Search - Relevance The Search organization Search Relevance team v9.2.0 labels Aug 14, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/search-eng (Team:SearchOrg)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/search-relevance (Team:Search - Relevance)

@mromaios mromaios removed >test-failure Triaged test failures from CI needs:risk Requires assignment of a risk label (low, medium, blocker) labels Aug 14, 2025
Copy link
Contributor

@Mikep86 Mikep86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks investigating and implementing the fix! I left one material comment about how we're getting index versions, other than that we're good

Comment on lines 846 to 848
IndexVersion indexVersion = ESTestCase.randomFrom(
IndexVersionUtils.allReleasedVersions().subSet(SPARSE_VECTOR_PRUNING_INDEX_OPTIONS_SUPPORT_BACKPORT_8_X, IndexVersion.current())
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a TODO here about why we are using randomFrom and that we should go back to IndexVersionUtils.randomVersionBetween once issues have been resolved with that?

The other thing we would want to improve here is making it equally likely we get one of these cases:

  • 8.x index version prior to index options support
  • 8.x index version with index options support
  • 9.x index version prior to index options support
  • 9.x index version with index options support

As written, the likelihood that we get an index version prior to index options support will shrink as the number of versions between SPARSE_VECTOR_PRUNING_INDEX_OPTIONS_SUPPORT_BACKPORT_8_X and IndexVersion.current() increases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Updated in 1b60a47

@mromaios mromaios requested review from markjhoy and kderusso August 15, 2025 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:SearchOrg/Relevance Label for the Search (solution/org) Relevance team Team:Search - Relevance The Search organization Search Relevance team Team:SearchOrg Meta label for the Search Org (Enterprise Search) >test Issues or PRs that are addressing/adding tests v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] SparseVectorFieldMapperTests testPruningScenarios failing
3 participants