Skip to content
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

Add Highlight field to SearchHit struct #654

Merged

Conversation

tim-holdaway-clever
Copy link
Contributor

Description

For queries specifying highlights, the the SearchHits on the SearchResp now include the highlight information returned in the api response.

Issues Resolved

Closes #653

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.

@tim-holdaway-clever
Copy link
Contributor Author

Failing tests

        --- FAIL: TestNodes/Stats/without_request (0.09s)
    --- FAIL: TestSearchTemplate/with_request (0.01s)

appear to also be failing in other recent PRs, for instance in https://github.com/opensearch-project/opensearch-go/actions/runs/12757766943/job/35558719849?pr=652, so seems like are probably not caused by this PR.

Failures look like

        	Error:      	Should be empty, but was [{"op":"remove","path":"/nodes/W0IruYOMTB2U8Ho0B6ZF4w/remote_store"}]

Which seems like maybe the version of OpenSearch cluster that the integration tests are running against may have changed and is now returning some extra unexpected property in the response, or something like that.

Signed-off-by: Tim Holdaway <[email protected]>
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.79%. Comparing base (06a6dc8) to head (035de67).
Report is 77 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #654      +/-   ##
==========================================
- Coverage   57.29%   54.79%   -2.50%     
==========================================
  Files         315      374      +59     
  Lines        9823    11035    +1212     
==========================================
+ Hits         5628     6047     +419     
- Misses       2902     3680     +778     
- Partials     1293     1308      +15     
Flag Coverage Δ
integration 54.79% <ø> (+3.95%) ⬆️
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
opensearchapi/api_nodes-stats.go 100.00% <ø> (ø)
opensearchapi/api_search-template.go 100.00% <ø> (ø)
opensearchapi/api_search.go 100.00% <ø> (ø)

... and 38 files with indirect coverage changes

@dblock
Copy link
Member

dblock commented Jan 17, 2025

Which seems like maybe the version of OpenSearch cluster that the integration tests are running against may have changed and is now returning some extra unexpected property in the response, or something like that.

Appreciate it if you could help fix those first, if you have time.

@tim-holdaway-clever
Copy link
Contributor Author

Appreciate it if you could help fix those first, if you have time.

I can take a look and see if I can reproduce it locally. Integration tests were all passing in my local run using make cluster.build cluster.start && make test-integ race=true, so seems like something in the CI build potentially using against a different version of the cluster or configuration or something, especially since they are failing on other recent PRs.

Do you have any hunches or pointers on where I should look, as I'm not really familiar yet with all the ins-and-outs of this projects testing setup and CI config?

@dblock
Copy link
Member

dblock commented Jan 17, 2025

Do you have any hunches or pointers on where I should look, as I'm not really familiar yet with all the ins-and-outs of this projects testing setup and CI config?

I do not. I am mostly just cheerleading in this repo vs. doing real work which is primarily @Jakob3xD and some other contributors more recently :)

This field is returned since OpenSearch 2.18

Signed-off-by: Tim Holdaway <[email protected]>
This field is returned since OpenSearch 2.18.
(see opensearch-project/OpenSearch#15611)

Signed-off-by: Tim Holdaway <[email protected]>
@@ -62,6 +62,7 @@ type SearchTemplateResp struct {
Took int `json:"took"`
Timeout bool `json:"timed_out"`
Shards ResponseShards `json:"_shards"`
Status int `json:"status"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒️ Integration tests were failing due to this field that was added to the opensearch 2.18 response. The test was comparing the parsed response to the raw response and failing on the missing field.

@@ -112,6 +112,7 @@ type NodesStats struct {
Repositories []json.RawMessage `json:"repositories"`
AdmissionControl NodesStatsAdmissionControl `json:"admission_control"`
Caches NodesStatsCaches `json:"caches"`
RemoteStore NodeStatsRemoteStore `json:"remote_store"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒️ Integration tests were failing due to this field that was added to the opensearch 2.18 response. The test was comparing the parsed response to the raw response and failing on the missing field.
See opensearch-project/OpenSearch#15611 for where this was added to 2.18

@@ -207,7 +207,7 @@ godoc: ## Display documentation for the package
godoc --http=localhost:6060 --play

cluster.build:
docker compose --project-directory .ci/opensearch build;
docker compose --project-directory .ci/opensearch build --pull;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒️ This ensures that when running integration tests locally, docker isn't using an older cached version of opensearch:latest, and actually gets the most recent one that the CI build will be using.

Otherwise we can end up with a cached version of opensearch:latest that is not the current latest, and would use that instead,
resulting in tests running against a different version than in the CI build.

Signed-off-by: Tim Holdaway <[email protected]>
@dblock dblock merged commit 040f709 into opensearch-project:main Jan 18, 2025
61 checks passed
@dblock
Copy link
Member

dblock commented Jan 18, 2025

Thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Highlight results are not returned on search results.
2 participants