Repository navigation
feat(clickhouse): adapt to derived DDL with honest capability declarations - #1
Merged
Merged
Conversation
Remove identity/identity_start/identity_increment/identity_clause from XxxColumnDefinition and XxxColumnOptions to match core's column-attribute migration; XxxColumnDefinition now forwards attributes.
Render the dialect-free column attributes channel in each backend's format_column_definition (it was silently dropped), and give backends whose identity syntax differs from the SQL-standard form a format_identity_clause override (MySQL/MariaDB AUTO_INCREMENT, SQL Server IDENTITY(seed, inc)). Oracle gates column COLLATE to 12.2+; MariaDB drops the duplicate dialect formatter so the attribute-aware mixin is authoritative.
Core added the ColumnAttributeSupport protocol and renamed the deriver plan entry to creation_plan(); update protocol conformance and derived-DDL tests.
ClickHouse parses COLLATE in column definitions only for MySQL migration compatibility and otherwise rejects it (NOT_IMPLEMENTED); SHOW COLUMNS reports the collation as always NULL. Return False from supports_column_collation so the attribute is not emitted.
ClickHouse renders the table comment as an inline clause and the column comment inside the column definition; declare supports_table_comment() and supports_column_comment() as True so the capability bits match the actual rendering behavior.
Inline table/column comments are now TableCommentClause / ColumnCommentClause nodes; render them through format_table_comment_clause / format_column_comment_clause. Tests updated to build the clause nodes.
Model base already defaults to "id"; fixtures should exercise the unspecified-PK default path.
supports_materialized_view() returned True while the backend implemented no MV
formatter, so it inherited the core ViewMixin and generated:
CREATE MATERIALIZED VIEW `mv1` AS SELECT ... WITH DATA
ClickHouse's documented grammar requires an ENGINE when TO is omitted, has no
WITH DATA / WITH NO DATA clause at all (the equivalents are POPULATE and EMPTY),
and removes views with DROP VIEW rather than DROP MATERIALIZED VIEW. All three
are invalid, yet the probe claimed support — the same class of defect as the
postgres MRO bug.
Now returns False: the generic expressions fail fast with
UnsupportedFeatureError instead of emitting SQL the server cannot execute. Real
ClickHouse materialized views need a ClickHouse-specific expression (TO / ENGINE
/ POPULATE / REFRESH / EMPTY, with insert-trigger rather than query-cache
semantics), tracked as a separate feature.
Tests: adds test_materialized_view_support.py locking the boundary (including
the concrete regression: never render WITH DATA); the case in
test_dialect_capabilities that asserted True now asserts False with the
rationale; the real MV case in query/test_query_features.py is kept (it uses raw
SQL, which was already the correct usage) and now explains why it does not go
through the expression layer.
test_materialized_view_supported in test_clickhouse_ddl_improvements also asserted supports_materialized_view() is True (surfaced by CI run 36208939214). Changed to assert False alongside its sibling in test_dialect_capabilities, with the same rationale.
Flipping supports_materialized_view() to False last round stopped the lying; it was not the destination. ClickHouse's materialized view semantics share almost nothing with the SQL-standard statement, so both flavours are now implemented against its native grammar. Two flavours (per the official CREATE VIEW documentation): - Incremental (insert trigger): TO <table> or ENGINE (ENGINE is mandatory without TO), optionally backfilled with POPULATE - Refreshable: REFRESH EVERY|AFTER [OFFSET] [RANDOMIZE FOR] [DEPENDS ON] [SETTINGS] [APPEND [INCREMENTAL]] [EMPTY] Operations: - CREATE MATERIALIZED VIEW [IF NOT EXISTS] ... AS SELECT - DROP VIEW [IF EXISTS] (ClickHouse removes MVs with DROP VIEW) - SYSTEM REFRESH VIEW (optionally SYSTEM WAIT VIEW to block until done) - ALTER TABLE ... MODIFY REFRESH (change the schedule; the statement replaces all refresh parameters at once) Server rules are validated in the constructor *and* the formatter: ENGINE/TO is mandatory; REFRESH needs at least one of EVERY/AFTER/DEPENDS ON; OFFSET requires EVERY; APPEND INCREMENTAL requires APPEND; POPULATE conflicts with REFRESH. The formatter validates too because a generic expression never reaches the constructor — which was a real hole: the generic CreateMaterializedViewExpression has neither TO nor ENGINE and previously produced SQL the server rejects. Clauses the generic expressions carry that ClickHouse lacks are refused rather than dropped: column aliases, TABLESPACE, storage parameters, WITH [NO] DATA (the error points at POPULATE/EMPTY), DROP ... CASCADE; refresh with_data and concurrent likewise. Kept as well: the backend-side assertion that a dialect claiming True must own the CREATE formatter and be ordered ahead of the core ViewMixin. Also fixed: the probe left in ClickHouseViewMixin last round is removed (it would shadow the new mixin), the base order is ClickHouseMaterializedViewMixin -> ClickHouseViewMixin -> ViewMixin, and ON CLUSTER goes through format_identifier per the existing format_drop_database_statement convention. Tests: 41 new cases covering both flavours, the schedule clauses, every invalid combination, clause order, and the concrete regression "never render WITH DATA". dialect+expression groups: 199 passed.
The relative import depth in show/expressions.py resolved to impl.clickhouse.show.dialect, which does not exist; the dialect module lives one level up at impl.clickhouse.dialect. Type checkers cannot resolve the import. Verified statically: the TYPE_CHECKING import resolves to an existing module and an existing top-level symbol, and no string annotation in the touched file references a name absent from the workspace.
… revisions The installed hook exec'd scripts/preflight.sh unconditionally. That hook is a local, untracked artifact, so it survives a checkout of any commit that predates the script, and on such a commit exec failed with a bare "No such file or directory" — blocking the commit with nothing to act on. In this repository scripts/preflight.sh first appears at 7ae8f16, so every earlier commit is affected. The failure is also silent in a dangerous way: it swallowed a `git commit --amend`, so a commit-message rewrite reported as applied when it had not been, and had to be redone. The hook now checks the script is executable and exits 0 otherwise, so a revision without the pre-flight simply skips it. Normal operation is unchanged. Also honours core.hooksPath. git ignores .git/hooks entirely once that is set, so the hook was being written to a location that would never run while this script still reported success. No repository sets it today, so that part is latent.
Both dependencies were pinned to feature/activerecord-derived-ddl, a branch that exists only to carry the derived-DDL work. Now that the core change has landed in release/v1.0.0.dev30 and the matching testsuite change in release/v1.0.0.dev17, pinning to the feature branch means CI keeps resolving a branch that will be deleted once the backend PRs merge. This is a prerequisite for merging: leaving the old ref in place means the first run after that branch disappears fails to resolve, not the code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adapts the ClickHouse backend to the derived-DDL model in
rhosocial/python-activerecord#121. The through-line here is capability honesty — the backend
was claiming support for things it could not actually render.
TABLESPACEandINHERITSare no longer silently dropped from table declarations.capabilities are advertised deliberately rather than by omission.
and real materialized views are supported. The tests that asserted the old probe
behaviour are realigned rather than deleted.
Comments use the core typed comment clause nodes, and the backend renders DDL through direct
DDL expressions.
DDLSourcecollection, table inheritance and tablespace are covered.Type of change
feat: A new featuredocs: Documentation only changes (if related to the new feature)test: Adding missing tests or correcting existing tests (for the new feature)chore: Changes to the build process or auxiliary tools and libraries (if related to the new feature)Breaking Change
No commit carries a
!marker. The public model and query APIs are unchanged.Impact on End-Users:
No backward-incompatible API changes.
TABLESPACEandINHERITSare no longer accepted anddropped, and capabilities the backend cannot honour are no longer reported as available. Code
that relied on either will now fail instead of quietly getting a different schema.
Impact on Backend Developers (Custom Implementations):
collation is declared unsupported, and the materialized-view probe is corrected. A
conformance suite asserting the old values will fail.
nodes.
typing.Listimport is dropped and TYPE_CHECKING-only imports broken by theexpression namespace split are repaired.
Related Repositories/Packages
The core half is rhosocial/python-activerecord#121 (merged into
release/v1.0.0.dev30) andthe shared contract half is rhosocial/python-activerecord-testsuite#21 (merged into
release/v1.0.0.dev17).This PR also repoints this repository's workflow at those release branches. Unlike the other
backends, this repository fetches the dependencies with
actions/checkoutrather thanpip install git+…, so the tworef:values are what changed; that is also the onedependency style in the family that will not trip the branch-security rating.
Merged or in flight: mysql #56, postgres #58, mariadb #5, sqlserver #4, oracle. Still to
merge: firebird, snowflake, bigquery.
Testing
Local full-suite execution was skipped at the user's instruction. Verification relied on
remote CI on this exact tip
03c6d8c, which is also the first execution against the releasebranches rather than the feature branch, so the new
ref:values are verified to resolve.Checklist
changelog.d/(to be added after this PR isopened, named by the actual PR number)
Additional Notes
test(view): align the second case that asserted the old probe behaviouris worth readingcarefully: the probe previously reported support for DDL it could not emit, so the test
encoded the wrong expectation. It is realigned rather than removed, and the single case that
genuinely covered a separate behaviour is untouched.