Async query get result bug fix #2443
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2443 +/- ##
=========================================
Coverage 95.39% 95.40%
- Complexity 4990 4992 +2
=========================================
Files 478 478
Lines 13910 13917 +7
Branches 932 933 +1
=========================================
+ Hits 13270 13277 +7
Misses 618 618
Partials 22 22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
dai-chen
changed the title
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
Signed-off-by: Chen Dai <daichen@amazon.com>
dai-chen
requested review from
pjfitzgibbons,
ps48,
kavithacm,
derek-ho,
joshuali925,
YANG-DB,
rupal-bq,
mengweieric,
vmmusings,
Swiddis,
penghuo,
seankao-az,
MaxKsyunz and
Yury-Fridlyand
as code owners
dai-chen
requested review from
anirudha,
forestmvey,
acarbonetto and
GumpacG
as code owners
|
The failed CI seems not relevant. It keeps failing on retry and failed in |
dai-chen
commented
Dec 1, 2023
spark/src/main/java/org/opensearch/sql/spark/dispatcher/AsyncQueryHandler.java
Show resolved
Hide resolved
kaituo
approved these changes
Dec 1, 2023
penghuo
reviewed
Dec 1, 2023
spark/src/test/java/org/opensearch/sql/spark/asyncquery/AsyncQueryGetResultSpecTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Chen Dai <daichen@amazon.com>
YANG-DB
reviewed
Dec 1, 2023
| // Consider statement still running if result doc created in submit() is not available yet | ||
| JSONObject result = new JSONObject(); | ||
| result.put(STATUS_FIELD, StatementState.RUNNING.getState()); | ||
| result.put(ERROR_FIELD, ""); |
There was a problem hiding this comment.
is the error field mandatory even if there is no error ?
There was a problem hiding this comment.
In getAsyncQueryResults() API code, it always fetch status and error field from what is returned here. Although it uses optString, I thought it maybe more clear to pass it here.
YANG-DB
reviewed
Dec 1, 2023
spark/src/test/java/org/opensearch/sql/spark/asyncquery/AsyncQueryExecutorServiceSpec.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Chen Dai <daichen@amazon.com>
penghuo
approved these changes
Dec 1, 2023
vmmusings
approved these changes
Dec 1, 2023
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Dec 1, 2023
* Add IT to reproduce the bug first Signed-off-by: Chen Dai <daichen@amazon.com> * Modify IT as temporary reproduce commit Signed-off-by: Chen Dai <daichen@amazon.com> * Fix issue by preferred option and modify IT Signed-off-by: Chen Dai <daichen@amazon.com> * Refactor IT with fluent assertion Signed-off-by: Chen Dai <daichen@amazon.com> * Add IT for batch query handler Signed-off-by: Chen Dai <daichen@amazon.com> * Add IT for streaming query handler Signed-off-by: Chen Dai <daichen@amazon.com> * Add more IT for normal case Signed-off-by: Chen Dai <daichen@amazon.com> * Add IT for drop index Signed-off-by: Chen Dai <daichen@amazon.com> * Consider drop statement running if result doc unavailable Signed-off-by: Chen Dai <daichen@amazon.com> * Fix broken UT Signed-off-by: Chen Dai <daichen@amazon.com> * Address PR comments Signed-off-by: Chen Dai <daichen@amazon.com> * Address PR comments Signed-off-by: Chen Dai <daichen@amazon.com> --------- Signed-off-by: Chen Dai <daichen@amazon.com> (cherry picked from commit 60058ce) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dai-chen
pushed a commit
that referenced
this pull request
Dec 1, 2023
* Add IT to reproduce the bug first * Modify IT as temporary reproduce commit * Fix issue by preferred option and modify IT * Refactor IT with fluent assertion * Add IT for batch query handler * Add IT for streaming query handler * Add more IT for normal case * Add IT for drop index * Consider drop statement running if result doc unavailable * Fix broken UT * Address PR comments * Address PR comments --------- (cherry picked from commit 60058ce) Signed-off-by: Chen Dai <daichen@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Followed option 2 in #2436 (comment). Consider query statement complete and successful only if statement state is SUCCESS and query result is available (searchable). Please see more details in the issue comment.
Changes:
AsyncQueryHandlerand subclass as neededIndexQuerySpecTestup to parent class so new IT in this PR can use it.Issues Resolved
#2436, #2439
Check List
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.