Add table settings support #22
Conversation
| @@ -343,7 +343,35 @@ def get_bind_types( | |||
|
|
|||
|
|
|||
| class YqlDDLCompiler(DDLCompiler): | |||
| pass | |||
| def post_create_table(self, table): | |||
There was a problem hiding this comment.
nitpick (blocking): Could you provide a typehint here?
| pass | ||
| def post_create_table(self, table): | ||
| ydb_opts = table.dialect_options["ydb"] | ||
| with_content = [] |
There was a problem hiding this comment.
nitpick (non-blocking) Usually in the compilers such variables are named "_clause". Up to you
| @@ -343,7 +343,35 @@ def get_bind_types( | |||
|
|
|||
|
|
|||
| class YqlDDLCompiler(DDLCompiler): | |||
| pass | |||
| def post_create_table(self, table): | |||
There was a problem hiding this comment.
suggestion (blocking) There are another table settings, for example, TTL, which may be rendered in the WITH clause. I suggest to move this code to the def render_table_partitioning_settings(self, table) -> List[str] and add call of the method to the post_create_table.
|
Looks good for me, let's remove "WIP" prefix |
| ) | ||
| table.create(connection) | ||
|
|
||
| session: ydb.Session = connection.connection.driver_connection.pool.acquire() |
There was a problem hiding this comment.
nitpick: We have to return this session to the pool at the end, but it's not crucial in tests I think
There was a problem hiding this comment.
It can be problem in tests too - pool has limited sessions (default 100), if the limit will be reached - code can't get new session.
It is not big problem in one place, but it will be problem in loops or if many tests will not return the session.
especially if reuse connection (and session pool) between tests.
It is better to fix it
There was a problem hiding this comment.
Added calling of delete() method for session
| ) | ||
| table.create(connection) | ||
|
|
||
| session: ydb.Session = connection.connection.driver_connection.pool.acquire() |
There was a problem hiding this comment.
It can be problem in tests too - pool has limited sessions (default 100), if the limit will be reached - code can't get new session.
It is not big problem in one place, but it will be problem in loops or if many tests will not return the session.
especially if reuse connection (and session pool) between tests.
It is better to fix it
test/test_core.py
Outdated
| @pytest.mark.parametrize( | ||
| "auto_partitioning_by_size,res", | ||
| [ | ||
| (None, 1), |
There was a problem hiding this comment.
Here and below:
What about import protobuf to tests and re-use constants from protobuf?
instead of hard-code int values?
import protobuf is bad for public interface, but so-ok for tests and better (by my opinion) then hard-codede values.
There was a problem hiding this comment.
Did I add it correctly?
from ydb._grpc.v4.protos import ydb_common_pb2
<...>
ydb_common_pb2.FeatureFlag.Status.ENABLED
No description provided.