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

[BUG FIX] Fix the visit of inner query for NestedQueryBuilder #14739

Merged
merged 5 commits into from
Jul 24, 2024

Conversation

zhichao-aws
Copy link
Member

@zhichao-aws zhichao-aws commented Jul 12, 2024

Description

In #10110 we add visitor design pattern in QueryBuilder. For this pattern, compound query need to manage the visit for the sub-query. It gets the child visitor and use it to visit the sub-query in the visit method.

However, we didn't implement the visit method for NestedQueryBuilder. And as a result some query processors can not work for the NestedQueryBuilder. ref: opensearch-project/neural-search#813

In this PR we implement the visit method in NestedQueryBuilder. The coding style is consistent with previous PR #10110.

Related Issues

#10110
opensearch-project/neural-search#813

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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

❌ Gradle check result for 1985a21: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: zhichao-aws <[email protected]>
@zhichao-aws
Copy link
Member Author

Please help add a backport label for this PR

@zhichao-aws
Copy link
Member Author

Hi @vibrantvarun @msfroh, could you please take a look at this PR? As you're the author and reviewer of #10110

@zhichao-aws zhichao-aws changed the title [BUG FIX] implement visit method in NestedQueryBuilder [BUG FIX] Fix the visit of inner query for NestedQueryBuilder Jul 15, 2024
Copy link
Contributor

✅ Gradle check result for eceb497: SUCCESS

Copy link

codecov bot commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.87%. Comparing base (1fe58b5) to head (eaf3263).

Files Patch % Lines
...org/opensearch/index/query/NestedQueryBuilder.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #14739      +/-   ##
============================================
+ Coverage     71.78%   71.87%   +0.08%     
+ Complexity    62694    62692       -2     
============================================
  Files          5160     5160              
  Lines        294211   294215       +4     
  Branches      42553    42554       +1     
============================================
+ Hits         211212   211457     +245     
+ Misses        65599    65338     -261     
- Partials      17400    17420      +20     

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

@vibrantvarun
Copy link
Member

Add the changelog

@kotwanikunal
Copy link
Member

@zhichao-aws Can you please rebase? It looks like you have conflicts.

Signed-off-by: zhichao-aws <[email protected]>
@zhichao-aws
Copy link
Member Author

@zhichao-aws Can you please rebase? It looks like you have conflicts.

Sure

Copy link
Contributor

✅ Gradle check result for feceae8: SUCCESS

Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
@dblock dblock added the v2.16.0 Issues and PRs related to version 2.16.0 label Jul 24, 2024
Copy link
Contributor

✅ Gradle check result for eaf3263: SUCCESS

@vibrantvarun
Copy link
Member

@zhichao-aws what is the status of this PR?

@dblock
Copy link
Member

dblock commented Jul 24, 2024

I had marked this for 2.16 as it's a pretty straightforward bug fix that we should have merged a while ago. If it doesn't make it to 2.16 it's ok.

@dblock dblock merged commit fcc231d into opensearch-project:main Jul 24, 2024
38 of 39 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-14739-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fcc231dfc349e092c3f68e49f49e32a062313f71
# Push it to GitHub
git push --set-upstream origin backport/backport-14739-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

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

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.16 failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.16 2.16
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.16
# Create a new branch
git switch --create backport/backport-14739-to-2.16
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fcc231dfc349e092c3f68e49f49e32a062313f71
# Push it to GitHub
git push --set-upstream origin backport/backport-14739-to-2.16
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.16

Then, create a pull request where the base branch is 2.16 and the compare/head branch is backport/backport-14739-to-2.16.

@dblock
Copy link
Member

dblock commented Jul 24, 2024

@zhichao-aws can you please backport this manually to 2.x? Thanks.

@zhichao-aws
Copy link
Member Author

Hi @dblock , I created PR to backport 2.x #14967 and backport 2.16 #14968 manually. If the backport 2.16 one is unneccessary feel free to close it. Thanks!

harshavamsi pushed a commit to harshavamsi/OpenSearch that referenced this pull request Aug 20, 2024
…arch-project#14739)

* fix nested query visit subquery

Signed-off-by: zhichao-aws <[email protected]>

* add change log

Signed-off-by: zhichao-aws <[email protected]>

---------

Signed-off-by: zhichao-aws <[email protected]>
Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
wdongyu pushed a commit to wdongyu/OpenSearch that referenced this pull request Aug 22, 2024
…arch-project#14739)

* fix nested query visit subquery

Signed-off-by: zhichao-aws <[email protected]>

* add change log

Signed-off-by: zhichao-aws <[email protected]>

---------

Signed-off-by: zhichao-aws <[email protected]>
Signed-off-by: Daniel (dB.) Doubrovkine <[email protected]>
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport 2.16 backport-failed v2.16.0 Issues and PRs related to version 2.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants