This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
Simple Query Cursor support #390
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
…ed previous cursor changes
penghuo
reviewed
Mar 19, 2020
src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/CursorIT.java
Outdated
Show resolved
Hide resolved
penghuo
reviewed
Apr 9, 2020
penghuo
reviewed
Apr 9, 2020
src/main/java/com/amazon/opendistroforelasticsearch/sql/query/DefaultQueryAction.java
Outdated
Show resolved
Hide resolved
penghuo
reviewed
Apr 9, 2020
| } | ||
| } | ||
|
|
||
| public String execute(Client client, Map<String, String> params) throws Exception { |
There was a problem hiding this comment.
Two concerns.
- The execute method is almost as same as CursorResultExecutor.execute, only difference is the LOG.error. Is it possible to avoid the duplication.
- In case we already define the Curser, could we construct CursorCloseExecutor by using cursor and encapsulate the Cursor construct operation in CursorFactory?
There was a problem hiding this comment.
Sure, will give a shot to refactor as described.
There was a problem hiding this comment.
Keeping it as it now, little tricky to refactor.
There was a problem hiding this comment.
In my opinion, the cursor format ":" which should be encapsulated to Cursor implementation and should not been exposed to caller.
We can leave it for now for time consideration, Please consider add UT to cover the logic in CursorCloseExecutor and CursorResultExecutor. If possible, add the issue to tracking the refactoring.
penghuo
reviewed
Apr 9, 2020
...ain/java/com/amazon/opendistroforelasticsearch/sql/executor/cursor/CursorResultExecutor.java
Show resolved
Hide resolved
penghuo
reviewed
Apr 9, 2020
src/main/java/com/amazon/opendistroforelasticsearch/sql/cursor/DefaultCursor.java
Outdated
Show resolved
Hide resolved
penghuo
reviewed
Apr 9, 2020
dai-chen
reviewed
Apr 9, 2020
src/main/java/com/amazon/opendistroforelasticsearch/sql/cursor/CursorType.java
Show resolved
Hide resolved
.../java/com/amazon/opendistroforelasticsearch/sql/executor/cursor/CursorAsyncRestExecutor.java
Show resolved
Hide resolved
penghuo
reviewed
Apr 13, 2020
There was a problem hiding this comment.
No major concern for the functionality.
I think we already have pretty good IT. But It seems we miss UT for new Class CursorXXXExecutor and changed class Protocol and SelectResultSet.
For the time consideration, we could postpone refactoring the Cursor code structure to the next stage, but adding enough UT is the precondition to make the refactor easily.
What do you think?
dai-chen
reviewed
Apr 13, 2020
src/main/java/com/amazon/opendistroforelasticsearch/sql/cursor/DefaultCursor.java
Show resolved
Hide resolved
...ain/java/com/amazon/opendistroforelasticsearch/sql/executor/cursor/CursorResultExecutor.java
Outdated
Show resolved
Hide resolved
...ain/java/com/amazon/opendistroforelasticsearch/sql/executor/cursor/CursorResultExecutor.java
Outdated
Show resolved
Hide resolved
...ain/java/com/amazon/opendistroforelasticsearch/sql/executor/cursor/CursorResultExecutor.java
Show resolved
Hide resolved
...java/com/amazon/opendistroforelasticsearch/sql/executor/format/PrettyFormatRestExecutor.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java
Show resolved
Hide resolved
Agreed. Adding UTs for CursorXXXExecutor where possible. |
dai-chen
reviewed
Apr 13, 2020
src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/SelectResultSet.java
Show resolved
Hide resolved
penghuo
approved these changes
Apr 14, 2020
dai-chen
approved these changes
Apr 14, 2020
penghuo
pushed a commit
to penghuo/sql
that referenced
this pull request
May 2, 2020
* Add integration tests to be passed * Add cluster settings for cursor - enabled, fetch_size, keep_alive * Add fetch_size and cursor params. fetch_size validation * new SqlRequest constructor for cursor * Add logic to open scroll based on settings, fetch_size and limit values * Add cursor close endpoint * Remove date formatting changes * Fix unit and integ tests, Ignored date format tests for a while, synced previous cursor changes * Add cursor generation * Add test helper methods * Cursor close API * Remove commented code and add partial date formatting change * Add error metrics when not able to close cursor * Add indexname and fieldAliasMap to cursor context * Remove ignored test cases affected by date formatting changes * Remove unneeded interface, refactor CursorType enum * Remove logs, unneeded fields, comments, refactor * Disable cursor by default * Fix cursor for parameterized request, add integration test for same * LIMIT changes * Changes to handle different LIMIT cases * Add default cursor metrics * Add integration test on explain cursor * Update monitoring, settings and endpoint docs * Refactor cursor classes to separate package * Add Lombok for DefaultCursor * Add unit test for DefaultCursor * Update doc * Unit tests, bug fix , refactoring (cherry picked from commit bf4810d)
penghuo
added a commit
that referenced
this pull request
May 5, 2020
* Update the opendistro sql 1.4.0 release notes (#359) (cherry picked from commit d9fe9dc) * adding DATETIME cast support (#310) Adding full support for CAST(), and adding it as a function. Fixed Datetime casting to be UTC-timezone default. (cherry picked from commit 68b971f) * Documentation for simple query (#366) * Add doc test for simple query * Add to index * Fix format * Add documentation * Add documentation * Add documentation * Add documentation * Add documentation * Add documentation * Add documentation * Add documentation * Add documentation * Add documentation * Add rdd * Add rdd * Add rdd * Add rdd * Fix order by example * Add more diagrams * Add more diagrams * Skip doctest in gradlew build (cherry picked from commit 57b379c) * Return Correct Type Information for Fields (#365) Return correct type information for fields in JDBC format and changed the return type of trig functions to return DOUBLE. (cherry picked from commit 9769c30) * Report date data as a standardized format (#367) * expose date fields using a standardized date format * move date format logic to new class * add tests for DateTimeFormatter * added some negative tests * style fixes * testing locale fixes * addressed code review comments and build failures * post-merge fix * additional fixes * get CAST alias info from result set class, rather than cluster state * remove unused import * remove unused import * reduce duplication & reference enum value * setting default timezone to UTC while parsing date values * add support for custom & multiple formats * add case for Kibana flights date data with T but no time field (cherry picked from commit 7c57d72) * Integration test with external ES cluster (#374) * Fix build issue by migrating to RestTestCase * Fix query IT * Fix subquery IT * Fix csv formatter and SQL functions IT * Fix aggregation and delete IT * Fix date IT * Fix jdbc IT * Fix show and metadata IT * Fix subquery IT * Fix date functions UT * Fix test data setup and cleanup issue * Fix correctness IT * Fix doctest IT * Add JavaDoc on base class * Address PR comments * Address PR comments (cherry picked from commit 56a2f44) * Bug fix, return object type for field which has implicit object datatype when describe the table (#377) * Bug fix, return object type for field which has implicit object datatype when describe the table (cherry picked from commit c30e342) * Pagination doc (#379) * Add design details in doc * Added images for pagination doc * Add image links and fix formatting * Add cursor setting details * Add Salient Points section in Detailed Design section (cherry picked from commit ac8a020) * Handle the elasticsearch exceptions in JDBC formatted outputs (#362) * Caught ES exception * Added details in errMsgs to enrich the behavior; added IT * Handled cases where ES exceptions are wrapped up; added default fetching details method * Added factory method to construct ErrorMessage; extended exception type for ErrorMessage * Added UT for ErrorMessageFactory * addressed comments (cherry picked from commit 71aba38) * Modified the wording of exception messages and created the troubleshooting page (#372) * Edit the wording of exception messages * Created troubleshooting page * Revised troubleshooting page * addressed comments (cherry picked from commit a4afc75) * Sql CI/CD (#384) * Create CD.yml * Create CI.yml (cherry picked from commit fa1d4a0) * FIX: field function name letter case preserved in select with group by (#381) * FIX: Method field name * REF: parser logic * RMV: remove unused function * STY: unused imports * STY: unused import (cherry picked from commit ad8ad3a) * Fix broken LICENSE link in README.md (#394) * Fix broken License link * Reverting NOTICE name change (cherry picked from commit 62e36c9) * New SQL cluster settings endpoint (#400) * Add new _sql/settings endpoint, and logic to only affect opendistro.sql settings * Add integration tests * Change endpoint HTTP method to PUT * Update Settings doc (cherry picked from commit f1d538f) * Bug Fix, add support for strict_date_optional_time (#412) (cherry picked from commit 9ed430f) * Invalidate HTTP GET method (#414) * Removed http GET method; added integTest * Removed GET method in doc and doctest (cherry picked from commit 56464f0) * More docs in reference manual and add architecture doc (#417) * Initial commit for PartiQL and complex query docs * Initial commit for PartiQL and complex query docs * Initial commit for PartiQL and complex query docs * Ignore multi-query for now because of bug * Add test index mappings * Fix partiql doc * Bypass LEFT JOIN for now * Add doc for JOINs * Add doc for JOINs * Add doc for JOINs * Add more cases for PartiQL * Add more cases for PartiQL * Add multi-line support * Add multi-line support * Multi-line all complex queries * Multi-line all complex queries * Add doc for full text search * Add doc for full text search * Add doc for metadata query * Add doc for multi match query * Add doc for delete statement * Remove join explain * Add RDD * Add RDD * Print test data for PartiQL * Print test data for PartiQL * Change titles * Add architecture doc * Add docs for SQL functions * Update index * Update docs * Update docs * Update docs * Address PR comments (cherry picked from commit 6457b0b) * Bug fix, support subquery in from doesn't have alias (#418) (cherry picked from commit 5e5f485) * Anonymize sensitive data in queries exposed to RestSqlAction logs (#419) * remove sensitive data from queries for logging * Added rule to mask sensitive data from es logs * Applied API in SQLUtils to rebuild query string from AST; replace data masks with anonymous words * Inlined log message; added doc for new rule (cherry picked from commit 45fa29c) * Bug fix, ignore the term query rewrite if there is no index found (#425) * Bug fix, ignore the term query rewrite if there is no index found * move IsEqualIgnoreCaseAndWhiteSpace to MatcherUtils (cherry picked from commit 2bfe326) * Simple Query Cursor support (#390) * Add integration tests to be passed * Add cluster settings for cursor - enabled, fetch_size, keep_alive * Add fetch_size and cursor params. fetch_size validation * new SqlRequest constructor for cursor * Add logic to open scroll based on settings, fetch_size and limit values * Add cursor close endpoint * Remove date formatting changes * Fix unit and integ tests, Ignored date format tests for a while, synced previous cursor changes * Add cursor generation * Add test helper methods * Cursor close API * Remove commented code and add partial date formatting change * Add error metrics when not able to close cursor * Add indexname and fieldAliasMap to cursor context * Remove ignored test cases affected by date formatting changes * Remove unneeded interface, refactor CursorType enum * Remove logs, unneeded fields, comments, refactor * Disable cursor by default * Fix cursor for parameterized request, add integration test for same * LIMIT changes * Changes to handle different LIMIT cases * Add default cursor metrics * Add integration test on explain cursor * Update monitoring, settings and endpoint docs * Refactor cursor classes to separate package * Add Lombok for DefaultCursor * Add unit test for DefaultCursor * Update doc * Unit tests, bug fix , refactoring (cherry picked from commit bf4810d) * [BugFix] Enforce AVG to return double data type (#437) * Enforce AVG to return double. Update UT, add IT. (cherry picked from commit 5870bbf) * Bug fix, count(distinct field) should transalte to cardinality aggregation (#442) (cherry picked from commit b3df480) * Fix CSV injection issue (#447) * Sanitize to avoid CSV injection * Add IT (cherry picked from commit 78d4158) * [BugFix] mock LocalClusterState settings in QueryPlanner base class (#446) (cherry picked from commit 6525dca) * Escape comma for CSV header and all queries (#456) (cherry picked from commit ced1cd5) * Bug Fix, support using aggregation function in order by clause (#452) * Bug Fix, support using aggregation function in order by clause * address comments (cherry picked from commit 30b76ce) * Remove CI and change release workflow based on tags (#457) (cherry picked from commit 9758598) Co-authored-by: David Cui <53581635+davidcui-amzn@users.noreply.github.com> Co-authored-by: Chen Dai <46505291+dai-chen@users.noreply.github.com> Co-authored-by: Jordan Wilson <37088125+jordanw-bq@users.noreply.github.com> Co-authored-by: Abbas Hussain <abbas_10690@yahoo.com> Co-authored-by: Chloe <chloezh1102@gmail.com> Co-authored-by: Rishabh Singh <rishabhksingh@gmail.com> Co-authored-by: Qi Chen <chenqi0805@gmail.com> Co-authored-by: Zhongnan Su <szhongna@amazon.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Issue #16
Description of changes: This is the 1st phase of Cursor support work. This PR implements the pagination support for simple SQL queries of the form
SELECT ... FROM ... WHERE ... ORDER BYReviewer: To make CR easier, please have a look at the following sections of the design doc.
Protocol, JDBC, Salient points, Settings.
Also the integration tests in
CursorIT.javaBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.