Skip to content

Conversation

@kderusso
Copy link
Member

See: SEARCH-910

@kderusso kderusso added >test Issues or PRs that are addressing/adding tests :SearchOrg/Relevance Label for the Search (solution/org) Relevance team v9.1.0 labels Mar 18, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

private static final NodeFeature TEST_RERANKING_SERVICE_PARSE_TEXT_AS_SCORE = new NodeFeature(
"test_reranking_service.parse_text_as_score"
);
private static final NodeFeature TEST_RULE_RETRIEVER_WITH_EMPTY_INDEX = new NodeFeature("test_rule_retriever.with_empty_index");
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find the feature this was fixed in, so I added a new test feature.

@kderusso kderusso requested a review from a team March 19, 2025 14:07
Copy link
Contributor

@Samiul-TheSoccerFan Samiul-TheSoccerFan left a comment

Choose a reason for hiding this comment

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

LGTM

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.

One nit comment about names. Also, should we backport this to 8.19?

private static final NodeFeature TEST_RERANKING_SERVICE_PARSE_TEXT_AS_SCORE = new NodeFeature(
"test_reranking_service.parse_text_as_score"
);
private static final NodeFeature TEST_RULE_RETRIEVER_WITH_EMPTY_INDEX = new NodeFeature("test_rule_retriever.with_empty_index");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this isn't a great description of the issue as the index isn't empty. Is there a more accurate feature name we can use?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, technically it's empty in the result set, but I'll make a more verbose name 😜

Copy link
Contributor

Choose a reason for hiding this comment

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

"Empty result set" is more descriptive, thank you :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I didn't backport it because I wasn't sure of the fix. It's just an aadditional test though so I think it's OK.

@kderusso kderusso enabled auto-merge (squash) March 19, 2025 15:03
@kderusso kderusso merged commit 00c8ad8 into elastic:main Mar 19, 2025
17 checks passed
smalyshev pushed a commit to smalyshev/elasticsearch that referenced this pull request Mar 21, 2025
* Add rule retriever yaml test with empty indices

* Add more specificity to NodeFeature name
omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025
* Add rule retriever yaml test with empty indices

* Add more specificity to NodeFeature name
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 >test Issues or PRs that are addressing/adding tests v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants