refactor: FIR-43722 refactor our result set logic from cursors #423
Conversation
stepansergeevitch
changed the title
There was a problem hiding this comment.
I tried looking to see if I understand and can review the code but I don't know python so it is hard to review a change this big.
One note though: do you want to change the quality gate for python sdk to fail if there are new issues (like new violations or test coverage is not enough). I did change it for JDBC and now the email tells you that he quality gate failed. for python it passes even though there are new issues.
| content=query, | ||
| timeout=timeout if timeout is not None else USE_CLIENT_DEFAULT, | ||
| ) | ||
| return await self._client.send(req, stream=True) |
There was a problem hiding this comment.
was this the default, previous behavior with stream? I thought this is the refactoring and then you will add the streaming functionality.
There was a problem hiding this comment.
We used to read the response here fully and then work with the result in memory. Now we stream the response here, and let the RowSet to decide whether to read the stream in memory and process it or (in future streaming implementation) to read it line by line and not store it in memory
stepansergeevitch
force-pushed
the
FIR-43722-add-streaming-to-python-sdk
branch
from
83b4bf9 to
f2014db
Compare
| @check_not_closed | ||
| def __enter__(self) -> Cursor: | ||
| return self |
There was a problem hiding this comment.
I think we can still support enter, at least to make sure we don't break anything. It's "free" from the looks of it.
There was a problem hiding this comment.
We cannot have __enter__ and __exit__ now since close becomes async aclose, and we're not able to call it from a sync function anymore
There was a problem hiding this comment.
But behind the scenes in async aclose we call super.close() so nothing stops us from having sync close pm cursor that would do the same. I believe one of the reasons why aenter and aexit were introduced instead of having async enter and exit is to allow an object that's both sync and async context manager.
There was a problem hiding this comment.
After discussion, decided to leave a no-op function with a deprecation warning
Co-authored-by: Petro Tiurin <93913847+ptiurin@users.noreply.github.com>
|
3 participants
Refactored out result storing and iteration into a separate RowSet class family. This is a prerequisite to implement support for streaming functionality