Query service client support #455
Conversation
|
🌋 Here are results of SLO test for python-sync: |
vgvoleg
changed the title
| for _ in res: | ||
| pass | ||
|
|
||
| with pytest.raises(Exception): |
|
|
||
| def test_tx_rollback_raises_before_begin(self, tx: BaseTxContext): | ||
| with pytest.raises(RuntimeError): | ||
| tx.rollback() |
ydb/_grpc/grpcwrapper/ydb_query.py
Outdated
| @dataclass | ||
| class QueryContent(IFromPublic, IToProto): | ||
| text: str | ||
| syntax: Optional[int] = None |
There was a problem hiding this comment.
Here and in other places.
I think defaults in internal wrappers is bad idea.
With default field you can forget to initialize it while create the object.
ydb/query/transaction.py
Outdated
| @property | ||
| def tx_id(self): | ||
| """ | ||
| Returns a id of open transaction or None otherwise |
There was a problem hiding this comment.
use "an" instead of "a" before vowel "i" in "id"
ydb/query/base.py
Outdated
| pass | ||
|
|
||
| @abc.abstractmethod | ||
| def begin(settings: QueryClientSettings = None): |
There was a problem hiding this comment.
here and below:
- first parameter should be self
- if default value is None - type hint must respect it and be Optional
ydb/query/transaction.py
Outdated
| """ | ||
| return self._tx_state.tx_id | ||
|
|
||
| def begin(self, settings=None): |
There was a problem hiding this comment.
need type hint for result for all functions, which return something
ydb/query/transaction.py
Outdated
| concurrent_result_sets: bool = False, | ||
| ): | ||
| self._ensure_prev_stream_finished() | ||
| self._tx_state._check_tx_not_terminal() |
There was a problem hiding this comment.
what if rename negative "not terminal" to positive "alive" for simplify read?
ydb/query/session.py
Outdated
| def execute( | ||
| self, | ||
| query: str, | ||
| tx_mode: base.BaseQueryTxMode = None, |
There was a problem hiding this comment.
Optional for typehint (here and in other similar places)
ydb/query/base.py
Outdated
| exec_mode: QueryExecMode = None, | ||
| parameters: dict = None, | ||
| concurrent_result_sets: bool = False, | ||
| empty_tx_control: bool = False, |
There was a problem hiding this comment.
Why we need separate flag empty_tx_control? What is use tx_mode = None for it?
And use SerializeReadWrite (or add some constant) as default value for methods instead of None?
ydb/query/pool.py
Outdated
| self._pool = pool | ||
| self._session = QuerySessionSync(pool._driver) | ||
|
|
||
| def __enter__(self): |
| "QuerySnapshotReadOnly", | ||
| "QueryStaleReadOnly", | ||
| "QuerySessionPool", | ||
| "QueryClientSync", |
There was a problem hiding this comment.
What about QuerySessionSync? it need as minimum for type hints
ydb/query/session.py
Outdated
| parameters: dict = None, | ||
| concurrent_result_sets: bool = False, | ||
| empty_tx_control: bool = False, | ||
| ): |
There was a problem hiding this comment.
Need typehint, now IDE can't understand what objects will be returned by iterator
|
|
||
| def callee(session): | ||
| print("=" * 50) | ||
| with session.execute("delete from example"): |
There was a problem hiding this comment.
need common style, with other example queries.
now delete, but CREATE. I prefer capital case for SQL commands (as CREATE)
| session.create() | ||
| res = [] | ||
| with session.execute("select 1; select 2") as results: | ||
| for result_set in results: |
There was a problem hiding this comment.
suggest - the test can check about the execute returns two result instead on two lines in one result.
ydb/query/pool.py
Outdated
|
|
||
| return retry_operation_sync(wrapped_callee, retry_settings) | ||
|
|
||
| def execute_with_retries(self, query: str, retry_settings: Optional[RetrySettings] = None, *args, **kwargs): |
There was a problem hiding this comment.
It needs typehint - for comfort work with result of the query
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information