add an option to deferred fetch result in Cursor.execute() #400
Conversation
dungdm93
changed the title
fetch result in Cursor.execute()
dungdm93
changed the title
fetch result in Cursor.execute()
|
This probably needs unit test coverage as well. |
dungdm93
force-pushed
the
deferred_fetch
branch
from
63535d0 to
4e32a11
Compare
|
Test added |
dungdm93
force-pushed
the
deferred_fetch
branch
from
4e32a11 to
be850fc
Compare
| Validates that the `TrinoQuery.execute` function deferred_fetch and non-block execution | ||
| """ | ||
|
|
||
| class MockResponse(mock.Mock): |
There was a problem hiding this comment.
Looks like this is copied and pasted from another function, so it would be nice to extract it out and stay DRY.
There was a problem hiding this comment.
It's depends on sample_get_response_data local variable.
Still have a way to extract it out, but I wonder is it generic enough for other tests.
There was a problem hiding this comment.
My suggestion is keep it as is. if there are more tests use it, we could extract it later, in other PR.
There was a problem hiding this comment.
That's something which could just be passed into the constructor though, right? There's no reason for it to refer to local scope. It's a whole class which has been copied and pasted and defined in a local function, I think now is the right time to refactor it and stay DRY personally.
dungdm93
force-pushed
the
deferred_fetch
branch
from
be850fc to
4d4a5e8
Compare
| for row in self._rows: | ||
| self._rownumber += 1 | ||
| logger.debug("row %s", row) | ||
| yield row | ||
|
|
||
| self._rows = next_rows | ||
| self._rows = self._query.fetch() if not self._query.finished else [] |
There was a problem hiding this comment.
Through this change the above comment is not valid anymore as the next_uri is only acknowledged after exposing all rows from self._rows through dbapi. The result is the same but now the query will be hanging until the dbapi method fetch_one or fetch_many has processed all rows in the buffer.
Also in the future the fetch could be made async, done in a separate thread, which allows for continuously looping through rows and fetching the next resultset.
dungdm93
force-pushed
the
deferred_fetch
branch
2 times, most recently
from
3af2b28 to
9e2ae77
Compare
dungdm93
force-pushed
the
deferred_fetch
branch
from
9e2ae77 to
59542e6
Compare
dungdm93
force-pushed
the
deferred_fetch
branch
from
59542e6 to
2e31394
Compare
|
|
||
| @pytest.fixture(scope="session") | ||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
Change fixture scope to function to fix issue where it's modified between test cases.
| @@ -14,8 +14,10 @@ | |||
|
|
|||
| import pytest | |||
|
|
|||
| from tests.unit.oauth_test_utils import SERVER_ADDRESS | |||
There was a problem hiding this comment.
Shouldn't we put SERVER_ADDRESS in a more central place if we want to use it in multiple places. here the usage is not related to oauth.
|
@dungdm93 : Can you handle the open comments? |
After introduce in #220, all query are now until at least one row is received or query is finished or cancelled.
In some cases such as Superset, users would like to be able to cancel the query but the execution still going on.
See also: apache/superset#24913
Fix: apache/superset#24858