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

Support of new k-NN query parameter, expand_nested #1013

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

bzhangam
Copy link
Contributor

Description

Support new knn query parameter expand_nested_docs.

Related Issues

Resolves #1008

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

Copy link
Contributor

@yizheliu-amazon yizheliu-amazon left a comment

Choose a reason for hiding this comment

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

LGTM overall

CHANGELOG.md Outdated Show resolved Hide resolved
@heemin32 heemin32 added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Dec 16, 2024
@bzhangam bzhangam changed the title [FEATURE]Support of new k-NN query parameter, expand_nested_docs #1008 [FEATURE]Support of new k-NN query parameter, expand_nested_docs Dec 16, 2024
@heemin32
Copy link
Collaborator

Please resolve the changelog conflict

@heemin32
Copy link
Collaborator

heemin32 commented Dec 16, 2024

  1. Remove [FEATURE] from PR title.
  2. Keep the CHANGELOG entry to be same as PR title.

@bzhangam bzhangam closed this Dec 16, 2024
@bzhangam bzhangam changed the title [FEATURE]Support of new k-NN query parameter, expand_nested_docs Support of new k-NN query parameter, expand_nested Dec 16, 2024
@bzhangam bzhangam reopened this Dec 16, 2024
@martin-gaievski
Copy link
Member

Overall code looks good to me.
One additional point - I think we have reached the point of NeuralQueryBuild having that always growing constructor of gazillion fields. Can you please add a builder class similar to one in KNNQueryBuilder, or log a new issue for this repo if the effort is beyond your bandwidth?

@bzhangam
Copy link
Contributor Author

Overall code looks good to me. One additional point - I think we have reached the point of NeuralQueryBuild having that always growing constructor of gazillion fields. Can you please add a builder class similar to one in KNNQueryBuilder, or log a new issue for this repo if the effort is beyond your bandwidth?

I'd like to create a new issue for it. - #1025

@bzhangam bzhangam closed this Dec 17, 2024
@bzhangam bzhangam reopened this Dec 17, 2024
@heemin32 heemin32 self-requested a review December 17, 2024 19:37
@bzhangam bzhangam reopened this Dec 17, 2024
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.72%. Comparing base (3d72cc3) to head (fa149d4).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1013      +/-   ##
============================================
+ Coverage     78.55%   78.72%   +0.16%     
- Complexity     1028     1033       +5     
============================================
  Files            85       85              
  Lines          3623     3633      +10     
  Branches        605      609       +4     
============================================
+ Hits           2846     2860      +14     
+ Misses          528      525       -3     
+ Partials        249      248       -1     

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

Copy link
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 fba4961 into opensearch-project:main Dec 18, 2024
79 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1013-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fba4961e9ec5fe28dabb3b004a1ea82b30d45773
# Push it to GitHub
git push --set-upstream origin backport/backport-1013-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1013-to-2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Label will add auto workflow to backport PR to 2.x branch enhancement v2.19.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]Support of new k-NN query parameter, expand_nested_docs
4 participants