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 missing search_pipeline field in opensearchapi.SearchParams #532

Merged

Conversation

jackh-ncl
Copy link
Contributor

Description

Added search_pipeline field in SearchParams struct.

Note: I didn't add any tests as I can't see any others there for existing SearchParam fields.

Context:

With the addition of hybrid search and search pipelines in Opensearch version 2.11, a new search_pipeline parameter exists on the _search endpoint (docs here).

Issues Resolved

This pull request supports the new search pipelines functionality by allowing users of the library to set the search_pipeline parameter via the SearchParams struct.

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.

@jackh-ncl jackh-ncl force-pushed the add-search_pipeline-search-param branch from 9978925 to 9250b81 Compare April 18, 2024 16:24
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.93%. Comparing base (06a6dc8) to head (cf059ca).
Report is 15 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #532       +/-   ##
===========================================
+ Coverage   57.29%   67.93%   +10.64%     
===========================================
  Files         315      376       +61     
  Lines        9823     8852      -971     
===========================================
+ Hits         5628     6014      +386     
+ Misses       2902     1556     -1346     
+ Partials     1293     1282       -11     
Flag Coverage Δ
integration 60.36% <0.00%> (+9.52%) ⬆️
unit 15.26% <100.00%> (+2.42%) ⬆️

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

Files Coverage Δ
opensearchapi/api_search-params.go 89.42% <100.00%> (+82.08%) ⬆️

... and 270 files with indirect coverage changes

@jackh-ncl jackh-ncl force-pushed the add-search_pipeline-search-param branch from 9250b81 to fcd2e3f Compare April 18, 2024 16:34
@jackh-ncl jackh-ncl marked this pull request as ready for review April 18, 2024 16:39
@dblock
Copy link
Member

dblock commented Apr 18, 2024

We do need tests for existing and new code, can you please add something simple that exercises params, please?

@jackh-ncl jackh-ncl force-pushed the add-search_pipeline-search-param branch 4 times, most recently from 5eb3e46 to 1c254cb Compare April 19, 2024 08:23
@jackh-ncl
Copy link
Contributor Author

@dblock I have pushed up some simple tests, though it's proving a bit difficult to get it right as the get() method is unexported, and golanglint wants test files to be under a test package (e.g. opensearchapi_test), which means I'm unable to access it.

I can either add make get() exported, add a nolint or it looks like I could follow this sort of structure, though I'm not too familiar with this approach.

Would appreciate any suggestions 😃

dblock
dblock previously approved these changes Apr 19, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

LGTM, @Jakob3xD has a good answer to your question?

Signed-off-by: Jack Hindmarch <jack.hindmarch@adarga.ai>
@jackh-ncl jackh-ncl force-pushed the add-search_pipeline-search-param branch from 1c254cb to 4aa53c4 Compare April 19, 2024 13:31
dblock
dblock previously approved these changes Apr 19, 2024
Copy link
Collaborator

@Jakob3xD Jakob3xD left a comment

Choose a reason for hiding this comment

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

LGTM, only thing missing is the build tag for testing.
Afterwards it is good to merge.

Co-authored-by: Jakob <jakob.hahn@hetzner.com>
Signed-off-by: Jack Hindmarch <1750152+jackh-ncl@users.noreply.github.com>
@jackh-ncl
Copy link
Contributor Author

Thank you 😃

@Jakob3xD Jakob3xD merged commit 0f7eb6f into opensearch-project:main Apr 22, 2024
55 checks passed
@jackh-ncl jackh-ncl deleted the add-search_pipeline-search-param branch April 22, 2024 10:55
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.

3 participants