Skip to content

fix explain action on query rewrite#17286

Merged
dbwiddis merged 1 commit intoopensearch-project:mainfrom
fen-qin:explain-fix
Mar 4, 2025
Merged

fix explain action on query rewrite#17286
dbwiddis merged 1 commit intoopensearch-project:mainfrom
fen-qin:explain-fix

Conversation

@fen-qin
Copy link
Copy Markdown
Contributor

@fen-qin fen-qin commented Feb 6, 2025

Description

There is an exception when call explain in "by_doc_id" mode. Response looks like this:

{
    "error": {
        "root_cause": [
            {
                "type": "query_shard_exception",
                "reason": "failed to create query: async actions are left after rewrite",
                "index": "my-nlp-index-1",
                "index_uuid": "I6_pzGG3QBWw0B6KeOJ1pA"
            }
        ],
        "type": "query_shard_exception",
        "reason": "failed to create query: async actions are left after rewrite",
        "index": "my-nlp-index-1",
        "index_uuid": "I6_pzGG3QBWw0B6KeOJ1pA",
        "caused_by": {
            "type": "illegal_state_exception",
            "reason": "async actions are left after rewrite"
        }
    },
    "status": 400
}

The error were caught from TransportExplainAction -> Rewriteable.java because there still be asyncActions left when the flag assertNoAsyncTasks returned as true.

Screenshot 2025-01-29 at 8 29 43 PM

The conflict is down to:

  1. The NeuralQueryBuilder or TermsQueryBuilder doRewrite method introduces asynchronous actions via queryRewriteContext.registerAsyncAction. So it then returns a context with potentially unresolved async tasks.

  2. Rewritable.rewrite then checksassertNoAsyncTasks flag, throws the exception if there exists any async tasks left.

Fix in this PR is resolve this conflict:

  • by moving the query rewrite to coordinator - TransportExplainAction.
  • verify the changes from local using TermsQueryBuilder
    • Before fix
    • Screenshot 2025-01-30 at 9 30 29 PM
    • After fix
    • Screenshot 2025-01-30 at 9 24 10 PM

Related Issues

Resolves - opensearch-project/neural-search#1126

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 6, 2025

❌ Gradle check result for 3b159cd: 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?

Copy link
Copy Markdown
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

This looks like a re-submittal of #17277 which has unresolved comments. Please reference earlier PRs when re-submitting them so that reviewers have the full context of previous comments (and can make sure they are resolved).

Code looks like it addresses the problem, but it's overly complex chaining an action listener with a null that gets ignored. Suggest moving the null check up to the top of the method and calling super.doExecute() (basically the existing code, but wrapped in a null check with a shortcut return before creating more objects). Only if the query is non-null do you need to instantiate the actionListener and chain that code.

Also looks like some checks are failing like DCO and Change log.

@github-actions
Copy link
Copy Markdown
Contributor

❌ Gradle check result for 2dcc19b: 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?

@github-actions
Copy link
Copy Markdown
Contributor

❌ Gradle check result for a111f2f:

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?

@github-actions
Copy link
Copy Markdown
Contributor

❌ Gradle check result for 9e7e026: 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?

@github-actions
Copy link
Copy Markdown
Contributor

✅ Gradle check result for 0389c20: SUCCESS

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 72.46%. Comparing base (e397903) to head (a5778cb).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
...nsearch/action/explain/TransportExplainAction.java 0.00% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #17286      +/-   ##
============================================
- Coverage     72.47%   72.46%   -0.01%     
- Complexity    65693    65697       +4     
============================================
  Files          5304     5304              
  Lines        304981   304990       +9     
  Branches      44238    44239       +1     
============================================
- Hits         221033   221023      -10     
+ Misses        65808    65792      -16     
- Partials      18140    18175      +35     

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

fen-qin added a commit to fen-qin/OpenSearch that referenced this pull request Feb 25, 2025
Signed-off-by: Fen Qin <mfenqin@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

✅ Gradle check result for ceadf37: SUCCESS

Signed-off-by: Fen Qin <mfenqin@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

❕ Gradle check result for a5778cb: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link
Copy Markdown
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM with some style nits that I won't hold up approval on.

@dbwiddis dbwiddis merged commit 2e4cc8c into opensearch-project:main Mar 4, 2025
32 of 33 checks passed
mayanksharma27 pushed a commit to mayanksharma27/OpenSearch that referenced this pull request Mar 5, 2025
vinaykpud pushed a commit to vinaykpud/OpenSearch that referenced this pull request Mar 18, 2025
…earch-project#17286)

Signed-off-by: Fen Qin <mfenqin@amazon.com>
Signed-off-by: Vinay Krishna Pudyodu <vinkrish.neo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement or improvement to existing feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants