Skip to content

[Enhancement] Add description in Search Configuration#293

Merged
martin-gaievski merged 3 commits intoopensearch-project:mainfrom
vibrantvarun:adddescription
Nov 5, 2025
Merged

[Enhancement] Add description in Search Configuration#293
martin-gaievski merged 3 commits intoopensearch-project:mainfrom
vibrantvarun:adddescription

Conversation

@vibrantvarun
Copy link
Copy Markdown
Member

@vibrantvarun vibrantvarun commented Nov 4, 2025

Description

Add description support in Search Configuration similar to QuerySets and JudgeMents.

Issues Resolved

281

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@vibrantvarun
Add description in Search Configuration
f7b9eef 
Signed-off-by: vibrantvarun <jainvarun4996@gmail.com>
@vibrantvarun
Add Changelog
c5e80de 
Signed-off-by: vibrantvarun <jainvarun4996@gmail.com>
@vibrantvarun vibrantvarun changed the title Add description in Search Configuration [Enhancement] Add description in Search Configuration Nov 4, 2025
epugh
epugh approved these changes Nov 4, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@epugh epugh left a comment

Choose a reason for hiding this comment

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

LGTM. We need to also modify the front end to support this ;-).

Comment thread src/main/java/org/opensearch/searchrelevance/executors/ExperimentRunningManager.java
(String) source.get("query"),
(String) source.get("searchPipeline")
(String) source.get("searchPipeline"),
(String) source.get("description")
Copy link
Copy Markdown
Collaborator

@epugh epugh Nov 4, 2025

Choose a reason for hiding this comment

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

we have a weird mix of using constants and strings through this code base. Not asking you to change this here, since it follows the pattern, more noting it as a potential refactoring opportunity in the future!

Comment thread src/main/java/org/opensearch/searchrelevance/rest/RestPutSearchConfigurationAction.java
);
}

String description = (String) source.get(DESCRIPTION);
Copy link
Copy Markdown
Collaborator

@epugh epugh Nov 4, 2025

Choose a reason for hiding this comment

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

Here is an example where we swapped back to a constant ;-(.

@epugh epugh added the v3.4.0 label Nov 4, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 4, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (bf6e9b4) to head (be722d2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files 
@@     Coverage Diff     @@
##   main   #293   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

martin-gaievski
martin-gaievski reviewed Nov 4, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

Overall code looks good. Few comments:

  • please add description to at least one of existing integ tests, corresponding requests are in this resource folder resources/searchconfig/CreateSearchConfiguration.json. Then add the assert logic for the new description. One of the straightforward tests for this change is this test class: java/org/opensearch/searchrelevance/action/searchConfiguration/SearchConfigurationIT.java
  • are you going to add UI part as well?

@vibrantvarun
Add assertion in integ test
be722d2 
Signed-off-by: vibrantvarun <jainvarun4996@gmail.com>
@vibrantvarun
Copy link
Copy Markdown
Member Author

vibrantvarun commented Nov 4, 2025

@martin-gaievski added integ test now.

martin-gaievski
martin-gaievski approved these changes Nov 5, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

Looks good, thank you

@martin-gaievski martin-gaievski merged commit ba08bdb into opensearch-project:main Nov 5, 2025
16 checks passed
@epugh epugh linked an issue Nov 5, 2025 that may be closed by this pull request
[FEATURE] Provide a Description field with Search Configurations #281
Open
@epugh
Copy link
Copy Markdown
Collaborator

epugh commented Nov 10, 2025

Did we miss adding the attribute description to https://github.com/opensearch-project/search-relevance/blob/main/src/main/resources/mappings/search_configuration.json ? I'm following up on this comment #305 (comment) and realized that I didn't think I'd seen anything about migrating an existing SRW deploy to a new one. I suppose it just uses the default field mapping? Is that okay? Seems odd to not have it listed in the file...

@epugh epugh mentioned this pull request Nov 10, 2025
Closed
@heemin32
Copy link
Copy Markdown
Collaborator

heemin32 commented Nov 10, 2025 •
edited
Loading

Did we miss adding the attribute description to https://github.com/opensearch-project/search-relevance/blob/main/src/main/resources/mappings/search_configuration.json ? I'm following up on this comment #305 (comment) and realized that I didn't think I'd seen anything about migrating an existing SRW deploy to a new one. I suppose it just uses the default field mapping? Is that okay? Seems odd to not have it listed in the file...

Good catch. We need to add the field there. Also, as far as I know, the current logic only creates the index if it doesn’t exist. However, we also need to update the index when its mapping is outdated.

@vibrantvarun could you follow up here?

heemin32 added a commit to heemin32/search-relevance that referenced this pull request Nov 10, 2025
@heemin32
Revert "[Enhancement] Add description in Search Configuration (opense…
556d205 
…arch-project#293)"

This reverts commit ba08bdb.

Signed-off-by: Heemin Kim <heemin@amazon.com>
@heemin32
Copy link
Copy Markdown
Collaborator

heemin32 commented Nov 10, 2025

Reverted the commit to avoid the release of the PR without fix. #309

heemin32 added a commit that referenced this pull request Nov 10, 2025
@heemin32
Revert "[Enhancement] Add description in Search Configuration (#293)" (…
885ed49 
…#309)

This reverts commit ba08bdb.

Signed-off-by: Heemin Kim <heemin@amazon.com>
@chloewqg
Copy link
Copy Markdown
Contributor

chloewqg commented Nov 18, 2025 •
edited
Loading

@vibrantvarun Hi Varun, a quick check on this, did we have any follow up PR to address index mapping update issue? Another PR #264 is currently blocked to merge because we add two new fields in judgment_cache.json

@vibrantvarun
Copy link
Copy Markdown
Member Author

vibrantvarun commented Nov 18, 2025

Hi @chloewqg No we don't have any follow up PR as of now. First this issue #310 needs to be fixed. I have some idea. I will drop my comment there soon.

@vibrantvarun vibrantvarun deleted the adddescription branch January 22, 2026 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@epugh epugh epugh approved these changes

@martin-gaievski martin-gaievski martin-gaievski approved these changes

@fen-qin fen-qin Awaiting requested review from fen-qin fen-qin is a code owner

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@wrigleyDan wrigleyDan Awaiting requested review from wrigleyDan wrigleyDan is a code owner

Assignees

No one assigned

Labels

v3.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] Provide a Description field with Search Configurations

5 participants

@vibrantvarun @epugh @heemin32 @chloewqg @martin-gaievski