-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: Add cancel and status queries to server-side async execution #192
Conversation
…r, . Added a slot and getter for query_id to BaseCursor. (Temporarily?) edited setup.cfg to ignore flake8 C901: function is too complex.
…and execute_many calls.
… logic for SET async_execution.
…tures. Edited callbacks and various tiny things in async test_cursor.py.
…y) on a unit test run.
…ync_execute() to remove async.
…uery. Changed AsyncExecutionUnavailableError to generically accept messages.
…rver_side_async_execute().
…han being sent in as an argument to execute(). Moved set parameter validation to its own function to deal with flake8 complaints re _do_exectute() being too complex.
… for more than one status before exiting the loop.
…ync/test_queries.py. That was done bc it won't run on my laptop, but it shouldn't have been committed that way.
…async_exectution test to not count SET statements as queries when determining whether a query is multi-statment. Trying to get sleepEachRow() to work for long aync execution queries.
…async_execution test to not count SET statements as queries when determining whether a query is multi-statment. Trying to get sleepEachRow() to work for long aync execution queries.
…firebolt-db/firebolt-python-sdk into async_queries_add_cancel_status
…This is because if it's off no log entries are written to query_history. Still using a long insert for integration tests on server-side async queries. Added and edited to unit tests for use_standard_sql correctness.
…de async tests to end.
…sync tests to end of modules to facilitate merging main.
…cs. Updated dictionary update in _api_request() to make mypy happy.
… explain usefullness of that functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, otherwise lgtm.
with connection.cursor() as c: | ||
try: | ||
await c.execute( | ||
"CREATE DIMENSION TABLE IF NOT EXISTS test_tbl (id int, name string)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be a global. In fact, since this as well as LONG_INSERT
are reused in both sync and async tests they can be placed at the dbapi/ conftest level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this and the DROP TABLE
below both globals, but I don't know how to move them out of their modules and up a level: I couldn't get the imports to work, and pytest won't import anything other than fixtures from the conftest files, as far as I can tell. I went ahead and merged without it, I figure we can address it in a later PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I had to reopen this. Can you tell me how to get the import to work? It seems to be beyond my Python abilities.
…s.py and into conftest.py. Currently name not defined error. Maybe it's in the wrong conftest file?
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how GH reviews work. do i need to submit my review for you to see my comments? testing testing...
========================================== | ||
|
||
Server-side asynchronous query execution allows you to run a long query in the background while executing other asynchronous or synchronous queries. An additional benefit of server-side async execution that can free up a connection or potentially even spin down an entire service (such as AWS Lambda) while a long-running database job is still underway. Note that it is not possible to retrieve the results of a server-side asynchronous query, so these queries are best used for running DMLs and DDLs. SELECTs should be used only for warming the cache. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding a brief paragraph here explaining the differences between server-side async vs. client-side async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericf-firebolt I see you merged the PR, but what about this comment above?
|
||
.. _basic_execute_example: | ||
|
||
Running queries server-side asynchronously is similar to running server-side asynchronous queries, but the ``execute()`` command receives an extra parameter, ``async_execution=True``. The example below uses ``cursor`` to create a new table called ``test_table``. ``execute(query, async_execution=True)`` will return a query ID, which can subsequently be used to check the query status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that it's similar to client-side async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericf-firebolt I see you merged the PR, but what about this comment above?
* ``EXECUTION_ERROR`` | ||
|
||
|
||
Once the status of the table creation is ``ENDED_SUCCESSFULLY`` created, data can be inserted into it: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete "created" in this sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericf-firebolt I see you merged the PR, but what about this comment above?
No description provided.