Fix TableEngine construction for alembic-rendered zero-arg engines and SummingMergeTree columns - #947
Conversation
…d SummingMergeTree columns Two bugs in clickhouse_connect/cc_sqlalchemy/ddl/tableengine.py, both from ClickHouse#946: 1. TableEngine.__init__(self, kwargs) had no default for `kwargs`. Engines with no required/optional constructor args (Memory, Log, StripeLog, TinyLog, Null, Set) don't define their own __init__, so they fall straight through to TableEngine.__init__. repr(Memory({})) already produced "Memory()", but calling that rendered code back -- exactly what Alembic does when it writes a migration from repr() -- raised "TypeError: __init__() missing 1 required positional argument: 'kwargs'". Fixed by giving kwargs a None default that's normalized to {} inside the constructor. 2. SummingMergeTree had no way to receive the column list that ClickHouse's own SummingMergeTree([columns]) engine accepts (e.g. SummingMergeTree((delta, n_tx)) ORDER BY id). It was a bare `class SummingMergeTree(MergeTree): pass`, so there was no `columns` parameter anywhere in its signature. Fixed by giving it its own constructor -- following the same pattern already used by ReplacingMergeTree/CollapsingMergeTree/etc: extend TableEngine directly (not MergeTree, whose narrower signature has no room for `columns`), add `columns` to arg_names as an optional positional arg, and extend TableEngine.__init__'s positional-arg rendering to accept a tuple/list value and render it as a parenthesized column list, matching how eng_params already renders tuples for ORDER BY/PARTITION BY. Testing: added 6 tests to tests/unit_tests/test_sqlalchemy/test_alembic.py covering both bugs, including the exact repr(Memory({})) -> eval() round trip from the issue and a SummingMergeTree repr -> eval round trip. All 455 unit tests pass (449 existing + 6 new). Reverting the fix makes 5 of the 6 new tests fail with the exact symptoms from the issue. ruff check is clean.
|
|
There was a problem hiding this comment.
Hi @agu2347 looks good! I added a few finishing touches that do the following:
- Preserves the existing
SummingMergeTreepositional API andMergeTreeinheritance - adds matching
columns=support forReplicatedSummingMergeTree - validates column-list inputs
- address missing type declarations
- adds the CHANGELOG entry
To get this merged, please sign the CLA. thanks!
|
Hey @agu2347 still waiting for CLA signing to merge. |
|
Hi @agu2347 just a friendly reminder to see if you're able to get the CLA signed. This wont' be able to merge until you do. Thanks! |
|
Hey @agu2347 if you're unable to sing the CLA I'm unfortunately going to have to close this PR. Please try and sign it soon! |
Closes #946.
Two independent bugs in
clickhouse_connect/cc_sqlalchemy/ddl/tableengine.py:1. Zero-arg engines can't be constructed from their own
repr().Memory,Log,StripeLog,TinyLog,Null, andSetdon't define their own__init__, so they fall through toTableEngine.__init__(self, kwargs), which had no default forkwargs.repr(Memory({}))already produced"Memory()", but Alembic's autogeneration writes exactly that no-arg call into the migration file, and running that migration executesMemory()-- which raisedTypeError: __init__() missing 1 required positional argument: 'kwargs'.Fixed by giving
kwargsaNonedefault, normalized to{}inside the constructor.2.
SummingMergeTreehas no way to accept a column list.ClickHouse's
SummingMergeTree([columns])engine takes an optional list of columns to sum on merge (e.g.SummingMergeTree((delta, n_tx)) ORDER BY id), but the Python class was a bareclass SummingMergeTree(MergeTree): pass-- nocolumnsparameter existed anywhere in the constructor chain.Fixed by giving
SummingMergeTreeits own constructor, following the same pattern already used byReplacingMergeTree/CollapsingMergeTree/etc: extendTableEnginedirectly (notMergeTree, whose narrower signature has no room forcolumns), addcolumnstoarg_namesas an optional positional arg, and extendTableEngine.__init__'s positional-arg rendering to accept a tuple/list value and render it as a parenthesized column list -- mirroring howeng_paramsalready renders tuples forORDER BY/PARTITION BY.Testing
Added 6 tests to
tests/unit_tests/test_sqlalchemy/test_alembic.py:test_no_arg_table_engines_accept_zero_args-- all six zero-arg engines construct and round-trip.test_memory_alembic_repr_round_trips-- the exact scenario from the issue:repr(Memory({}))produces code that evals back cleanly.test_summing_merge_tree_accepts_columns--SummingMergeTree(columns=(...), order_by=...)compiles to valid DDL.test_summing_merge_tree_columns_optional--columnsremains optional.test_summing_merge_tree_requires_order_by_or_primary_key-- existing MergeTree-family constraint preserved.test_summing_merge_tree_repr_round_trips-- repr -> eval round trip with columns.All 455 unit tests in
tests/unit_tests/test_sqlalchemy/pass (449 existing + 6 new), confirmed with no regressions. Reverting the fix makes 5 of the 6 new tests fail with the exact symptoms described in the issue (the 6th doesn't exercise the changed code path, so it correctly still passes).ruff checkis clean on both changed files.