fix: Contract enforcement for FixedString(N) columns - #727
Conversation
ClickHouseColumn.data_type collapsed FixedString(N) to String, so enforced contracts failed with a 'data type mismatch' when the YAML contract declared FixedString(N), while a String contract incorrectly passed against a FixedString(N) column. Render parameterized FixedString exactly so the verbatim type comparison in clickhouse__get_assert_columns_equivalent matches.
Mirror the numeric_type pattern: is_fixed_string dispatches to a dedicated fixedstring_type(size) classmethod instead of an inline conditional in data_type. No behavior change.
bartcode
changed the title
There was a problem hiding this comment.
Pull request overview
Preserves FixedString(N) type parameters for contract enforcement and schema-change handling.
Changes:
- Render sized
FixedStringtypes exactly. - Add column and contract regression coverage.
- Document the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
dbt/adapters/clickhouse/column.py |
Preserves sized FixedString rendering. |
tests/integration/adapter/column_types/test_column_types.py |
Tests direct and wrapped rendering. |
tests/integration/adapter/constraints/test_constraints.py |
Adds contract coverage. |
CHANGELOG.md |
Records the bug fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return [ | ||
| ["1::Int32", "Int32", "Int32"], | ||
| ["'1'", "String", "String"], | ||
| ["toFixedString('1', 16)", "FixedString(16)", "FixedString(16)"], |
There was a problem hiding this comment.
Good point. The Int128 loop could never catch this direction. Added a test in 1126e84: the model produces toFixedString('1', 16) and the contract declares String, and the run is expected to fail preflight with 'data type mismatch'. Confirmed it fails without the fix (that's exactly the silent pass from #726) and passes with it.
| if self.is_fixed_string(): | ||
| # Contract enforcement compares data_type strings verbatim against | ||
| # YAML data_type declarations, so the exact type must be preserved | ||
| data_t = self.fixedstring_type(self.string_size()) |
There was a problem hiding this comment.
Added in the same commit: TestFixedStringSchemaChange covers append_new_columns and sync_all_columns with FixedString columns, for both regular and distributed incremental. It checks system.columns after the ALTERs. A newly added column has to come in as FixedString(16), and a sync from FixedString(16) to FixedString(32) has to end up as FixedString(32). Also made sure a third run stays clean, since without the fix the column was added as String and the next run hit the drift error.
Cover the two gaps from the PR ClickHouse#727 review: - a String contract now has a preflight-failure test against a FixedString(N) model column (previously passed silently, ClickHouse#726) - append_new_columns ADD COLUMN and sync_all_columns MODIFY COLUMN are pinned to the exact FixedString(N) type in ALTER DDL, including a clean third incremental run
Fixes #726
Problem
ClickHouseColumn.data_typerendered every string-family column asStringbecausestring_type()ignores its size argument, soFixedString(N)collapsed toString. Contract enforcement comparesdata_typestrings verbatim against YAMLdata_typedeclarations, which produced two bugs (#726):FixedString(N)contract failed against aFixedString(N)model column with "data type mismatch"Stringcontract silently passed against aFixedString(N)column; the table was then created with aStringcolumn while the model producesFixedString(N), and later incremental runs failed with a misleadingNew column types: ['col String']error, since the drift message renders the type through the same collapsing propertyChange
data_typenow renders parameterizedFixedString(N)exactly (FixedString(16)instead ofString). BareFixedString(not a valid ClickHouse type) and plainStringkeep their previous rendering, andstring_type()/string_size()are unchanged, so schema-expansion checks (can_expand_to) are unaffected.Side effects (all improvements)
append_new_columns/sync_all_columnsALTERs now use the exact type (ADD/MODIFY COLUMN ... FixedString(N)is valid ClickHouse DDL) instead of silently widening the column toString. The row in dbt Core 2.0 parity: Materializationincremental#704's Fusion parity table documents the old rendering and should be updated when this lands.check_incremental_schema_changeserror messages now display the actual column type instead of a collapsedString.Testing
FixedString(16)rendering, plain and throughNullable/LowCardinalitywrappers; fixed a stale assertion intest_low_cardinality_nullable_typethat only held because of the collapse.FixedString(16)case to the contractdata_typesfixture, exercised across table/view/distributed materializations (both matching and mismatching paths); verified the new test fails without the code fix.make lintclean; 92 unit/column tests and 21 contract integration tests pass against dockerized ClickHouse.