[ PECO-2065 ] Create the async execution flow for the PySQL Connector #463
Conversation
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
… invalid operation handle still persists
jprakash-db
had a problem deploying
to
azure-prod
GitHub Actions
Failure
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
jprakash-db
had a problem deploying
to
azure-prod
GitHub Actions
Failure
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
jprakash-db
had a problem deploying
to
azure-prod
GitHub Actions
Failure
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
jprakash-db
requested review from
rcypher-databricks,
yunbodeng-db,
andrefurlan-db,
jackyhu-db and
benc-db
as code owners
jprakash-db
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
src/databricks/sql/client.py
Outdated
| @@ -1119,7 +1161,7 @@ def __init__( | |||
| self._arrow_schema_bytes = execute_response.arrow_schema_bytes | |||
| self._next_row_index = 0 | |||
|
|
|||
| if execute_response.arrow_queue: | |||
| if execute_response.arrow_queue or async_op: | |||
There was a problem hiding this comment.
why async_op depends on arrow? What if pyarrow is not installed
There was a problem hiding this comment.
It does not depend on arrow, currently in our codebase all data is named as arrow_queue be it arrow queue or column queue. So in this statement I am checking if data is already present or if it is an async operation don't do anything.
There was a problem hiding this comment.
My point is ResultSet should not couple with async_op as I do not see it has something related to async. If you want to force to use arrow_queue, please use force_arrow_queue or similar parameter instead of async_op in the constructor.
| @@ -796,13 +797,15 @@ def execute( | |||
| cursor=self, | |||
| use_cloud_fetch=self.connection.use_cloud_fetch, | |||
| parameters=prepared_params, | |||
| async_op=async_op, | |||
| ) | |||
| self.active_result_set = ResultSet( | |||
There was a problem hiding this comment.
result set is not ready yet when async_op is True, why do you set this? It should be set in theget_execution_result
There was a problem hiding this comment.
The result set that is returned over here is empty and does not have any data.
There was a problem hiding this comment.
I know, but this will make the code confusing and I do not think it is is necessary.
There was a problem hiding this comment.
I did this to keep the same logical flow for both execute_async and execute. Like in execute the active_result_set has data and in execute_async since there is no data so it is none. Once data is available the active_result_set will again have data, so logically I felt it made sense
There was a problem hiding this comment.
the return result comes from the returned value from execute_command in the sync flow, which means it is not ready until the sync completes, this is why I said it is confusing as it should be set in the completion of the async operation, this is standard practice/ways for most of the async code (on_complete = (result) => { setResult(result) )
There was a problem hiding this comment.
I got it what Jacky meant here. This is about confusing the users regarding the interface. We can keep the logic internally same, but don't need to keep the interface same for async and sync. For async what matters is the operationHandle. We can have different interface for both, but internally can reuse the code if possible.
jprakash-db
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
|
|
||
| resp = self.make_request(self._client.FetchResults, req) | ||
|
|
||
| t_result_set_metadata_resp = resp.resultSetMetadata |
There was a problem hiding this comment.
we don't need to check the state of response?
|
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Description
Implementing the flow for the asynchronous execution of the execute command
Functions added
execute_async
This is the main command that will be used to execute the query, otherwise the syntax is identical to the existing execute function
get_query_state
This function is used for the purpose of polling the status of the query, to know what is the status of execution
get_execution_result
This function is used to fetch the results that have been completed and then populate the ResultSet. The flow of handling the ResultSet onwards is the same
Testing Details
Added Integration tests