From 85652b07b852bcbe671069e1cb5198d8a32b7129 Mon Sep 17 00:00:00 2001 From: KeKs0r Date: Sat, 26 Sep 2026 18:57:50 -0700 Subject: [PATCH 1/5] =?UTF-8?q?=E2=9C=A8=20Support=20column=20expression?= =?UTF-8?q?=20kinds=20(#206)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vex-Session: session-3bdf6c82f39781ebe0b0ef8f --- .changeset/column-expression-kinds.md | 12 + .../src/content/docs/schema/dsl-reference.mdx | 64 +++++ chkit_python/CHANGELOG.md | 5 + chkit_python/src/chkit/__init__.py | 2 + .../src/chkit/cli/commands/drift_compare.py | 1 + chkit_python/src/chkit/cli/commands/pull.py | 6 +- .../src/chkit/cli/commands/pull_render.py | 2 + .../src/chkit/clickhouse/introspect.py | 30 ++- chkit_python/src/chkit/core/__init__.py | 2 + chkit_python/src/chkit/core/canonical.py | 1 + chkit_python/src/chkit/core/model.py | 6 + chkit_python/src/chkit/core/planner.py | 11 +- chkit_python/src/chkit/core/sql.py | 16 +- chkit_python/src/chkit/core/validate.py | 20 ++ .../src/chkit_plugin_backfill/planner.py | 18 +- .../chkit_plugin_codegen/type_artifacts.py | 17 +- chkit_python/tests/test_column_expressions.py | 225 +++++++++++++++++ .../tests/test_column_expressions_e2e.py | 113 +++++++++ chkit_python/tests/test_introspect.py | 5 +- packages/cli/src/commands/drift/compare.ts | 1 + .../src/test/column-expressions.e2e.test.ts | 232 ++++++++++++++++++ .../clickhouse/src/column-expressions.test.ts | 51 ++++ packages/clickhouse/src/index.ts | 17 +- packages/core/src/canonical.ts | 4 +- packages/core/src/column-expressions.test.ts | 127 ++++++++++ packages/core/src/model-types.ts | 6 + packages/core/src/planner.ts | 7 +- packages/core/src/sql.ts | 14 +- packages/core/src/validate.ts | 15 ++ packages/plugin-backfill/src/planner.test.ts | 30 +++ packages/plugin-backfill/src/planner.ts | 14 +- .../src/generators/ingest-artifacts.ts | 16 +- .../plugin-codegen/src/generators/shared.ts | 9 + .../src/generators/type-artifacts.ts | 15 +- packages/plugin-pull/src/render-schema.ts | 1 + 35 files changed, 1074 insertions(+), 41 deletions(-) create mode 100644 .changeset/column-expression-kinds.md create mode 100644 chkit_python/tests/test_column_expressions.py create mode 100644 chkit_python/tests/test_column_expressions_e2e.py create mode 100644 packages/cli/src/test/column-expressions.e2e.test.ts create mode 100644 packages/clickhouse/src/column-expressions.test.ts create mode 100644 packages/core/src/column-expressions.test.ts diff --git a/.changeset/column-expression-kinds.md b/.changeset/column-expression-kinds.md new file mode 100644 index 00000000..223248c4 --- /dev/null +++ b/.changeset/column-expression-kinds.md @@ -0,0 +1,12 @@ +--- +"@chkit/core": minor +"@chkit/clickhouse": minor +"chkit": minor +"@chkit/plugin-pull": minor +"@chkit/plugin-codegen": minor +"@chkit/plugin-backfill": patch +--- + +Support `MATERIALIZED`, `ALIAS`, and `EPHEMERAL` column expressions with `defaultKind`, preserving kinds through SQL rendering, pull, snapshots, and drift. Keep existing defaults and snapshots stable; use the existing `fn:` prefix for SQL expressions and allow expressionless `EPHEMERAL` columns. + +Generate separate row and insert shapes for tables with special column kinds. Exclude generated columns from automatic backfill insert projections and reject automatic backfills that cannot reconstruct ephemeral inputs. Emit explicit removal of stored expressions, require manual migrations for storage-kind conversions involving `ALIAS` or `EPHEMERAL`, and never automatically rewrite historical materialized values. diff --git a/apps/docs/src/content/docs/schema/dsl-reference.mdx b/apps/docs/src/content/docs/schema/dsl-reference.mdx index 510f2c21..e50def24 100644 --- a/apps/docs/src/content/docs/schema/dsl-reference.mdx +++ b/apps/docs/src/content/docs/schema/dsl-reference.mdx @@ -281,6 +281,70 @@ Default value for the column. +### `defaultKind` (optional) + +Choose `DEFAULT` (the implicit default), `MATERIALIZED`, `ALIAS`, or `EPHEMERAL`. +Python also accepts `default_kind`. The `default` field holds the value or expression +for every kind: strings remain SQL literals, and `fn:` marks raw SQL expressions. + +| Kind | Behavior | +| --- | --- | +| `DEFAULT` | Stored; the expression applies when the insert omits the value. | +| `MATERIALIZED` | Computed on insert and stored; cannot be supplied in a normal insert. | +| `ALIAS` | Computed when explicitly selected; neither stored nor insertable. | +| `EPHEMERAL` | Input for other column expressions; neither stored nor selectable. | + + + + ```ts + columns: [ + { name: 'ts', type: 'DateTime' }, + { name: 'day', type: 'Date', defaultKind: 'MATERIALIZED', default: 'fn:toDate(ts)' }, + { name: 'label', type: 'String', defaultKind: 'ALIAS', default: 'fn:toString(day)' }, + { name: 'raw', type: 'String', defaultKind: 'EPHEMERAL' }, + { name: 'size', type: 'UInt64', default: 'fn:length(raw)' }, + ] + ``` + + + ```python + columns=[ + {"name": "ts", "type": "DateTime"}, + {"name": "day", "type": "Date", "default_kind": "MATERIALIZED", "default": "fn:toDate(ts)"}, + {"name": "label", "type": "String", "default_kind": "ALIAS", "default": "fn:toString(day)"}, + {"name": "raw", "type": "String", "default_kind": "EPHEMERAL"}, + {"name": "size", "type": "UInt64", "default": "fn:length(raw)"}, + ] + ``` + + + +`MATERIALIZED` and `ALIAS` require a value or expression. `EPHEMERAL` may omit it; +supply these inputs with an explicit insert column list. ClickHouse normally +excludes all three special kinds from `SELECT *`. + +`pull`, snapshots, and drift preserve the column kind. Omitted kind and explicit +`DEFAULT` compare identically, so existing snapshots need no migration. Keep the +base type in `type`; do not embed `MATERIALIZED ...` in the type string. + +Changing a stored expression emits `MODIFY COLUMN`, without rewriting historical +values. Use a separately reviewed `ALTER TABLE ... MATERIALIZE COLUMN ...` if you +need that rewrite. Removing a `DEFAULT` or `MATERIALIZED` expression emits an +explicit `REMOVE` clause. Automatic kind conversions involving `ALIAS` or +`EPHEMERAL` are rejected: use an [empty manual migration](/cli/generate/) and review +the storage and insert behavior before updating the snapshot. The planner never +silently drops and recreates a column to perform these conversions. + +Generated row models exclude `EPHEMERAL` columns. Tables using a special kind also +get a separate `RowInsert` model that excludes `MATERIALIZED` and `ALIAS`; generated +TypeScript ingest helpers use that model. Read models describe an explicit projection +of all readable columns, rather than the default `SELECT *` result. Field requiredness +is unchanged: insert models require their declared input fields, including defaults. + +Automatic backfill projections omit `MATERIALIZED` and `ALIAS` columns. Targets with +`EPHEMERAL` columns require an explicit SQL insert with an input mapping, because +these inputs cannot be recovered from stored rows. + ### `comment` (string, optional) Column-level comment rendered in SQL. diff --git a/chkit_python/CHANGELOG.md b/chkit_python/CHANGELOG.md index 2bbf2b55..b877194e 100644 --- a/chkit_python/CHANGELOG.md +++ b/chkit_python/CHANGELOG.md @@ -18,6 +18,11 @@ - Preserve quoted clause names, delimiters, whitespace, and escaped trailing backslashes in table introspection and migration statement splitting. + +- Support `default_kind` / `defaultKind` for `DEFAULT`, `MATERIALIZED`, `ALIAS`, and `EPHEMERAL` columns through SQL rendering, introspection, pull, snapshots, and drift. Existing defaults and snapshots remain compatible. Use `fn:` for SQL expressions; expressionless `EPHEMERAL` is supported. +- Generate separate read/insert models for tables with special column kinds, and make backfill projections respect generated columns. Automatic backfills with ephemeral inputs require explicit SQL input mappings. +- Emit explicit removal of stored column expressions; require manual migrations for kind conversions involving `ALIAS` or `EPHEMERAL`. Expression changes never automatically materialize historical data. + ## 0.2.0 — 2026-08-10 **Full parity with the TypeScript chkit.** Every remaining gap is closed; diff --git a/chkit_python/src/chkit/__init__.py b/chkit_python/src/chkit/__init__.py index fe1fe444..e1cebfdb 100644 --- a/chkit_python/src/chkit/__init__.py +++ b/chkit_python/src/chkit/__init__.py @@ -7,6 +7,7 @@ ChxUserClickHouseConfig, ChxUserConfig, ChxValidationError, + ColumnDefaultKind, ColumnDefinition, DictionaryAttribute, DictionaryDefinition, @@ -60,6 +61,7 @@ "ChxUserClickHouseConfig", "ChxUserConfig", "ChxValidationError", + "ColumnDefaultKind", "ColumnDefinition", "DictionaryAttribute", "DictionaryDefinition", diff --git a/chkit_python/src/chkit/cli/commands/drift_compare.py b/chkit_python/src/chkit/cli/commands/drift_compare.py index 71869ecb..861ca292 100644 --- a/chkit_python/src/chkit/cli/commands/drift_compare.py +++ b/chkit_python/src/chkit/cli/commands/drift_compare.py @@ -240,6 +240,7 @@ def _normalize_default_value(value: str) -> str: f"type={str(column.type).strip()}", f"nullable={'1' if column.nullable else '0'}", f"default={normalized_default}", + f"defaultKind={column.default_kind or 'DEFAULT'}", f"comment={(column.comment or '').strip()}", ] return "|".join(parts) diff --git a/chkit_python/src/chkit/cli/commands/pull.py b/chkit_python/src/chkit/cli/commands/pull.py index 9f2ed103..689e29e3 100644 --- a/chkit_python/src/chkit/cli/commands/pull.py +++ b/chkit_python/src/chkit/cli/commands/pull.py @@ -107,7 +107,11 @@ def _introspected_table_to_definition( database=item.database, name=item.name, engine=item.engine or "MergeTree", - columns=list(item.columns), + columns=[ + column.model_copy(update={"default": f"fn:{column.default}"}) + if isinstance(column.default, str) else column + for column in item.columns + ], primary_key=[] if kafka else _split_clause(item.primary_key) or [item.columns[0].name], order_by=[] if kafka else _split_clause(item.order_by) or [item.columns[0].name], unique_key=_split_clause(item.unique_key) or None, diff --git a/chkit_python/src/chkit/cli/commands/pull_render.py b/chkit_python/src/chkit/cli/commands/pull_render.py index c723cca5..0d4f1656 100644 --- a/chkit_python/src/chkit/cli/commands/pull_render.py +++ b/chkit_python/src/chkit/cli/commands/pull_render.py @@ -219,6 +219,8 @@ def _render_column(column: ColumnDefinition) -> str: ] if column.nullable: parts.append("nullable=True") + if column.default_kind and column.default_kind != "DEFAULT": + parts.append(f"default_kind={_render_string(column.default_kind)}") if column.default is not None: parts.append(f"default={_render_literal(column.default)}") if column.comment: diff --git a/chkit_python/src/chkit/clickhouse/introspect.py b/chkit_python/src/chkit/clickhouse/introspect.py index 7d5c6ce3..ea8aae82 100644 --- a/chkit_python/src/chkit/clickhouse/introspect.py +++ b/chkit_python/src/chkit/clickhouse/introspect.py @@ -148,20 +148,28 @@ def normalize_column_from_system_row(row: SystemColumnRow) -> ColumnDefinition: nullable = bool(inner) default_value: str | None = None - if row.default_expression and row.default_kind == "DEFAULT": - default_value = normalize_sql_fragment(row.default_expression) - + kind = row.default_kind + if kind and kind not in {"DEFAULT", "MATERIALIZED", "ALIAS", "EPHEMERAL"}: + raise ValueError(f"Unsupported column default kind: {kind}") + if row.default_expression and kind: + # Preserve whitespace inside SQL string literals when pulling expressions. + default_value = row.default_expression.strip() + + escaped_type = row.type.replace("'", "\\'") + if kind == "EPHEMERAL" and default_value == f"defaultValueOfTypeName('{escaped_type}')": + default_value = None codec_steps = parse_codec(row.compression_codec) comment = row.comment.strip() if row.comment is not None else None - return ColumnDefinition( - name=row.name, - type=type_, - nullable=nullable or None, - default=default_value, - comment=comment or None, - codec=codec_steps, - ) + return ColumnDefinition.model_validate({ + "name": row.name, + "type": type_, + "nullable": nullable or None, + "default": default_value, + "defaultKind": kind if kind and kind != "DEFAULT" else None, + "comment": comment or None, + "codec": codec_steps, + }) def _split_int_args(args: str | None) -> list[int]: diff --git a/chkit_python/src/chkit/core/__init__.py b/chkit_python/src/chkit/core/__init__.py index ae9262ff..67420ce8 100644 --- a/chkit_python/src/chkit/core/__init__.py +++ b/chkit_python/src/chkit/core/__init__.py @@ -36,6 +36,7 @@ ChxValidationError, ColumnCodec, ColumnCodecSpec, + ColumnDefaultKind, ColumnDefinition, DictionaryAttribute, DictionaryDefinition, @@ -99,6 +100,7 @@ "ChxValidationError", "ColumnCodec", "ColumnCodecSpec", + "ColumnDefaultKind", "ColumnDefinition", "DictionaryAttribute", "DictionaryDefinition", diff --git a/chkit_python/src/chkit/core/canonical.py b/chkit_python/src/chkit/core/canonical.py index 2fe9bf9d..1d3c7d57 100644 --- a/chkit_python/src/chkit/core/canonical.py +++ b/chkit_python/src/chkit/core/canonical.py @@ -52,6 +52,7 @@ def _canonicalize_column(column: ColumnDefinition) -> ColumnDefinition: canon_type = type_value.strip() if isinstance(type_value, str) else type_value return column.model_copy( update={ + "default_kind": None if column.default_kind == "DEFAULT" else column.default_kind, "name": column.name.strip(), "renamed_from": column.renamed_from.strip() if column.renamed_from is not None diff --git a/chkit_python/src/chkit/core/model.py b/chkit_python/src/chkit/core/model.py index b93a80bf..2c30362d 100644 --- a/chkit_python/src/chkit/core/model.py +++ b/chkit_python/src/chkit/core/model.py @@ -124,12 +124,16 @@ class RawColumnCodec(_StrictModel): ColumnType: TypeAlias = PrimitiveColumnType | str +ColumnDefaultKind: TypeAlias = Literal["DEFAULT", "MATERIALIZED", "ALIAS", "EPHEMERAL"] + + class ColumnDefinition(_StrictModel): name: str type: ColumnType renamed_from: str | None = Field(default=None, alias="renamedFrom") nullable: bool | None = None default: str | int | float | bool | None = None + default_kind: ColumnDefaultKind | None = Field(default=None, alias="defaultKind") comment: str | None = None codec: ColumnCodecSpec | None = None @@ -692,6 +696,8 @@ class MigrationPlan(_StrictModel): "codec_chain_must_end_with_general", "codec_chain_multiple_general", "codec_chain_empty", + "column_default_kind_invalid", + "column_expression_required", "dictionary_missing_primary_key", "dictionary_primary_key_missing_attribute", "dictionary_missing_source", diff --git a/chkit_python/src/chkit/core/planner.py b/chkit_python/src/chkit/core/planner.py index 93c69e83..255f3f17 100644 --- a/chkit_python/src/chkit/core/planner.py +++ b/chkit_python/src/chkit/core/planner.py @@ -439,10 +439,19 @@ def _diff_tables( ) ) for column_change in column_diff.changed: + old_kind = column_change.old_item.default_kind or "DEFAULT" + new_kind = column_change.new_item.default_kind or "DEFAULT" + if old_kind != new_kind and {old_kind, new_kind} & {"ALIAS", "EPHEMERAL"}: + raise ValueError( + f"Cannot automatically change column {new.database}.{new.name}." + f"{column_change.name} " + f"from {old_kind} to {new_kind}; " + "use an explicit manual migration for storage-kind changes" + ) sql = ( render_alter_remove_codec(new, column_change.name) if _is_codec_removal(column_change.old_item, column_change.new_item) - else render_alter_modify_column(new, column_change.new_item) + else render_alter_modify_column(new, column_change.new_item, column_change.old_item) ) ops.append( MigrationOperation( diff --git a/chkit_python/src/chkit/core/sql.py b/chkit_python/src/chkit/core/sql.py index 78a8863f..cba58f57 100644 --- a/chkit_python/src/chkit/core/sql.py +++ b/chkit_python/src/chkit/core/sql.py @@ -73,7 +73,9 @@ def _render_column(col: ColumnDefinition) -> str: type_text = f"Nullable({col.type})" if col.nullable else f"{col.type}" out = f"`{col.name}` {type_text}" if col.default is not None: - out += f" DEFAULT {_render_default(col.default)}" + out += f" {col.default_kind or 'DEFAULT'} {_render_default(col.default)}" + elif col.default_kind == "EPHEMERAL": + out += " EPHEMERAL" if col.comment is not None and len(col.comment) > 0: escaped = col.comment.replace("'", "''") out += f" COMMENT '{escaped}'" @@ -330,11 +332,19 @@ def render_alter_add_column(definition: TableDefinition, column: ColumnInput) -> ) -def render_alter_modify_column(definition: TableDefinition, column: ColumnInput) -> str: +def render_alter_modify_column( + definition: TableDefinition, column: ColumnInput, previous: ColumnDefinition | None = None +) -> str: normalized = _normalize_column(column) + remove = "" + if ( + previous is not None and previous.default is not None + and normalized.default is None and normalized.default_kind != "EPHEMERAL" + ): + remove = f", MODIFY COLUMN `{normalized.name}` REMOVE {previous.default_kind or 'DEFAULT'}" return ( f"ALTER TABLE {definition.database}.{definition.name} " - f"MODIFY COLUMN {_render_column(normalized)};" + f"MODIFY COLUMN {_render_column(normalized)}{remove};" ) diff --git a/chkit_python/src/chkit/core/validate.py b/chkit_python/src/chkit/core/validate.py index d279a614..8ff41e21 100644 --- a/chkit_python/src/chkit/core/validate.py +++ b/chkit_python/src/chkit/core/validate.py @@ -180,6 +180,25 @@ def _validate_kafka_table(definition: TableDefinition, issues: list[ValidationIs ) +def _validate_column_expression( + definition: TableDefinition, column: ColumnDefinition, issues: list[ValidationIssue] +) -> None: + if ( + column.default_kind in {"MATERIALIZED", "ALIAS"} and column.default is None + ) or ( + isinstance(column.default, str) + and column.default.startswith("fn:") + and not column.default[3:].strip() + ): + _push( + issues, + definition, + "column_expression_required", + f'Column "{column.name}" requires a non-empty expression; ' + "use fn: for SQL expressions", + ) + + def _validate_table(definition: TableDefinition, issues: list[ValidationIssue]) -> None: _validate_kafka_table(definition, issues) column_seen: set[str] = set() @@ -196,6 +215,7 @@ def _validate_table(definition: TableDefinition, issues: list[ValidationIssue]) continue column_seen.add(column.name) column_set.add(column.name) + _validate_column_expression(definition, column, issues) _validate_column_codec(definition, column, issues) _validate_indexes(definition, issues) diff --git a/chkit_python/src/chkit_plugin_backfill/planner.py b/chkit_python/src/chkit_plugin_backfill/planner.py index 32028c0a..947f1037 100644 --- a/chkit_python/src/chkit_plugin_backfill/planner.py +++ b/chkit_python/src/chkit_plugin_backfill/planner.py @@ -71,8 +71,6 @@ def _detect_backfill_strategy( try: definitions = load_schema_definitions(schema, cwd=config_dir) mvs = find_mvs_for_target(definitions, database, table) - if len(mvs) == 0: - return _BackfillStrategy(mvs=[]) table_def = next( ( @@ -84,15 +82,29 @@ def _detect_backfill_strategy( ), None, ) + if table_def is not None and any( + column.default_kind == "EPHEMERAL" for column in table_def.columns + ): + raise BackfillConfigError( + "Automatic backfill cannot reconstruct EPHEMERAL inputs; " + "use an explicit INSERT with an input column mapping." + ) + if len(mvs) == 0: + return _BackfillStrategy(mvs=[]) return _BackfillStrategy( mvs=mvs, mv_replay_queries=[mv.as_ for mv in mvs], target_columns=( - [column.name for column in table_def.columns] + [ + column.name for column in table_def.columns + if column.default_kind in {None, "DEFAULT"} + ] if table_def is not None else None ), ) + except BackfillConfigError: + raise except Exception: # Schema load failed, fall back to direct copy. return _BackfillStrategy(mvs=[]) diff --git a/chkit_python/src/chkit_plugin_codegen/type_artifacts.py b/chkit_python/src/chkit_plugin_codegen/type_artifacts.py index cba1ffe5..f0afd095 100644 --- a/chkit_python/src/chkit_plugin_codegen/type_artifacts.py +++ b/chkit_python/src/chkit_plugin_codegen/type_artifacts.py @@ -302,9 +302,22 @@ def _render_table_model( options: CodegenOptions, ) -> tuple[list[str], list[CodegenFinding], set[str]]: """Render the lines for a single table → Pydantic model.""" - return _render_fields_model( - list(table.columns), class_name, f"{table.database}.{table.name}", options + lines, findings, imports = _render_fields_model( + [column for column in table.columns if column.default_kind != "EPHEMERAL"], + class_name, f"{table.database}.{table.name}", options ) + if any(column.default_kind not in {None, "DEFAULT"} for column in table.columns): + insert_lines, insert_findings, insert_imports = _render_fields_model( + [ + column for column in table.columns + if column.default_kind not in {"MATERIALIZED", "ALIAS"} + ], + f"{class_name}Insert", f"{table.database}.{table.name}", options + ) + lines.extend(insert_lines) + findings.extend(insert_findings) + imports.update(insert_imports) + return lines, findings, imports def _render_dictionary_model( diff --git a/chkit_python/tests/test_column_expressions.py b/chkit_python/tests/test_column_expressions.py new file mode 100644 index 00000000..c54d5731 --- /dev/null +++ b/chkit_python/tests/test_column_expressions.py @@ -0,0 +1,225 @@ +"""Column expression lifecycle and legacy snapshot compatibility (#206).""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +import pytest + +from chkit import ColumnDefinition, table +from chkit.cli.commands.drift_compare import compare_table_shape +from chkit.cli.commands.pull import _introspected_table_to_definition +from chkit.cli.commands.pull_render import render_schema_file +from chkit.clickhouse.introspect import ( + IntrospectedTable, + SystemColumnRow, + normalize_column_from_system_row, +) +from chkit.core.model import Snapshot, TableDefinition +from chkit.core.planner import plan_diff +from chkit.core.snapshot import create_snapshot +from chkit.core.sql import to_create_sql +from chkit.core.validate import validate_definitions +from chkit_plugin_backfill.planner import _detect_backfill_strategy +from chkit_plugin_codegen import generate_type_artifacts + + +def definition(**column: Any) -> TableDefinition: + return table( + database="default", + name="events", + engine="MergeTree()", + primary_key=["ts"], + order_by=["ts"], + columns=[ + {"name": "ts", "type": "DateTime"}, + {"name": "day", "type": "Date", **column}, + ], + ) + + +def actual_table(columns: list[ColumnDefinition]) -> IntrospectedTable: + return IntrospectedTable( + database="default", + name="events", + columns=columns, + settings={}, + indexes=[], + projections=[], + engine="MergeTree()", + primary_key="ts", + order_by="ts", + ) + + +@pytest.mark.parametrize("kind", ["DEFAULT", "MATERIALIZED", "ALIAS", "EPHEMERAL"]) +def test_roundtrip_kind_and_expression(kind: str) -> None: + expected = definition(default_kind=kind, default="fn:toDate(ts)") + actual = normalize_column_from_system_row( + SystemColumnRow( + database="default", + table="events", + name="day", + type="Date", + position=2, + default_kind=kind, + default_expression="toDate(ts)", + ) + ) + live = actual_table([expected.columns[0], actual]) + assert compare_table_shape(expected, live) is None + pulled = _introspected_table_to_definition(live) + assert pulled is not None + source = render_schema_file([pulled]) + namespace: dict[str, Any] = {} + exec(source, namespace) + reloaded = namespace["definitions"][0] + assert plan_diff([expected], [reloaded]).operations == [] + assert f"`day` Date {kind} toDate(ts)" in to_create_sql(reloaded) + assert plan_diff([reloaded], [reloaded]).operations == [] + + +def test_bare_ephemeral_synthetic_expression_is_normalized() -> None: + expected = definition(default_kind="EPHEMERAL") + actual = normalize_column_from_system_row( + SystemColumnRow( + database="default", + table="events", + name="day", + type="Date", + position=2, + default_kind="EPHEMERAL", + default_expression="defaultValueOfTypeName('Date')", + ) + ) + assert actual.default is None + assert actual.default_kind == "EPHEMERAL" + assert compare_table_shape(expected, actual_table([expected.columns[0], actual])) is None + assert "`day` Date EPHEMERAL" in to_create_sql(expected) + + +def test_legacy_snapshot_stability() -> None: + for value in (None, 0, False, "", "fn:toDate(ts)"): + old = definition(default=value) + explicit = definition(default_kind="DEFAULT", default=value) + payload = create_snapshot([old]).model_dump(mode="json", by_alias=True, exclude_none=True) + assert "defaultKind" not in str(payload) + legacy = Snapshot.model_validate(payload) + assert plan_diff(list(legacy.definitions), [explicit]).operations == [] + assert "defaultKind" not in str( + create_snapshot([explicit]).model_dump(exclude_none=True, by_alias=True) + ) + + +def test_drift_detects_kind_only_changes() -> None: + materialized = definition(default_kind="MATERIALIZED", default="fn:toDate(ts)") + normal = definition(default="fn:toDate(ts)") + result = compare_table_shape(normal, actual_table(materialized.columns)) + assert result is not None + assert result.changed_columns == ["day"] + assert len(plan_diff([normal], [materialized]).operations) == 1 + + +@pytest.mark.parametrize("kind", ["DEFAULT", "MATERIALIZED"]) +def test_remove_expression_with_type_change(kind: str) -> None: + operations = plan_diff( + [definition(default_kind=kind, default="fn:toDate(ts)")], [definition(type="Date32")] + ).operations + assert len(operations) == 1 + assert ( + operations[0].sql + == f"ALTER TABLE default.events MODIFY COLUMN `day` Date32, MODIFY COLUMN `day` REMOVE {kind};" + ) + + +@pytest.mark.parametrize("kind", ["ALIAS", "EPHEMERAL"]) +def test_storage_kind_changes_require_manual_migration(kind: str) -> None: + virtual = definition(default_kind=kind, default="fn:toDate(ts)") + with pytest.raises(ValueError, match="explicit manual migration"): + plan_diff([definition()], [virtual]) + with pytest.raises(ValueError, match="explicit manual migration"): + plan_diff([virtual], [definition()]) + + +def test_validation_and_alias() -> None: + col = ColumnDefinition.model_validate( + {"name": "day", "type": "Date", "defaultKind": "MATERIALIZED", "default": "fn:toDate(ts)"} + ) + assert col.default_kind == "MATERIALIZED" + for kind in ("MATERIALIZED", "ALIAS"): + assert any( + issue.code == "column_expression_required" + for issue in validate_definitions([definition(default_kind=kind)]) + ) + assert any( + issue.code == "column_expression_required" + for issue in validate_definitions([definition(default="fn:")]) + ) + + +def test_codegen_read_and_insert_models() -> None: + expected = definition(default_kind="MATERIALIZED", default="fn:toDate(ts)") + expected = expected.model_copy( + update={ + "columns": [ + *expected.columns, + ColumnDefinition(name="raw", type="String", defaultKind="EPHEMERAL"), + ColumnDefinition( + name="label", type="String", default_kind="ALIAS", default="fn:toString(day)" + ), + ] + } + ) + output = generate_type_artifacts(definitions=[expected]) + namespace: dict[str, Any] = {"__name__": "generated_columns"} + exec(output.content, namespace) + models = [value for key, value in namespace.items() if key.endswith(("Row", "RowInsert"))] + read = next(model for model in models if model.__name__.endswith("Row")) + insert = next(model for model in models if model.__name__.endswith("RowInsert")) + assert set(read.model_fields) == {"ts", "day", "label"} + assert set(insert.model_fields) == {"ts", "raw"} + + +def test_backfill_uses_implicit_insert_columns(tmp_path: Path) -> None: + + (tmp_path / "schema.py").write_text("""from chkit import table, materialized_view +result = table(database="default", name="events", engine="MergeTree()", order_by=["id"], primary_key=["id"], columns=[ + {"name": "id", "type": "UInt32"}, + {"name": "size", "type": "UInt64", "default_kind": "MATERIALIZED", "default": "fn:length(raw)"}, + {"name": "label", "type": "String", "default_kind": "ALIAS", "default": "fn:toString(size)"}, +]) +mv = materialized_view(database="default", name="mv", to={"database": "default", "name": "events"}, as_="SELECT id FROM default.source") +""") + strategy = _detect_backfill_strategy( + schema=["schema.py"], config_dir=tmp_path, database="default", table="events" + ) + assert strategy.target_columns == ["id"] + path = tmp_path / "ephemeral.py" + path.write_text( + (tmp_path / "schema.py") + .read_text() + .replace( + '{"name": "id", "type": "UInt32"}', + '{"name": "id", "type": "UInt32"}, {"name": "raw", "type": "String", "default_kind": "EPHEMERAL"}', + ) + ) + with pytest.raises(Exception, match="cannot reconstruct EPHEMERAL inputs"): + _detect_backfill_strategy( + schema=["ephemeral.py"], config_dir=tmp_path, database="default", table="events" + ) + + +def test_introspection_preserves_sql_literal_whitespace() -> None: + column = normalize_column_from_system_row( + SystemColumnRow( + database="default", + table="events", + name="label", + type="String", + position=1, + default_kind="ALIAS", + default_expression=" concat('a b', toString(id)) ", + ) + ) + assert column.default == "concat('a b', toString(id))" diff --git a/chkit_python/tests/test_column_expressions_e2e.py b/chkit_python/tests/test_column_expressions_e2e.py new file mode 100644 index 00000000..63ecb3d8 --- /dev/null +++ b/chkit_python/tests/test_column_expressions_e2e.py @@ -0,0 +1,113 @@ +"""Execute expression-column DDL, inserts, pull and ALTER against ClickHouse.""" + +from __future__ import annotations + +from typing import Any +from uuid import uuid4 + +import pytest + +from chkit import table +from chkit.cli.commands.drift_compare import compare_table_shape +from chkit.cli.commands.pull import _introspected_table_to_definition +from chkit.cli.commands.pull_render import render_schema_file +from chkit.clickhouse.introspect import ( + IntrospectedTable, + SystemColumnRow, + normalize_column_from_system_row, +) +from chkit.core.planner import plan_diff +from chkit.core.sql import to_create_sql + + +def test_expression_column_lifecycle(ch_client: Any) -> None: + client = ch_client._client + name = f"column_expr_py_{uuid4().hex}" + database = client.database + definition = table( + database=database, + name=name, + engine="MergeTree()", + order_by=["id"], + primary_key=["id"], + columns=[ + {"name": "id", "type": "UInt32"}, + {"name": "raw", "type": "String", "default_kind": "EPHEMERAL"}, + { + "name": "size", + "type": "UInt64", + "default_kind": "MATERIALIZED", + "default": "fn:length(raw)", + }, + { + "name": "label", + "type": "String", + "default_kind": "ALIAS", + "default": "fn:toString(size)", + }, + ], + ) + target = f"{database}.{name}" + try: + client.command(to_create_sql(definition)) + rows = client.query( + f"SELECT database, table, name, type, position, default_kind, default_expression FROM system.columns WHERE database='{database}' AND table='{name}' ORDER BY position" + ).named_results() + columns = [normalize_column_from_system_row(SystemColumnRow(**row)) for row in rows] + actual = IntrospectedTable( + database=database, + name=name, + columns=columns, + settings={}, + indexes=[], + projections=[], + engine="MergeTree()", + primary_key="id", + order_by="id", + ) + assert compare_table_shape(definition, actual) is None + pulled = _introspected_table_to_definition(actual) + assert pulled is not None + namespace: dict[str, Any] = {} + exec(render_schema_file([pulled]), namespace) + assert plan_diff([definition], namespace["definitions"]).operations == [] + client.command(f"INSERT INTO {target} (id, raw) VALUES (1, 'abc')") + assert client.query(f"SELECT size, label FROM {target}").result_rows == [(3, "3")] + with pytest.raises(Exception, match="MATERIALIZED"): + client.command(f"INSERT INTO {target} (id, size) VALUES (2, 10)") + with pytest.raises(Exception, match="raw"): + client.query(f"SELECT raw FROM {target}") + # Changing the expression leaves the already stored value intact. + changed = definition.model_copy( + update={ + "columns": [ + column.model_copy(update={"default": "fn:toUInt64(7)"}) + if column.name == "size" + else column + for column in definition.columns + ] + } + ) + for operation in plan_diff([definition], [changed]).operations: + assert "MATERIALIZE COLUMN" not in operation.sql + client.command(operation.sql) + assert client.query(f"SELECT size FROM {target}").result_rows == [(3,)] + client.command(f"INSERT INTO {target} (id, raw) VALUES (2, 'abcd')") + assert client.query(f"SELECT size FROM {target} ORDER BY id").result_rows == [(3,), (7,)] + plain = changed.model_copy( + update={ + "columns": [ + column.model_copy(update={"default": None, "default_kind": None}) + if column.name == "size" + else column + for column in changed.columns + ] + } + ) + for operation in plan_diff([changed], [plain]).operations: + client.command(operation.sql) + assert client.query( + f"SELECT default_kind FROM system.columns WHERE database='{database}' AND table='{name}' AND name='size'" + ).result_rows == [("",)] + finally: + client.command(f"DROP TABLE IF EXISTS {target} SYNC") diff --git a/chkit_python/tests/test_introspect.py b/chkit_python/tests/test_introspect.py index bb5e3c1e..05a68784 100644 --- a/chkit_python/tests/test_introspect.py +++ b/chkit_python/tests/test_introspect.py @@ -93,7 +93,7 @@ def test_normalize_column_picks_up_default_when_kind_is_default() -> None: assert column.default == "now()" -def test_normalize_column_ignores_default_when_kind_is_materialized() -> None: +def test_normalize_column_preserves_materialized_expression() -> None: row = SystemColumnRow( database="db", table="t", @@ -104,7 +104,8 @@ def test_normalize_column_ignores_default_when_kind_is_materialized() -> None: default_expression="now()", ) column = normalize_column_from_system_row(row) - assert column.default is None + assert column.default == "now()" + assert column.default_kind == "MATERIALIZED" def test_normalize_column_preserves_comment_and_codec() -> None: diff --git a/packages/cli/src/commands/drift/compare.ts b/packages/cli/src/commands/drift/compare.ts index 97adfc24..6e8c20ab 100644 --- a/packages/cli/src/commands/drift/compare.ts +++ b/packages/cli/src/commands/drift/compare.ts @@ -202,6 +202,7 @@ function normalizeColumnShape(column: ColumnDefinition): string { `type=${String(column.type).trim()}`, `nullable=${column.nullable ? '1' : '0'}`, `default=${normalizedDefault}`, + `defaultKind=${column.defaultKind ?? 'DEFAULT'}`, `comment=${column.comment?.trim() ?? ''}`, ] return parts.join('|') diff --git a/packages/cli/src/test/column-expressions.e2e.test.ts b/packages/cli/src/test/column-expressions.e2e.test.ts new file mode 100644 index 00000000..c112ca2b --- /dev/null +++ b/packages/cli/src/test/column-expressions.e2e.test.ts @@ -0,0 +1,232 @@ +import { expect, test } from 'bun:test' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createClient } from '@clickhouse/client' +import { planDiff, table, toCreateSQL, type TableDefinition } from '@chkit/core' +import { + normalizeColumnFromSystemRow, + type SystemColumnRow, +} from '@chkit/clickhouse' +import { compareTableShape } from '../commands/drift/compare.js' +import { renderSchemaFile } from '../../../plugin-pull/src/render-schema.js' +import { + generateTypeArtifacts, + generateIngestArtifacts, +} from '../../../plugin-codegen/src/index.js' +import { getRequiredEnv } from './e2e-testkit.js' + +test('column expressions survive create, pull, drift, inserts and ALTER on live ClickHouse', async () => { + const env = getRequiredEnv() + const client = createClient({ + url: env.clickhouseUrl, + username: env.clickhouseUser, + password: env.clickhousePassword, + database: env.clickhouseDatabase, + }) + const dir = await mkdtemp(join(tmpdir(), 'chkit-expression-pull-')) + const name = `column_expr_${Date.now()}_${Math.random().toString(16).slice(2)}` + let def = table({ + database: env.clickhouseDatabase, + name, + engine: 'MergeTree()', + primaryKey: ['id'], + orderBy: ['id'], + columns: [ + { name: 'id', type: 'UInt32' }, + { name: 'ts', type: 'DateTime' }, + { name: 'raw', type: 'String', defaultKind: 'EPHEMERAL' }, + { + name: 'day', + type: 'Date', + defaultKind: 'MATERIALIZED', + default: 'fn:toDate(ts)', + }, + { + name: 'label', + type: 'String', + defaultKind: 'ALIAS', + default: 'fn:toString(day)', + }, + { name: 'size', type: 'UInt64', default: 'fn:length(raw)' }, + ], + }) + const query = async (sql: string) => + (await client.query({ query: sql, format: 'JSONEachRow' })).json() + const columns = async () => + ( + await query( + `SELECT database, table, name, type, position, default_kind, default_expression FROM system.columns WHERE database='${def.database}' AND table='${name}' ORDER BY position`, + ) + ).map(normalizeColumnFromSystemRow) + const actual = async () => ({ + columns: await columns(), + settings: {}, + indexes: [], + projections: [], + engine: 'MergeTree()', + primaryKey: 'id', + orderBy: 'id', + }) + const migrate = async (next: TableDefinition) => { + const plan = planDiff([def], [next]) + for (const operation of plan.operations) { + expect(operation.sql).not.toContain('MATERIALIZE COLUMN') + await client.command({ query: operation.sql }) + } + def = next + expect(compareTableShape(def, await actual())).toBeNull() + } + try { + await client.command({ query: toCreateSQL(def) }) + expect(compareTableShape(def, await actual())).toBeNull() + const pulled = { + ...def, + columns: (await columns()).map((column) => ({ + ...column, + default: + typeof column.default === 'string' + ? `fn:${column.default}` + : column.default, + })), + } + const source = renderSchemaFile([pulled]).replace( + "'@chkit/core'", + JSON.stringify( + new URL('../../../core/src/index.ts', import.meta.url).href, + ), + ) + const path = join(dir, 'pulled.ts') + await writeFile(path, source) + const reloaded = (await import(path)).default + expect(planDiff(reloaded, [pulled]).operations).toEqual([]) + expect(toCreateSQL(reloaded[0])).toContain('MATERIALIZED toDate(ts)') + expect(toCreateSQL(reloaded[0])).toContain('`raw` String EPHEMERAL') + + await client.command({ + query: `INSERT INTO ${def.database}.${name} (id, ts, raw) VALUES (1, '2026-01-01 12:00:00', 'abc')`, + }) + expect( + await query( + `SELECT day, label, toString(size) AS size FROM ${def.database}.${name}`, + ), + ).toEqual([{ day: '2026-01-01', label: '2026-01-01', size: '3' }]) + await expect( + client.command({ + query: `INSERT INTO ${def.database}.${name} (id, ts, day) VALUES (2, '2026-01-01 12:00:00', '2000-01-01')`, + }), + ).rejects.toThrow() + await expect( + query(`SELECT raw FROM ${def.database}.${name}`), + ).rejects.toThrow() + + await migrate({ + ...def, + columns: def.columns.map((column) => + column.name === 'day' + ? { ...column, default: 'fn:addDays(toDate(ts), 1)' } + : column, + ), + }) + expect( + await query(`SELECT day FROM ${def.database}.${name} WHERE id=1`), + ).toEqual([{ day: '2026-01-01' }]) + await client.command({ + query: `INSERT INTO ${def.database}.${name} (id, ts, raw) VALUES (2, '2026-01-01 12:00:00', 'abcd')`, + }) + expect( + await query(`SELECT day FROM ${def.database}.${name} WHERE id=2`), + ).toEqual([{ day: '2026-01-02' }]) + await migrate({ + ...def, + columns: [ + ...def.columns, + { + name: 'copy', + type: 'UInt32', + defaultKind: 'MATERIALIZED', + default: 'fn:id', + }, + ], + }) + await migrate({ + ...def, + columns: def.columns.map((column) => + column.name === 'copy' ? { ...column, defaultKind: 'DEFAULT' } : column, + ), + }) + await migrate({ + ...def, + columns: def.columns.map((column) => + column.name === 'copy' || column.name === 'day' + ? { name: column.name, type: column.type } + : column, + ), + }) + await migrate({ + ...def, + columns: def.columns.map((column) => + column.name === 'raw' ? { ...column, default: 'seed' } : column, + ), + }) + await migrate({ + ...def, + columns: def.columns.map((column) => + column.name === 'raw' ? { ...column, default: undefined } : column, + ), + }) + } finally { + await client.command({ + query: `DROP TABLE IF EXISTS ${def.database}.${name} SYNC`, + }) + await client.close() + await rm(dir, { recursive: true, force: true }) + } +}, 30_000) + +test('generated ingest helpers use insert shapes, while rows exclude ephemeral inputs', () => { + const def = table({ + database: 'default', + name: 'events', + engine: 'MergeTree()', + primaryKey: ['id'], + orderBy: ['id'], + columns: [ + { name: 'id', type: 'UInt32' }, + { name: 'raw', type: 'String', defaultKind: 'EPHEMERAL' }, + { + name: 'computed', + type: 'UInt32', + defaultKind: 'MATERIALIZED', + default: 'fn:length(raw)', + }, + { + name: 'label', + type: 'String', + defaultKind: 'ALIAS', + default: 'fn:toString(id)', + }, + ], + }) + const types = generateTypeArtifacts({ + definitions: [def], + options: { emitZod: true }, + }).content + const read = types.split('export type DefaultEventsRow = {')[1]?.split('}')[0] + const insert = types + .split('export type DefaultEventsRowInsert = {')[1] + ?.split('}')[0] + expect(read).toContain('computed: number') + expect(read).toContain('label: string') + expect(read).not.toContain('raw:') + expect(insert).toContain('raw: string') + expect(insert).not.toContain('computed:') + expect(insert).not.toContain('label:') + const ingest = generateIngestArtifacts({ + definitions: [def], + options: { emitZod: true }, + }).content + expect(ingest).toContain('rows: DefaultEventsRowInsert[]') + expect(ingest).toContain('DefaultEventsRowInsertSchema.parse(row)') + expect(ingest).toContain('function ingestDefaultEvents(') +}) diff --git a/packages/clickhouse/src/column-expressions.test.ts b/packages/clickhouse/src/column-expressions.test.ts new file mode 100644 index 00000000..29d2c9c3 --- /dev/null +++ b/packages/clickhouse/src/column-expressions.test.ts @@ -0,0 +1,51 @@ +import { expect, test } from 'bun:test' +import { normalizeColumnFromSystemRow } from './index.js' + +for (const kind of ['DEFAULT', 'MATERIALIZED', 'ALIAS', 'EPHEMERAL'] as const) { + test(`introspection preserves ${kind} expressions including literal whitespace`, () => { + const column = normalizeColumnFromSystemRow({ + database: 'default', + table: 'events', + name: 'label', + type: 'String', + position: 1, + default_kind: kind, + default_expression: " concat('a b', toString(id)) ", + }) + expect(column.default).toBe("concat('a b', toString(id))") + expect(column.defaultKind).toBe(kind === 'DEFAULT' ? undefined : kind) + }) +} + +test('expressionless nullable EPHEMERAL columns preserve kind without synthetic defaults', () => { + const column = normalizeColumnFromSystemRow({ + database: 'default', + table: 'events', + name: 'raw', + type: 'Nullable(String)', + position: 1, + default_kind: 'EPHEMERAL', + default_expression: "defaultValueOfTypeName('Nullable(String)')", + }) + expect(column).toMatchObject({ + name: 'raw', + type: 'String', + nullable: true, + defaultKind: 'EPHEMERAL', + }) + expect(column.default).toBeUndefined() +}) + +test('unknown expression kinds fail explicitly instead of losing metadata', () => { + expect(() => + normalizeColumnFromSystemRow({ + database: 'default', + table: 'events', + name: 'x', + type: 'String', + position: 1, + default_kind: 'FUTURE_KIND', + default_expression: 'someExpression()', + }), + ).toThrow('Unsupported column default kind') +}) diff --git a/packages/clickhouse/src/index.ts b/packages/clickhouse/src/index.ts index 29c14dc5..8666ff42 100644 --- a/packages/clickhouse/src/index.ts +++ b/packages/clickhouse/src/index.ts @@ -192,8 +192,18 @@ export function normalizeColumnFromSystemRow( const type = nullableMatch?.[1] ? nullableMatch[1] : row.type const nullable = Boolean(nullableMatch?.[1]) let defaultValue: ColumnDefinition['default'] | undefined - if (row.default_expression && row.default_kind === 'DEFAULT') { - defaultValue = normalizeSQLFragment(row.default_expression) + const defaultKind = row.default_kind + if (defaultKind && !['DEFAULT', 'MATERIALIZED', 'ALIAS', 'EPHEMERAL'].includes(defaultKind)) { + throw new Error(`Unsupported column default kind: ${defaultKind}`) + } + if (row.default_expression && defaultKind) { + // Preserve whitespace inside SQL string literals when pulling expressions. + defaultValue = row.default_expression.trim() + } + // ClickHouse synthesizes this expression for a bare EPHEMERAL column. + const implicitEphemeralDefault = `defaultValueOfTypeName('${row.type.replace(/'/g, "\\'")}')` + if (defaultKind === 'EPHEMERAL' && defaultValue === implicitEphemeralDefault) { + defaultValue = undefined } const codecSteps = parseCodec(row.compression_codec) return { @@ -201,6 +211,9 @@ export function normalizeColumnFromSystemRow( type, nullable: nullable || undefined, default: defaultValue, + defaultKind: defaultKind && defaultKind !== 'DEFAULT' + ? defaultKind as ColumnDefinition['defaultKind'] + : undefined, comment: row.comment?.trim() || undefined, codec: codecSteps, } diff --git a/packages/core/src/canonical.ts b/packages/core/src/canonical.ts index 46313b9d..84ceb2aa 100644 --- a/packages/core/src/canonical.ts +++ b/packages/core/src/canonical.ts @@ -28,8 +28,10 @@ function sortKind(kind: SchemaDefinition['kind']): number { } function canonicalizeColumn(column: ColumnDefinition): ColumnDefinition { + const { defaultKind, ...rest } = column return { - ...column, + ...rest, + defaultKind: defaultKind === 'DEFAULT' ? undefined : defaultKind, name: column.name.trim(), renamedFrom: column.renamedFrom?.trim(), type: typeof column.type === 'string' ? column.type.trim() : column.type, diff --git a/packages/core/src/column-expressions.test.ts b/packages/core/src/column-expressions.test.ts new file mode 100644 index 00000000..f39b650e --- /dev/null +++ b/packages/core/src/column-expressions.test.ts @@ -0,0 +1,127 @@ +import { describe, expect, test } from 'bun:test' +import { + createSnapshot, + planDiff, + table, + toCreateSQL, + validateDefinitions, + type ColumnDefinition, +} from './index.js' + +const definition = (column: Partial = {}) => + table({ + database: 'default', + name: 'events', + engine: 'MergeTree()', + primaryKey: ['ts'], + orderBy: ['ts'], + columns: [ + { name: 'ts', type: 'DateTime' }, + { name: 'day', type: 'Date', ...column }, + ], + }) + +describe('column expressions', () => { + for (const defaultKind of [ + 'DEFAULT', + 'MATERIALIZED', + 'ALIAS', + 'EPHEMERAL', + ] as const) { + test(`renders ${defaultKind} expressions and retains literal quoting`, () => { + const def = definition({ defaultKind, default: 'fn:toDate(ts)' }) + expect(toCreateSQL(def)).toContain( + `\`day\` Date ${defaultKind} toDate(ts)`, + ) + expect( + toCreateSQL( + definition({ defaultKind, type: 'String', default: "it's literal" }), + ), + ).toContain(`${defaultKind} 'it''s literal'`) + expect(planDiff([def], [def]).operations).toEqual([]) + }) + } + + test('supports EPHEMERAL without an expression', () => { + expect(toCreateSQL(definition({ defaultKind: 'EPHEMERAL' }))).toContain( + '`day` Date EPHEMERAL', + ) + }) + + test('keeps legacy and explicit DEFAULT snapshots equivalent', () => { + for (const value of [undefined, 0, false, '', 'fn:toDate(ts)']) { + const old = definition({ default: value }) + const explicit = definition({ defaultKind: 'DEFAULT', default: value }) + const legacy = JSON.parse(JSON.stringify(createSnapshot([old]))) + expect(planDiff(legacy.definitions, [explicit]).operations).toEqual([]) + expect(JSON.stringify(createSnapshot([explicit]))).not.toContain( + 'defaultKind', + ) + } + }) + + test('detects kind-only and expression changes', () => { + const old = definition({ default: 'fn:toDate(ts)' }) + const materialized = definition({ + defaultKind: 'MATERIALIZED', + default: 'fn:toDate(ts)', + }) + const plan = planDiff([old], [materialized]) + expect(plan.operations).toHaveLength(1) + expect(plan.operations[0]?.sql).toContain('MATERIALIZED toDate(ts)') + expect(plan.operations[0]?.risk).toBe('caution') + expect( + planDiff( + [materialized], + [definition({ defaultKind: 'MATERIALIZED', default: 'fn:today()' })], + ).operations[0]?.sql, + ).toContain('MATERIALIZED today()') + }) + + for (const defaultKind of ['DEFAULT', 'MATERIALIZED'] as const) { + test(`explicitly removes ${defaultKind}, including simultaneous type changes`, () => { + const plan = planDiff( + [definition({ defaultKind, default: 'fn:toDate(ts)' })], + [definition({ type: 'Date32' })], + ) + expect(plan.operations[0]?.sql).toBe( + `ALTER TABLE default.events MODIFY COLUMN \`day\` Date32, MODIFY COLUMN \`day\` REMOVE ${defaultKind};`, + ) + expect(plan.operations).toHaveLength(1) + }) + } + + test('rejects automatic storage-kind changes in both directions', () => { + for (const defaultKind of ['ALIAS', 'EPHEMERAL'] as const) { + const virtual = definition({ defaultKind, default: 'fn:toDate(ts)' }) + expect(() => planDiff([definition()], [virtual])).toThrow( + 'explicit manual migration', + ) + expect(() => planDiff([virtual], [definition()])).toThrow( + 'explicit manual migration', + ) + } + }) + + test('validates kind and missing/empty expressions', () => { + for (const defaultKind of ['MATERIALIZED', 'ALIAS'] as const) { + expect( + validateDefinitions([definition({ defaultKind })]).map( + (issue) => issue.code, + ), + ).toContain('column_expression_required') + } + expect( + validateDefinitions([definition({ default: 'fn: ' })]).map( + (issue) => issue.code, + ), + ).toContain('column_expression_required') + expect( + validateDefinitions([ + definition({ + defaultKind: 'invalid' as ColumnDefinition['defaultKind'], + }), + ]).map((issue) => issue.code), + ).toContain('column_default_kind_invalid') + }) +}) diff --git a/packages/core/src/model-types.ts b/packages/core/src/model-types.ts index c0e9f438..e5d0450b 100644 --- a/packages/core/src/model-types.ts +++ b/packages/core/src/model-types.ts @@ -52,12 +52,16 @@ export type ColumnCodec = GeneralColumnCodec | PreprocessingColumnCodec | RawCol /** Single codec or a chain (preprocessors then exactly one general codec). */ export type ColumnCodecSpec = ColumnCodec | ColumnCodec[] +export type ColumnDefaultKind = 'DEFAULT' | 'MATERIALIZED' | 'ALIAS' | 'EPHEMERAL' + export interface ColumnDefinition { name: string type: PrimitiveColumnType | string renamedFrom?: string nullable?: boolean + /** Strings are literals; prefix SQL expressions with `fn:`. */ default?: string | number | boolean + defaultKind?: ColumnDefaultKind comment?: string codec?: ColumnCodecSpec } @@ -426,6 +430,8 @@ export type ValidationIssueCode = | 'codec_chain_must_end_with_general' | 'codec_chain_multiple_general' | 'codec_chain_empty' + | 'column_default_kind_invalid' + | 'column_expression_required' | 'dictionary_missing_primary_key' | 'dictionary_primary_key_missing_attribute' | 'dictionary_missing_source' diff --git a/packages/core/src/planner.ts b/packages/core/src/planner.ts index 5d180f2c..a2a54a3f 100644 --- a/packages/core/src/planner.ts +++ b/packages/core/src/planner.ts @@ -409,9 +409,14 @@ function diffTables(oldDef: TableDefinition, newDef: TableDefinition): TableDiff }) } for (const { name, oldItem, newItem } of columnDiff.changed) { + const oldKind = oldItem.defaultKind ?? 'DEFAULT' + const newKind = newItem.defaultKind ?? 'DEFAULT' + if (oldKind !== newKind && [oldKind, newKind].some((kind) => kind === 'ALIAS' || kind === 'EPHEMERAL')) { + throw new Error(`Cannot automatically change column ${newDef.database}.${newDef.name}.${name} from ${oldKind} to ${newKind}; use an explicit manual migration for storage-kind changes`) + } const sql = isCodecRemoval(oldItem, newItem) ? renderAlterRemoveCodec(newDef, name) - : renderAlterModifyColumn(newDef, newItem) + : renderAlterModifyColumn(newDef, newItem, oldItem) ops.push( { type: 'alter_table_modify_column', key: `table:${newDef.database}.${newDef.name}:column:${name}`, diff --git a/packages/core/src/sql.ts b/packages/core/src/sql.ts index 8ac3d364..7fe0190c 100644 --- a/packages/core/src/sql.ts +++ b/packages/core/src/sql.ts @@ -27,7 +27,8 @@ function renderDefault(value: string | number | boolean): string { function renderColumn(col: ColumnDefinition): string { let out = `\`${col.name}\` ${col.nullable ? `Nullable(${col.type})` : col.type}` - if (col.default !== undefined) out += ` DEFAULT ${renderDefault(col.default)}` + if (col.default !== undefined) out += ` ${col.defaultKind ?? 'DEFAULT'} ${renderDefault(col.default)}` + else if (col.defaultKind === 'EPHEMERAL') out += ' EPHEMERAL' if (col.comment) out += ` COMMENT '${col.comment.replace(/'/g, "''")}'` if (col.codec) out += ` ${renderCodec(col.codec)}` return out @@ -211,8 +212,15 @@ export function renderAlterAddColumn(def: TableDefinition, column: ColumnDefinit return `ALTER TABLE ${def.database}.${def.name} ADD COLUMN IF NOT EXISTS ${renderColumn(column)};` } -export function renderAlterModifyColumn(def: TableDefinition, column: ColumnDefinition): string { - return `ALTER TABLE ${def.database}.${def.name} MODIFY COLUMN ${renderColumn(column)};` +export function renderAlterModifyColumn( + def: TableDefinition, + column: ColumnDefinition, + previous?: ColumnDefinition +): string { + const remove = previous?.default !== undefined && column.default === undefined && column.defaultKind !== 'EPHEMERAL' + ? `, MODIFY COLUMN \`${column.name}\` REMOVE ${previous.defaultKind ?? 'DEFAULT'}` + : '' + return `ALTER TABLE ${def.database}.${def.name} MODIFY COLUMN ${renderColumn(column)}${remove};` } export function renderAlterDropColumn(def: TableDefinition, columnName: string): string { diff --git a/packages/core/src/validate.ts b/packages/core/src/validate.ts index ecc2b68b..db0dc8ff 100644 --- a/packages/core/src/validate.ts +++ b/packages/core/src/validate.ts @@ -123,6 +123,21 @@ function validateTableDefinition(def: TableDefinition, issues: ValidationIssue[] } columnSeen.add(column.name) columnSet.add(column.name) + const kind = column.defaultKind + if (kind !== undefined && !['DEFAULT', 'MATERIALIZED', 'ALIAS', 'EPHEMERAL'].includes(kind)) { + pushValidationIssue( + issues, def, 'column_default_kind_invalid', `Invalid defaultKind on column "${column.name}"` + ) + } + const missingExpression = (kind === 'MATERIALIZED' || kind === 'ALIAS') && column.default === undefined + const emptyExpression = typeof column.default === 'string' + && column.default.startsWith('fn:') && !column.default.slice(3).trim() + if (missingExpression || emptyExpression) { + pushValidationIssue( + issues, def, 'column_expression_required', + `Column "${column.name}" requires a non-empty expression; use fn: for SQL expressions` + ) + } validateColumnCodec(def, column, issues) } diff --git a/packages/plugin-backfill/src/planner.test.ts b/packages/plugin-backfill/src/planner.test.ts index a9717f5d..12ec4926 100644 --- a/packages/plugin-backfill/src/planner.test.ts +++ b/packages/plugin-backfill/src/planner.test.ts @@ -548,3 +548,33 @@ export const api_mv = { } }) }) + +test('MV replay omits computed columns and rejects unrecoverable ephemeral inputs', async () => { + const dir = await mkdtemp(join(tmpdir(), 'chkit-backfill-expressions-')) + try { + await writeFile(join(dir, 'schema.ts'), ` + export const target = { kind: 'table', database: 'app', name: 'events_agg', engine: 'MergeTree()', + primaryKey: ['event_time'], orderBy: ['event_time'], columns: [ + { name: 'event_time', type: 'DateTime' }, { name: 'count', type: 'UInt64' }, + { name: 'day', type: 'Date', defaultKind: 'MATERIALIZED', default: 'fn:toDate(event_time)' }, + { name: 'label', type: 'String', defaultKind: 'ALIAS', default: 'fn:toString(count)' } + ] } + export const mv = { kind: 'materialized_view', database: 'app', name: 'events_mv', + to: { database: 'app', name: 'events_agg' }, as: 'SELECT event_time, count() AS count FROM app.events GROUP BY event_time' } + `) + const output = await buildBackfillPlan({ opts: PlanSchema.parse({ target: 'app.events_agg' }), + configPath: join(dir, 'clickhouse.config.ts'), config: resolveConfig({ schema: './schema.ts', metaDir: './chkit/meta' }), + clickhouseQuery: createMockQuery(), + }) + expect(output.plan.execution.targetColumns).toEqual(['event_time', 'count']) + const ephemeralPath = join(dir, 'ephemeral.ts') + const source = await readFile(join(dir, 'schema.ts'), 'utf8') + await writeFile(ephemeralPath, source.replace("{ name: 'count', type: 'UInt64' }", "{ name: 'raw', type: 'String', defaultKind: 'EPHEMERAL' }")) + await expect(buildBackfillPlan({ opts: PlanSchema.parse({ target: 'app.events_agg' }), + configPath: join(dir, 'clickhouse.config.ts'), config: resolveConfig({ schema: './ephemeral.ts', metaDir: './chkit/meta' }), + clickhouseQuery: createMockQuery(), + })).rejects.toThrow('cannot reconstruct EPHEMERAL inputs') + } finally { + await rm(dir, { recursive: true, force: true }) + } +}) diff --git a/packages/plugin-backfill/src/planner.ts b/packages/plugin-backfill/src/planner.ts index e5d40c2f..b6a8ca5b 100644 --- a/packages/plugin-backfill/src/planner.ts +++ b/packages/plugin-backfill/src/planner.ts @@ -38,7 +38,6 @@ async function detectBackfillStrategy(input: { try { const definitions = await loadSchemaDefinitions(input.schema, { cwd: input.configDir }) const mvs = findMvsForTarget(definitions, input.database, input.table) - if (mvs.length === 0) return { mvs: [] } const tableDef = definitions.find( (definition) => @@ -46,12 +45,21 @@ async function detectBackfillStrategy(input: { definition.database === input.database && definition.name === input.table ) + if (tableDef?.kind === 'table' && tableDef.columns.some((column) => column.defaultKind === 'EPHEMERAL')) { + throw new BackfillConfigError('Automatic backfill cannot reconstruct EPHEMERAL inputs; use an explicit INSERT with an input column mapping.') + } + if (mvs.length === 0) return { mvs: [] } return { mvs, mvReplayQueries: mvs.map((mv) => mv.as), - targetColumns: tableDef?.kind === 'table' ? tableDef.columns.map((column) => column.name) : undefined, + targetColumns: tableDef?.kind === 'table' + ? tableDef.columns + .filter((column) => !column.defaultKind || column.defaultKind === 'DEFAULT') + .map((column) => column.name) + : undefined, } - } catch { + } catch (error) { + if (error instanceof BackfillConfigError) throw error // Schema load failed, fall back to direct copy. return { mvs: [] } } diff --git a/packages/plugin-codegen/src/generators/ingest-artifacts.ts b/packages/plugin-codegen/src/generators/ingest-artifacts.ts index b15d8586..aa25465a 100644 --- a/packages/plugin-codegen/src/generators/ingest-artifacts.ts +++ b/packages/plugin-codegen/src/generators/ingest-artifacts.ts @@ -11,7 +11,7 @@ import type { } from '../types.js' import { normalizeCodegenOptions } from '../options.js' import { resolveTableNames } from '../naming.js' -import { renderHeader } from './shared.js' +import { insertTypeName, renderHeader } from './shared.js' function computeRelativeImportPath(fromFile: string, toFile: string): string { const fromDir = dirname(fromFile) @@ -34,21 +34,22 @@ function renderIngestFunction( ): string[] { const funcName = `ingest${stripRowSuffix(interfaceName)}` const tableFqn = `${table.database}.${table.name}` + const inputType = insertTypeName(table, interfaceName) const lines: string[] = [] if (emitZod) { lines.push(`export async function ${funcName}(`) lines.push(` ingestor: Ingestor,`) - lines.push(` rows: ${interfaceName}[],`) + lines.push(` rows: ${inputType}[],`) lines.push(` options?: IngestOptions`) lines.push(`): Promise {`) - lines.push(` const data = options?.validate ? rows.map(row => ${interfaceName}Schema.parse(row)) : rows`) + lines.push(` const data = options?.validate ? rows.map(row => ${inputType}Schema.parse(row)) : rows`) lines.push(` await ingestor.insert({ table: '${tableFqn}', values: data, compressed: options?.compressed ?? true })`) lines.push(`}`) } else { lines.push(`export async function ${funcName}(`) lines.push(` ingestor: Ingestor,`) - lines.push(` rows: ${interfaceName}[],`) + lines.push(` rows: ${inputType}[],`) lines.push(` options?: IngestOptions`) lines.push(`): Promise {`) lines.push(` await ingestor.insert({ table: '${tableFqn}', values: rows, compressed: options?.compressed ?? true })`) @@ -76,9 +77,12 @@ export function generateIngestArtifacts( const typeImports: string[] = [] const valueImports: string[] = [] for (const entry of resolved) { - typeImports.push(entry.interfaceName) + const name = entry.definition.kind === 'table' + ? insertTypeName(entry.definition, entry.interfaceName) + : entry.interfaceName + typeImports.push(name) if (normalized.emitZod) { - valueImports.push(`${entry.interfaceName}Schema`) + valueImports.push(`${name}Schema`) } } diff --git a/packages/plugin-codegen/src/generators/shared.ts b/packages/plugin-codegen/src/generators/shared.ts index 8f8751ac..b3688553 100644 --- a/packages/plugin-codegen/src/generators/shared.ts +++ b/packages/plugin-codegen/src/generators/shared.ts @@ -1,3 +1,5 @@ +import type { TableDefinition } from '@chkit/core' + export function renderHeader(toolVersion: string): string[] { const lines = [ '// This file is auto-generated by chkit codegen — do not edit manually.', @@ -5,3 +7,10 @@ export function renderHeader(toolVersion: string): string[] { lines.push(`// chkit-codegen-version: ${toolVersion}`) return lines } + +/** Preserve existing names for ordinary tables; special columns need an insert shape. */ +export function insertTypeName(table: TableDefinition, rowType: string): string { + return table.columns.some((column) => column.defaultKind && column.defaultKind !== 'DEFAULT') + ? `${rowType}Insert` + : rowType +} diff --git a/packages/plugin-codegen/src/generators/type-artifacts.ts b/packages/plugin-codegen/src/generators/type-artifacts.ts index 90eaece0..5eb9017c 100644 --- a/packages/plugin-codegen/src/generators/type-artifacts.ts +++ b/packages/plugin-codegen/src/generators/type-artifacts.ts @@ -17,7 +17,7 @@ import type { import { UnsupportedTypeError } from '../errors.js' import { normalizeCodegenOptions } from '../options.js' import { renderPropertyName, resolveTableNames } from '../naming.js' -import { renderHeader } from './shared.js' +import { insertTypeName, renderHeader } from './shared.js' const LARGE_INTEGER_TYPES = new Set([ 'Int64', @@ -268,7 +268,18 @@ function renderTableInterface( interfaceName: string, options: Required ): { lines: string[]; findings: CodegenFinding[] } { - return renderFieldsInterface(table.columns, interfaceName, `${table.database}.${table.name}`, options) + const path = `${table.database}.${table.name}` + const read = renderFieldsInterface( + table.columns.filter((column) => column.defaultKind !== 'EPHEMERAL'), + interfaceName, path, options + ) + const insertName = insertTypeName(table, interfaceName) + if (insertName === interfaceName) return read + const insert = renderFieldsInterface( + table.columns.filter((column) => column.defaultKind !== 'MATERIALIZED' && column.defaultKind !== 'ALIAS'), + insertName, path, options + ) + return { lines: [...read.lines, '', ...insert.lines], findings: [...read.findings, ...insert.findings] } } function renderDictionaryInterface( diff --git a/packages/plugin-pull/src/render-schema.ts b/packages/plugin-pull/src/render-schema.ts index 3afaa063..1b85fb43 100644 --- a/packages/plugin-pull/src/render-schema.ts +++ b/packages/plugin-pull/src/render-schema.ts @@ -193,6 +193,7 @@ function renderColumn(column: ColumnDefinition): string { `type: ${renderString(column.type)}`, ] if (column.nullable) parts.push('nullable: true') + if (column.defaultKind && column.defaultKind !== 'DEFAULT') parts.push(`defaultKind: ${renderString(column.defaultKind)}`) if (column.default !== undefined) parts.push(`default: ${renderLiteral(column.default)}`) if (column.comment) parts.push(`comment: ${renderString(column.comment)}`) if (column.codec) parts.push(`codec: ${renderCodecSource(column.codec)}`) From 134c77e4804cc1b1bc5bb291c9d7cec878a68ecc Mon Sep 17 00:00:00 2001 From: KeKs0r Date: Sat, 26 Sep 2026 19:00:16 -0700 Subject: [PATCH 2/5] =?UTF-8?q?=E2=9C=85=20Run=20column=20lifecycle=20test?= =?UTF-8?q?s=20in=20ClickHouse=20CI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vex-Session: session-3bdf6c82f39781ebe0b0ef8f --- .github/workflows/ci.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dca86cbb..394dc7c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -118,6 +118,11 @@ jobs: - name: Test Python text indexes working-directory: chkit_python run: python -m pytest tests/test_text_index.py tests/test_text_index_e2e.py -q + - name: Test TypeScript column expressions + run: bun test packages/core/src/column-expressions.test.ts packages/clickhouse/src/column-expressions.test.ts packages/cli/src/test/column-expressions.e2e.test.ts + - name: Test Python column expressions + working-directory: chkit_python + run: python -m pytest tests/test_column_expressions.py tests/test_column_expressions_e2e.py -q obsessiondb: runs-on: blacksmith-8vcpu-ubuntu-2404 From de1c652d445768e6524b73a3f7ff798d139ec46a Mon Sep 17 00:00:00 2001 From: KeKs0r Date: Sat, 26 Sep 2026 23:55:54 -0700 Subject: [PATCH 3/5] =?UTF-8?q?=F0=9F=90=9B=20Close=20column=20expression?= =?UTF-8?q?=20safety=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vex-Session: session-3bdf6c82f39781ebe0b0ef8f --- .changeset/column-expression-kinds.md | 3 + .github/workflows/ci.yml | 2 +- apps/docs/src/content/docs/cli/generate.md | 34 +++++ .../src/content/docs/schema/dsl-reference.mdx | 45 ++++--- chkit_python/CHANGELOG.md | 5 + .../src/chkit/cli/commands/drift_compare.py | 32 ++--- .../src/chkit/cli/commands/generate.py | 110 ++++++++++++---- .../chkit/cli/commands/generate_reconcile.py | 56 +++++++++ chkit_python/src/chkit/cli/migration_store.py | 4 +- chkit_python/src/chkit/core/model.py | 1 + chkit_python/src/chkit/core/planner.py | 62 ++++----- chkit_python/src/chkit/core/sql.py | 2 +- .../src/chkit_plugin_backfill/planner.py | 34 ++++- .../src/chkit_plugin_backfill/plugin.py | 27 ++-- .../chkit_plugin_codegen/type_artifacts.py | 9 +- chkit_python/tests/test_backfill_planner.py | 4 + chkit_python/tests/test_column_expressions.py | 80 +++++++++++- .../tests/test_column_expressions_e2e.py | 81 ++++++++++++ packages/cli/src/commands/drift/compare.ts | 26 ++-- packages/cli/src/commands/generate/command.ts | 40 +++++- .../cli/src/commands/generate/reconcile.ts | 56 +++++++++ .../src/test/column-expression-safety.test.ts | 103 +++++++++++++++ .../src/test/column-expressions.e2e.test.ts | 118 +++++++++++++++++- packages/codegen/src/index.ts | 6 +- packages/core/src/index.ts | 4 +- packages/core/src/model-types.ts | 1 + packages/core/src/planner.ts | 5 +- packages/core/src/sql-normalizer.ts | 6 + packages/core/src/sql.ts | 4 +- packages/plugin-backfill/src/planner.test.ts | 21 ++++ packages/plugin-backfill/src/planner.ts | 26 +++- packages/plugin-backfill/src/plugin.ts | 5 +- .../src/generators/type-artifacts.ts | 11 +- 33 files changed, 879 insertions(+), 144 deletions(-) create mode 100644 chkit_python/src/chkit/cli/commands/generate_reconcile.py create mode 100644 packages/cli/src/commands/generate/reconcile.ts create mode 100644 packages/cli/src/test/column-expression-safety.test.ts diff --git a/.changeset/column-expression-kinds.md b/.changeset/column-expression-kinds.md index 223248c4..221eb688 100644 --- a/.changeset/column-expression-kinds.md +++ b/.changeset/column-expression-kinds.md @@ -1,5 +1,6 @@ --- "@chkit/core": minor +"@chkit/codegen": patch "@chkit/clickhouse": minor "chkit": minor "@chkit/plugin-pull": minor @@ -10,3 +11,5 @@ Support `MATERIALIZED`, `ALIAS`, and `EPHEMERAL` column expressions with `defaultKind`, preserving kinds through SQL rendering, pull, snapshots, and drift. Keep existing defaults and snapshots stable; use the existing `fn:` prefix for SQL expressions and allow expressionless `EPHEMERAL` columns. Generate separate row and insert shapes for tables with special column kinds. Exclude generated columns from automatic backfill insert projections and reject automatic backfills that cannot reconstruct ephemeral inputs. Emit explicit removal of stored expressions, require manual migrations for storage-kind conversions involving `ALIAS` or `EPHEMERAL`, and never automatically rewrite historical materialized values. + +Compare defaults with quote-aware SQL tokens, preserving literal whitespace and distinguishing SQL expressions from string literals. Generate `Row` for default `SELECT *`, `RowExplicit` for all readable columns, and `RowInsert` for writes. Verify live column kinds during backfill planning and local execution, fail closed on unavailable metadata, and provide `generate --reconcile --table` for verified snapshot adoption after manual column migrations. Surface historical-value warnings in CLI output and migration files. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 394dc7c3..1b5eebad 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -119,7 +119,7 @@ jobs: working-directory: chkit_python run: python -m pytest tests/test_text_index.py tests/test_text_index_e2e.py -q - name: Test TypeScript column expressions - run: bun test packages/core/src/column-expressions.test.ts packages/clickhouse/src/column-expressions.test.ts packages/cli/src/test/column-expressions.e2e.test.ts + run: bun test packages/cli/src/test/column-expression-safety.test.ts packages/core/src/column-expressions.test.ts packages/clickhouse/src/column-expressions.test.ts packages/cli/src/test/column-expressions.e2e.test.ts - name: Test Python column expressions working-directory: chkit_python run: python -m pytest tests/test_column_expressions.py tests/test_column_expressions_e2e.py -q diff --git a/apps/docs/src/content/docs/cli/generate.md b/apps/docs/src/content/docs/cli/generate.md index ba0ce188..471f9354 100644 --- a/apps/docs/src/content/docs/cli/generate.md +++ b/apps/docs/src/content/docs/cli/generate.md @@ -24,6 +24,7 @@ chkit generate [flags] | `--rename-dictionary ` | string | — | Explicit dictionary rename: `old_db.old_dict=new_db.new_dict` | | `--table ` | string | — | Scope operations to matching tables | | `--dryrun` | boolean | `false` | Print the plan without writing any files | +| `--reconcile` | boolean | `false` | Verify live column expressions and reconcile their snapshot; requires `--table` | | `--empty` | boolean | `false` | Scaffold a blank manual migration without diffing the schema | Global flags documented on [CLI Overview](/cli/overview/#global-flags). @@ -94,6 +95,39 @@ The stub carries the standard migration header (with `operation-count: 0`) plus `chkit migrate` picks the file up like any other migration and applies it in filename order. Write your SQL into the stub *before* applying it — editing a migration after it has run triggers a checksum mismatch. +### Reconcile manual column changes + +When changing a column to or from `ALIAS` or `EPHEMERAL`, write and review an explicit +migration that handles the storage change. Preserve any stored values you need +before converting a column to a non-stored kind. + +1. Update the column's expression/kind in the schema. Keep other edits to that table + for a separate migration. +2. Create a manual SQL file in the configured `migrationsDir`. In TypeScript, + `chkit generate --empty --name convert_column` creates the stub. In Python, + create a timestamped `.sql` file there directly. Include any cluster clauses + required by your deployment; manual SQL runs verbatim. +3. Review and apply the migration through `chkit migrate --apply`. +4. Preview snapshot reconciliation with + `chkit generate --reconcile --table analytics.events --dryrun`. +5. Run `chkit generate --reconcile --table analytics.events`, then regenerate models + with `chkit codegen` if you use the codegen plugin. Commit the schema, migration, + snapshot, and regenerated models together. + +`--reconcile` works in both languages and requires a live ClickHouse connection, +`--table`, and an existing snapshot. It verifies the selected tables against the +current schema before writing anything. Only column expression/kind metadata is +adopted: other schema changes are rejected, and unselected snapshot objects are +preserved. It generates no SQL and cannot be combined with rename flags or +`--empty`. `--dryrun` verifies the live schema without updating the snapshot. + +A missing table, metadata query failure, or schema mismatch leaves the snapshot +unchanged. After successful reconciliation, another `generate --dryrun` should show +no operations for the converted column. This verifies schema metadata, not the +correctness or completeness of your data conversion; verify the data before +reconciling. Run reconciliation against the intended migration environment and +avoid concurrent DDL during the check. + ### Codegen integration If the codegen plugin is configured with `runOnGenerate: true` (the default), `chkit generate` automatically runs codegen after writing migration artifacts. A codegen failure causes `generate` to fail. diff --git a/apps/docs/src/content/docs/schema/dsl-reference.mdx b/apps/docs/src/content/docs/schema/dsl-reference.mdx index e50def24..63b5c64e 100644 --- a/apps/docs/src/content/docs/schema/dsl-reference.mdx +++ b/apps/docs/src/content/docs/schema/dsl-reference.mdx @@ -328,22 +328,35 @@ excludes all three special kinds from `SELECT *`. base type in `type`; do not embed `MATERIALIZED ...` in the type string. Changing a stored expression emits `MODIFY COLUMN`, without rewriting historical -values. Use a separately reviewed `ALTER TABLE ... MATERIALIZE COLUMN ...` if you -need that rewrite. Removing a `DEFAULT` or `MATERIALIZED` expression emits an -explicit `REMOVE` clause. Automatic kind conversions involving `ALIAS` or -`EPHEMERAL` are rejected: use an [empty manual migration](/cli/generate/) and review -the storage and insert behavior before updating the snapshot. The planner never -silently drops and recreates a column to perform these conversions. - -Generated row models exclude `EPHEMERAL` columns. Tables using a special kind also -get a separate `RowInsert` model that excludes `MATERIALIZED` and `ALIAS`; generated -TypeScript ingest helpers use that model. Read models describe an explicit projection -of all readable columns, rather than the default `SELECT *` result. Field requiredness -is unchanged: insert models require their declared input fields, including defaults. - -Automatic backfill projections omit `MATERIALIZED` and `ALIAS` columns. Targets with -`EPHEMERAL` columns require an explicit SQL insert with an input mapping, because -these inputs cannot be recovered from stored rows. +values. The planner reports this explicitly in human and JSON output and includes +the warning in migration SQL. Use a separately reviewed `ALTER TABLE ... MATERIALIZE +COLUMN ...` only when you need that rewrite and the required inputs still exist. +Values derived from discarded `EPHEMERAL` inputs cannot be reconstructed this way. +Removing a `DEFAULT` or `MATERIALIZED` expression emits an explicit `REMOVE` clause. + +Automatic kind conversions involving `ALIAS` or `EPHEMERAL` are rejected. Use a +[manual migration and verified snapshot reconciliation](/cli/generate/#reconcile-manual-column-changes). +The planner never silently drops and recreates a column for these conversions. + +Generated `Row` models describe the default `SELECT *` result, excluding +`MATERIALIZED`, `ALIAS`, and `EPHEMERAL`. Tables using special kinds also get: + +- `RowExplicit`, containing all readable columns, for queries that name those columns explicitly. +- `RowInsert`, excluding `MATERIALIZED` and `ALIAS` and including `EPHEMERAL` inputs. + +These names are suffixes: for example, `DefaultEventsRow`, `DefaultEventsRowExplicit`, +and `DefaultEventsRowInsert`. TypeScript emits matching Zod schemas when enabled; +Python emits Pydantic models. Generated TypeScript ingest helpers use `RowInsert`. +Field requiredness is unchanged: insert models require their declared input fields, +including defaults. Non-default `asterisk_include_materialized_columns` or +`asterisk_include_alias_columns` settings change the result shape; use an explicit +projection and its model when selecting those columns. + +Automatic backfill projections omit `MATERIALIZED` and `ALIAS` columns. Planning +and local execution both check live target column kinds, including when the local +schema is missing or cannot load. Missing, unknown, or unreadable metadata blocks +the backfill. Targets with `EPHEMERAL` columns require explicit SQL with an input +mapping because these inputs cannot be recovered from stored rows. ### `comment` (string, optional) diff --git a/chkit_python/CHANGELOG.md b/chkit_python/CHANGELOG.md index b877194e..4fd36049 100644 --- a/chkit_python/CHANGELOG.md +++ b/chkit_python/CHANGELOG.md @@ -3,6 +3,11 @@ ## Unreleased ### Added +- Compare column defaults with quote-aware SQL tokens and correctly escape literal backslashes. +- Generate separate `Row` (default `SELECT *`), `RowExplicit`, and `RowInsert` models. +- Check live column metadata before backfill planning and local execution; block unknown metadata and unrecoverable `EPHEMERAL` inputs. +- Warn about unchanged historical values in migration output and SQL. Support `generate --reconcile --table` to verify manually applied column expression/kind changes and adopt only those snapshot changes. + - Add `SkipIndexText` for full-text index generation, introspection, pull, and drift. Preserve quoted SQL literals, normalize ClickHouse’s fixed granularity, and reject malformed or unsupported metadata. Exercise adversarial round trips and actual diff --git a/chkit_python/src/chkit/cli/commands/drift_compare.py b/chkit_python/src/chkit/cli/commands/drift_compare.py index 861ca292..8e5b4db2 100644 --- a/chkit_python/src/chkit/cli/commands/drift_compare.py +++ b/chkit_python/src/chkit/cli/commands/drift_compare.py @@ -29,8 +29,10 @@ TableDefinition, ) from chkit.core.projection import is_index_projection, normalize_projection_index +from chkit.core.sql import _render_default from chkit.core.sql_normalizer import normalize_engine, normalize_sql_fragment from chkit.core.text_index import render_text_index_type, text_index_fingerprint +from chkit.core.text_index_sql import text_expression_fingerprint, text_sql_fingerprint _MIN_QUOTED_LEN = 2 @@ -216,25 +218,10 @@ def summarize_drift_reasons( def _normalize_column_shape(column: ColumnDefinition) -> str: - def _normalize_default_value(value: str) -> str: - normalized = normalize_sql_fragment(value) - if ( - len(normalized) >= _MIN_QUOTED_LEN - and normalized[0] == "'" - and normalized[-1] == "'" - ): - inner = normalized[1:-1] - return inner.replace("''", "'") - return normalized - - if column.default is None: - normalized_default = "" - else: - as_string = str(column.default) - if as_string.startswith("fn:"): - normalized_default = _normalize_default_value(as_string[3:]) - else: - normalized_default = _normalize_default_value(as_string) + normalized_default = ( + "" if column.default is None + else text_sql_fingerprint(text_expression_fingerprint(_render_default(column.default))) + ) parts = [ f"type={str(column.type).strip()}", @@ -338,7 +325,12 @@ def compare_table_shape( # noqa: PLR0912, PLR0915 """Compare every shape-bearing field on the table. Returns None if identical.""" column_diff = diff_by_name( expected.columns, - actual.columns, + [ + column.model_copy(update={"default": f"fn:{column.default}"}) + if isinstance(column.default, str) and not column.default.startswith("fn:") + else column + for column in actual.columns + ], lambda c: c.name, _normalize_column_shape, ) diff --git a/chkit_python/src/chkit/cli/commands/generate.py b/chkit_python/src/chkit/cli/commands/generate.py index e3e95b6a..ea53dffc 100644 --- a/chkit_python/src/chkit/cli/commands/generate.py +++ b/chkit_python/src/chkit/cli/commands/generate.py @@ -23,6 +23,7 @@ from chkit.cli.commands.dictionary_password_warnings import ( detect_dictionary_password_warnings, ) +from chkit.cli.commands.drift_compare import compare_table_shape from chkit.cli.commands.generate_plan_pipeline import ( apply_explicit_dictionary_renames, apply_explicit_table_renames, @@ -30,6 +31,7 @@ assert_cli_column_mappings_resolvable, build_explicit_column_rename_suggestions, ) +from chkit.cli.commands.generate_reconcile import reconcile_column_expressions from chkit.cli.commands.generate_rename_mappings import ( ColumnRenameMapping, DictionaryRenameMapping, @@ -66,8 +68,16 @@ resolve_table_scope, table_keys_from_definitions, ) +from chkit.clickhouse.client import ClickHouseClient +from chkit.clickhouse.introspect import list_table_details from chkit.core.canonical import canonicalize_definitions -from chkit.core.model import ChxConfigEnv, ChxResolvedConfig, ChxValidationError, SchemaDefinition +from chkit.core.model import ( + ChxConfigEnv, + ChxResolvedConfig, + ChxValidationError, + SchemaDefinition, + TableDefinition, +) from chkit.core.on_cluster import apply_on_cluster_to_plan from chkit.core.planner import plan_diff from chkit.core.snapshot import create_snapshot @@ -132,10 +142,7 @@ def _run_codegen_integration( ) exit_code = plugin_runtime.run_plugin_command("codegen", "codegen", ctx) if exit_code != 0: - msg = ( - f'Plugin "codegen" failed in generate integration with exit ' - f"code {exit_code}." - ) + msg = f'Plugin "codegen" failed in generate integration with exit code {exit_code}.' raise typer.Exit(code=1) from RuntimeError(msg) @@ -242,20 +249,14 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 list[str] | None, typer.Option( "--rename-table", - help=( - "Explicit table rename mapping old_db.old_table=new_db.new_table. " - "Repeatable." - ), + help=("Explicit table rename mapping old_db.old_table=new_db.new_table. Repeatable."), ), ] = None, rename_column: Annotated[ list[str] | None, typer.Option( "--rename-column", - help=( - "Explicit column rename mapping db.table.old_column=new_column. " - "Repeatable." - ), + help=("Explicit column rename mapping db.table.old_column=new_column. Repeatable."), ), ] = None, rename_dictionary: Annotated[ @@ -263,11 +264,18 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 typer.Option( "--rename-dictionary", help=( - "Explicit dictionary rename mapping old_db.old_dict=new_db.new_dict. " - "Repeatable." + "Explicit dictionary rename mapping old_db.old_dict=new_db.new_dict. Repeatable." ), ), ] = None, + reconcile: Annotated[ + bool, + typer.Option( + "--reconcile", + help=("Verify live column expressions and update their snapshot " + "after a manual migration (requires --table)."), + ), + ] = False, dryrun: Annotated[ bool, typer.Option("--dryrun", help="Print plan without writing artifacts."), @@ -277,6 +285,10 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 typer.Option("--json", help="Emit a JSON-formatted summary."), ] = False, ) -> None: + if reconcile and (not table_selector or rename_column or rename_table or rename_dictionary): + raise typer.BadParameter( + "--reconcile requires --table and cannot be combined with rename flags." + ) config = load_config(config_path, ChxConfigEnv(command="generate")) plugin_runtime = load_plugin_runtime( [p for p in config.plugins if isinstance(p, ChxPlugin)] @@ -330,6 +342,62 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 previous = read_snapshot(meta_dir) old_defs = list(previous.definitions) if previous is not None else [] + if reconcile: + if previous is None: + raise typer.BadParameter( + "Snapshot not found; reconciliation requires an existing snapshot." + ) + scope = resolve_table_scope(table_selector, table_keys_from_definitions(canonical)) + if not scope.match_count: + raise typer.BadParameter("No tables matched --table; snapshot unchanged.") + reconciled = reconcile_column_expressions(old_defs, canonical, list(scope.matched_tables)) + if config.clickhouse is None: + raise typer.BadParameter("clickhouse config is required for --reconcile.") + selected = [ + item + for item in canonical + if isinstance(item, TableDefinition) + and f"{item.database}.{item.name}" in scope.matched_tables + ] + with ClickHouseClient.connect(config.clickhouse) as client: + actual = list_table_details(client, sorted({item.database for item in selected})) + for expected in selected: + live = next( + ( + item + for item in actual + if item.database == expected.database and item.name == expected.name + ), + None, + ) + if live is None or compare_table_shape(expected, live): + raise typer.BadParameter( + f"Live table {expected.database}.{expected.name} does not match the schema; " + "apply and verify the manual migration before --reconcile. Snapshot unchanged." + ) + snapshot_path = meta_dir / "snapshot.json" + if not dryrun: + write_snapshot(meta_dir, create_snapshot(reconciled)) + if output_json: + typer.echo( + json.dumps( + { + "mode": "reconcile", + "verified": True, + "dryrun": dryrun, + "snapshotFile": str(snapshot_path), + "tables": scope.matched_tables, + } + ) + ) + else: + typer.echo( + f"{'Verified' if dryrun else 'Reconciled'} column expressions " + "against live ClickHouse. " + "No migration generated." + ) + return + ( remapped_old_defs, active_table_mappings, @@ -350,9 +418,7 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 ) table_scope = resolve_table_scope(table_selector, available_keys) if table_scope.enabled and table_scope.match_count == 0: - warning = ( - f'No tables matched selector "{table_scope.selector or ""}". No changes planned.' - ) + warning = f'No tables matched selector "{table_scope.selector or ""}". No changes planned.' if output_json: typer.echo( json.dumps( @@ -425,11 +491,11 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 # filtering) — so plugin-injected SQL is also covered. ``migrate`` never # re-runs this: the clause is baked into the migration file at generate # time and applied verbatim. - plan = apply_on_cluster_to_plan( - plan, config.clickhouse.cluster if config.clickhouse else None - ) + plan = apply_on_cluster_to_plan(plan, config.clickhouse.cluster if config.clickhouse else None) - dictionary_password_warnings = detect_dictionary_password_warnings(plan) + dictionary_password_warnings = detect_dictionary_password_warnings(plan) + [ + op.warning for op in plan.operations if op.warning + ] if not plan.operations: if output_json: diff --git a/chkit_python/src/chkit/cli/commands/generate_reconcile.py b/chkit_python/src/chkit/cli/commands/generate_reconcile.py new file mode 100644 index 00000000..b1a3da69 --- /dev/null +++ b/chkit_python/src/chkit/cli/commands/generate_reconcile.py @@ -0,0 +1,56 @@ +"""Narrow snapshot adoption after a manually applied column expression migration.""" + +from __future__ import annotations + +from chkit.core.canonical import canonicalize_definitions +from chkit.core.model import SchemaDefinition, TableDefinition + + +def reconcile_column_expressions( + previous: list[SchemaDefinition], + next_: list[SchemaDefinition], + selected: list[str], +) -> list[SchemaDefinition]: + current = canonicalize_definitions(next_) + result = canonicalize_definitions(previous) + for key in selected: + index = next( + ( + i + for i, item in enumerate(result) + if isinstance(item, TableDefinition) and f"{item.database}.{item.name}" == key + ), + None, + ) + before = result[index] if index is not None else None + after = next( + ( + item + for item in current + if isinstance(item, TableDefinition) and f"{item.database}.{item.name}" == key + ), + None, + ) + if not isinstance(before, TableDefinition) or after is None or index is None: + raise ValueError( + f"Cannot reconcile {key}: table must exist in both the snapshot and schema." + ) + columns = [] + for column in before.columns: + updated = next((item for item in after.columns if item.name == column.name), None) + columns.append( + column.model_copy( + update={ + "default": updated.default if updated else None, + "default_kind": updated.default_kind if updated else None, + } + ) + ) + candidate = before.model_copy(update={"columns": columns}) + if canonicalize_definitions([candidate]) != [after]: + raise ValueError( + f"Cannot reconcile {key}: --reconcile accepts only column expression/kind changes. " + "Generate other schema changes separately." + ) + result[index] = candidate + return result diff --git a/chkit_python/src/chkit/cli/migration_store.py b/chkit_python/src/chkit/cli/migration_store.py index 0acd9756..82df6207 100644 --- a/chkit_python/src/chkit/cli/migration_store.py +++ b/chkit_python/src/chkit/cli/migration_store.py @@ -150,7 +150,9 @@ def _build_migration_content( for s in plan.rename_suggestions ] body_blocks = [ - f"-- operation: {op.type} key={op.key} risk={op.risk}\n{op.sql}" + f"-- operation: {op.type} key={op.key} risk={op.risk}\n" + + ("-- Warning: " + " ".join(op.warning.splitlines()) + "\n" if op.warning else "") + + op.sql for op in plan.operations ] body = "\n\n".join(body_blocks) diff --git a/chkit_python/src/chkit/core/model.py b/chkit_python/src/chkit/core/model.py index 2c30362d..18e4ee55 100644 --- a/chkit_python/src/chkit/core/model.py +++ b/chkit_python/src/chkit/core/model.py @@ -628,6 +628,7 @@ class MigrationOperation(_StrictModel): key: str risk: RiskLevel sql: str + warning: str | None = None class ColumnRenameSuggestion(_StrictModel): diff --git a/chkit_python/src/chkit/core/planner.py b/chkit_python/src/chkit/core/planner.py index 255f3f17..e13e6ddc 100644 --- a/chkit_python/src/chkit/core/planner.py +++ b/chkit_python/src/chkit/core/planner.py @@ -81,10 +81,7 @@ def _push_drop( type="drop_dictionary", key=definition_key(definition), risk=risk, - sql=( - f"DROP DICTIONARY IF EXISTS " - f"{definition.database}.{definition.name};" - ), + sql=(f"DROP DICTIONARY IF EXISTS {definition.database}.{definition.name};"), ) ) return @@ -248,13 +245,8 @@ def _is_codec_removal(old: ColumnDefinition, new: ColumnDefinition) -> bool: return _column_identity_without_codec(old) == _column_identity_without_codec(new) -def _render_rename_column_suggestion_sql( - table: TableDefinition, from_: str, to: str -) -> str: - return ( - f"ALTER TABLE {table.database}.{table.name} " - f"RENAME COLUMN `{from_}` TO `{to}`;" - ) +def _render_rename_column_suggestion_sql(table: TableDefinition, from_: str, to: str) -> str: + return f"ALTER TABLE {table.database}.{table.name} RENAME COLUMN `{from_}` TO `{to}`;" def _infer_column_rename_suggestions( @@ -446,7 +438,8 @@ def _diff_tables( f"Cannot automatically change column {new.database}.{new.name}." f"{column_change.name} " f"from {old_kind} to {new_kind}; " - "use an explicit manual migration for storage-kind changes" + "use an explicit manual migration for storage-kind changes, then run " + f"generate --reconcile --table {new.database}.{new.name} after applying it" ) sql = ( render_alter_remove_codec(new, column_change.name) @@ -459,6 +452,19 @@ def _diff_tables( key=f"table:{new.database}.{new.name}:column:{column_change.name}", risk="caution", sql=sql, + warning=( + f"Changing the expression for {new.database}.{new.name}.{column_change.name} " + "does not rewrite stored historical values. " + "Review a separate MATERIALIZE COLUMN " + "migration if a rewrite is required; never reconstruct values " + "from discarded EPHEMERAL inputs." + if {old_kind, new_kind} & {"DEFAULT", "MATERIALIZED"} + and ( + column_change.old_item.default != column_change.new_item.default + or old_kind != new_kind + ) + else None + ), ) ) for column in column_diff.removed: @@ -517,10 +523,10 @@ def _diff_tables( list(old.projections or []), list(new.projections or []), lambda p: p.name, - lambda left, right: json.dumps( - left.model_dump(mode="json"), sort_keys=True, default=str - ) - == json.dumps(right.model_dump(mode="json"), sort_keys=True, default=str), + lambda left, right: ( + json.dumps(left.model_dump(mode="json"), sort_keys=True, default=str) + == json.dumps(right.model_dump(mode="json"), sort_keys=True, default=str) + ), ) for projection in projection_diff.added: ops.append( @@ -535,10 +541,7 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_drop_projection", - key=( - f"table:{new.database}.{new.name}:projection:" - f"{projection_change.name}" - ), + key=(f"table:{new.database}.{new.name}:projection:{projection_change.name}"), risk="caution", sql=render_alter_drop_projection(new, projection_change.name), ) @@ -546,10 +549,7 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_add_projection", - key=( - f"table:{new.database}.{new.name}:projection:" - f"{projection_change.name}" - ), + key=(f"table:{new.database}.{new.name}:projection:{projection_change.name}"), risk="caution", sql=render_alter_add_projection(new, projection_change.new_item), ) @@ -570,10 +570,7 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_reset_setting", - key=( - f"table:{new.database}.{new.name}:setting:" - f"{setting_change.key}" - ), + key=(f"table:{new.database}.{new.name}:setting:{setting_change.key}"), risk="caution", sql=render_alter_reset_setting(new, setting_change.key), ) @@ -582,14 +579,9 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_modify_setting", - key=( - f"table:{new.database}.{new.name}:setting:" - f"{setting_change.key}" - ), + key=(f"table:{new.database}.{new.name}:setting:{setting_change.key}"), risk="caution", - sql=render_alter_modify_setting( - new, setting_change.key, setting_change.value - ), + sql=render_alter_modify_setting(new, setting_change.key, setting_change.value), ) ) diff --git a/chkit_python/src/chkit/core/sql.py b/chkit_python/src/chkit/core/sql.py index cba58f57..36b892d0 100644 --- a/chkit_python/src/chkit/core/sql.py +++ b/chkit_python/src/chkit/core/sql.py @@ -62,7 +62,7 @@ def _render_default(value: str | int | float | bool) -> str: if isinstance(value, str): if value.startswith("fn:"): return value[3:] - escaped = value.replace("'", "''") + escaped = value.replace("\\", "\\\\").replace("'", "''") return f"'{escaped}'" if isinstance(value, bool): return "true" if value else "false" diff --git a/chkit_python/src/chkit_plugin_backfill/planner.py b/chkit_python/src/chkit_plugin_backfill/planner.py index 947f1037..1fc1933b 100644 --- a/chkit_python/src/chkit_plugin_backfill/planner.py +++ b/chkit_python/src/chkit_plugin_backfill/planner.py @@ -110,6 +110,35 @@ def _detect_backfill_strategy( return _BackfillStrategy(mvs=[]) +def assert_backfill_target_safe( + *, database: str, table: str, query: PlannerQuery, + query_settings: QuerySettings | None = None, +) -> None: + """Fail closed when live metadata cannot establish safe input semantics.""" + def quote(value: str) -> str: + return "'" + value.replace("\\", "\\\\").replace("'", "\\'") + "'" + + rows = query( + "SELECT name, default_kind FROM system.columns " + f"WHERE database = {quote(database)} AND table = {quote(table)} ORDER BY position", + query_settings, + ) + if not rows or any( + not column.get("name") + or column.get("default_kind") not in {"", "DEFAULT", "MATERIALIZED", "ALIAS", "EPHEMERAL"} + for column in rows + ): + raise BackfillConfigError( + "Cannot verify live target column kinds; automatic backfill is blocked. " + "Check metadata access and use an explicit INSERT if needed." + ) + if any(column["default_kind"] == "EPHEMERAL" for column in rows): + raise BackfillConfigError( + "Automatic backfill cannot reconstruct EPHEMERAL inputs; " + "use an explicit INSERT with an input column mapping." + ) + + @dataclass(frozen=True) class BuildBackfillPlanOutput: plan: BackfillPlanState @@ -136,13 +165,16 @@ def build_backfill_plan( # backfill sizes its chunks against the MV *source* (the table its SELECT # reads), because the injected chunk conditions run against that source — # not the target, which is legitimately empty when bootstrapping an - # aggregate. Only the copy path introspects the target itself. + # aggregate. Target column safety is checked separately for both paths. strategy = _detect_backfill_strategy( schema=config.schema_, config_dir=Path(config_path).resolve().parent, database=database, table=table, ) + assert_backfill_target_safe( + database=database, table=table, query=clickhouse_query, query_settings=query_settings, + ) replay_source = ( resolve_mv_replay_source(strategy.mvs) if strategy.mv_replay_queries is not None diff --git a/chkit_python/src/chkit_plugin_backfill/plugin.py b/chkit_python/src/chkit_plugin_backfill/plugin.py index f8a4b66a..6061b45e 100644 --- a/chkit_python/src/chkit_plugin_backfill/plugin.py +++ b/chkit_python/src/chkit_plugin_backfill/plugin.py @@ -53,7 +53,7 @@ plan_payload, status_payload, ) -from chkit_plugin_backfill.planner import build_backfill_plan +from chkit_plugin_backfill.planner import assert_backfill_target_safe, build_backfill_plan from chkit_plugin_backfill.queries import ( cancel_backfill_run, get_backfill_doctor_report, @@ -230,6 +230,14 @@ def _run_backfill( # noqa: PLR0915 — mirrors TS runBackfill db = _ThreadLocalExecutor(clickhouse) try: + database, table = plan.target.split(".") + assert_backfill_target_safe( + database=database, + table=table, + query=lambda sql, settings: ( + db._client().query(sql, dict(settings) if settings is not None else None).rows + ), + ) run_state = BackfillRunState( plan_id=plan.plan_id, target=plan.target, @@ -418,8 +426,7 @@ def clickhouse_query( sort_keys = output.plan.chunk_plan.table.sort_keys primary_sort_key = sort_keys[0] if sort_keys else None sort_key_label = ( - f", sort key: {primary_sort_key.name}" - f" ({primary_sort_key.category})" + f", sort key: {primary_sort_key.name} ({primary_sort_key.category})" if primary_sort_key is not None else "" ) @@ -610,8 +617,7 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ChxPluginCommand( name="plan", description=( - "Build a deterministic backfill plan and persist immutable" - " plan state" + "Build a deterministic backfill plan and persist immutable plan state" ), run=_guarded(_plan, "plan", "Backfill plan"), flags=list(PLAN_FLAGS), @@ -627,10 +633,7 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ), ChxPluginCommand( name="run", - description=( - "Execute a planned backfill with async query submission" - " and polling" - ), + description=("Execute a planned backfill with async query submission and polling"), run=_guarded(_run, "run", "Backfill run"), flags=list(RUN_FLAGS), ), @@ -649,8 +652,7 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ChxPluginCommand( name="cancel", description=( - "Cancel an in-progress backfill run and prevent further" - " chunk execution" + "Cancel an in-progress backfill run and prevent further chunk execution" ), run=_guarded(_cancel, "cancel", "Backfill cancel"), flags=list(PLAN_ID_FLAGS), @@ -658,8 +660,7 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ChxPluginCommand( name="doctor", description=( - "Provide actionable remediation steps for failed or pending" - " backfill runs" + "Provide actionable remediation steps for failed or pending backfill runs" ), run=_guarded(_doctor, "doctor", "Backfill doctor"), flags=list(PLAN_ID_FLAGS), diff --git a/chkit_python/src/chkit_plugin_codegen/type_artifacts.py b/chkit_python/src/chkit_plugin_codegen/type_artifacts.py index f0afd095..49d82e01 100644 --- a/chkit_python/src/chkit_plugin_codegen/type_artifacts.py +++ b/chkit_python/src/chkit_plugin_codegen/type_artifacts.py @@ -303,10 +303,17 @@ def _render_table_model( ) -> tuple[list[str], list[CodegenFinding], set[str]]: """Render the lines for a single table → Pydantic model.""" lines, findings, imports = _render_fields_model( - [column for column in table.columns if column.default_kind != "EPHEMERAL"], + [column for column in table.columns if column.default_kind in {None, "DEFAULT"}], class_name, f"{table.database}.{table.name}", options ) if any(column.default_kind not in {None, "DEFAULT"} for column in table.columns): + explicit_lines, explicit_findings, explicit_imports = _render_fields_model( + [column for column in table.columns if column.default_kind != "EPHEMERAL"], + f"{class_name}Explicit", f"{table.database}.{table.name}", options + ) + lines.extend(explicit_lines) + findings.extend(explicit_findings) + imports.update(explicit_imports) insert_lines, insert_findings, insert_imports = _render_fields_model( [ column for column in table.columns diff --git a/chkit_python/tests/test_backfill_planner.py b/chkit_python/tests/test_backfill_planner.py index 582d5edc..9343d173 100644 --- a/chkit_python/tests/test_backfill_planner.py +++ b/chkit_python/tests/test_backfill_planner.py @@ -79,6 +79,8 @@ def _create_mock_query( def query(sql: str, settings: QuerySettings | None) -> list[dict[str, object]]: _ = settings + if "SELECT name, default_kind" in sql: + return [{"name": "id", "default_kind": ""}] if "SELECT 1 FROM" in sql: return [{"ok": 1}] if "FROM system.parts" in sql: @@ -124,6 +126,8 @@ def _create_source_scoped_mock_query( def query(sql: str, settings: QuerySettings | None) -> list[dict[str, object]]: _ = settings + if "SELECT name, default_kind" in sql: + return [{"name": "id", "default_kind": ""}] if "SELECT 1 FROM" in sql: return [{"ok": 1}] if "FROM system.parts" in sql and f"table = '{source_table}'" in sql: diff --git a/chkit_python/tests/test_column_expressions.py b/chkit_python/tests/test_column_expressions.py index c54d5731..904da6f3 100644 --- a/chkit_python/tests/test_column_expressions.py +++ b/chkit_python/tests/test_column_expressions.py @@ -9,6 +9,7 @@ from chkit import ColumnDefinition, table from chkit.cli.commands.drift_compare import compare_table_shape +from chkit.cli.commands.generate_reconcile import reconcile_column_expressions from chkit.cli.commands.pull import _introspected_table_to_definition from chkit.cli.commands.pull_render import render_schema_file from chkit.clickhouse.introspect import ( @@ -21,7 +22,7 @@ from chkit.core.snapshot import create_snapshot from chkit.core.sql import to_create_sql from chkit.core.validate import validate_definitions -from chkit_plugin_backfill.planner import _detect_backfill_strategy +from chkit_plugin_backfill.planner import _detect_backfill_strategy, assert_backfill_target_safe from chkit_plugin_codegen import generate_type_artifacts @@ -177,7 +178,10 @@ def test_codegen_read_and_insert_models() -> None: models = [value for key, value in namespace.items() if key.endswith(("Row", "RowInsert"))] read = next(model for model in models if model.__name__.endswith("Row")) insert = next(model for model in models if model.__name__.endswith("RowInsert")) - assert set(read.model_fields) == {"ts", "day", "label"} + assert set(read.model_fields) == {"ts"} + explicit = namespace[read.__name__ + "Explicit"] + assert set(explicit.model_fields) == {"ts", "day", "label"} + read.model_validate({"ts": "2026-01-01 00:00:00"}) assert set(insert.model_fields) == {"ts", "raw"} @@ -223,3 +227,75 @@ def test_introspection_preserves_sql_literal_whitespace() -> None: ) ) assert column.default == "concat('a b', toString(id))" + + +@pytest.mark.parametrize( + ("expected", "actual", "equal"), + [ + ("fn:concat('a b', toString(ts))", "concat('a b', toString(ts))", False), + ("fn:toString(ts+1)", "toString(ts + 1)", True), + ("toString(ts)", "toString(ts)", False), + ("toString(ts)", "'toString(ts)'", True), + (" a b ", "' a b '", True), + ("O'Reilly", "'O\\'Reilly'", True), + ("\\n", "'\\\\n'", True), + ("\\n", "'\\n'", False), + ("", None, False), + (False, "false", True), + (0, "0", True), + ("fn:concat('a', `ts`)", "concat('a', ts)", True), + ("fn:toString(ts /* comment */ +1)", "toString(ts + 1)", True), + ("fn:concat('/* a */', ts)", "concat('/* b */', ts)", False), + ], +) +def test_expression_comparison_preserves_literals(expected: Any, actual: Any, equal: bool) -> None: + result = compare_table_shape( + definition(default=expected), actual_table(definition(default=actual).columns) + ) + assert (result is None) == equal + + +def test_reconcile_only_selected_expression_metadata() -> None: + + before = definition(default="fn:toDate(ts)") + after = definition(default="fn:toDate(ts)", default_kind="ALIAS") + unrelated = before.model_copy(update={"name": "other"}) + reconciled = reconcile_column_expressions( + [before, unrelated], + [after, unrelated.model_copy(update={"engine": "Log"})], + ["default.events"], + ) + assert plan_diff(reconciled, [after, unrelated]).operations == [] + with pytest.raises(ValueError, match="only column expression/kind changes"): + reconcile_column_expressions( + [before], [after.model_copy(update={"engine": "Log"})], ["default.events"] + ) + with pytest.raises(ValueError, match="both the snapshot and schema"): + reconcile_column_expressions([], [after], ["default.events"]) + + +def test_stored_expression_warning() -> None: + plan = plan_diff([definition(default="fn:toDate(ts)")], [definition(default="fn:today()")]) + assert "does not rewrite stored historical values" in (plan.operations[0].warning or "") + plan = plan_diff( + [definition(default="fn:toDate(ts)", default_kind="ALIAS")], + [definition(default="fn:today()", default_kind="ALIAS")], + ) + assert plan.operations[0].warning is None + + +@pytest.mark.parametrize( + ("rows", "message"), + [ + ([{"name": "raw", "default_kind": "EPHEMERAL"}], "cannot reconstruct EPHEMERAL"), + ([], "Cannot verify live target column kinds"), + ([{"name": "raw"}], "Cannot verify live target column kinds"), + ([{"name": "raw", "default_kind": "FUTURE"}], "Cannot verify live target column kinds"), + ], +) +def test_backfill_live_safety_gate(rows: list[dict[str, object]], message: str) -> None: + + with pytest.raises(Exception, match=message): + assert_backfill_target_safe( + database="default", table="events", query=lambda sql, settings: rows + ) diff --git a/chkit_python/tests/test_column_expressions_e2e.py b/chkit_python/tests/test_column_expressions_e2e.py index 63ecb3d8..5e4ec025 100644 --- a/chkit_python/tests/test_column_expressions_e2e.py +++ b/chkit_python/tests/test_column_expressions_e2e.py @@ -2,15 +2,18 @@ from __future__ import annotations +import json from typing import Any from uuid import uuid4 import pytest +from typer.testing import CliRunner from chkit import table from chkit.cli.commands.drift_compare import compare_table_shape from chkit.cli.commands.pull import _introspected_table_to_definition from chkit.cli.commands.pull_render import render_schema_file +from chkit.cli.main import app from chkit.clickhouse.introspect import ( IntrospectedTable, SystemColumnRow, @@ -18,6 +21,8 @@ ) from chkit.core.planner import plan_diff from chkit.core.sql import to_create_sql +from chkit_plugin_codegen import generate_type_artifacts +from tests.e2e_testkit import get_required_env def test_expression_column_lifecycle(ch_client: Any) -> None: @@ -73,6 +78,13 @@ def test_expression_column_lifecycle(ch_client: Any) -> None: assert plan_diff([definition], namespace["definitions"]).operations == [] client.command(f"INSERT INTO {target} (id, raw) VALUES (1, 'abc')") assert client.query(f"SELECT size, label FROM {target}").result_rows == [(3, "3")] + models: dict[str, Any] = {"__name__": "generated_live"} + exec(generate_type_artifacts(definitions=[definition]).content, models) + read = next(value for key, value in models.items() if key.endswith("Row")) + explicit = models[read.__name__ + "Explicit"] + read.model_validate(next(client.query(f"SELECT * FROM {target}").named_results())) + # UInt64 models use the same string encoding as ClickHouse JSON output. + explicit.model_validate({"id": 1, "size": "3", "label": "3"}) with pytest.raises(Exception, match="MATERIALIZED"): client.command(f"INSERT INTO {target} (id, size) VALUES (2, 10)") with pytest.raises(Exception, match="raw"): @@ -111,3 +123,72 @@ def test_expression_column_lifecycle(ch_client: Any) -> None: ).result_rows == [("",)] finally: client.command(f"DROP TABLE IF EXISTS {target} SYNC") + + +def test_manual_conversion_reconciliation(ch_client: Any, tmp_path: Any, monkeypatch: Any) -> None: + + + + env = get_required_env() + client = ch_client._client + name = f"reconcile_py_{uuid4().hex}" + before = table( + database=env.clickhouse_database, + name=name, + engine="MergeTree()", + primary_key=["id"], + order_by=["id"], + columns=[ + {"name": "id", "type": "UInt32"}, + {"name": "label", "type": "String", "default": "fn:toString(id)"}, + ], + ) + after = before.model_copy( + update={ + "columns": [ + before.columns[0], + before.columns[1].model_copy(update={"default_kind": "ALIAS"}), + ] + } + ) + monkeypatch.chdir(tmp_path) + config = { + "schema": "./schema.py", + "metaDir": "./meta", + "migrationsDir": "./migrations", + "clickhouse": { + "url": env.clickhouse_url, + "username": env.clickhouse_user, + "password": env.clickhouse_password, + "database": env.clickhouse_database, + }, + } + (tmp_path / "clickhouse.config.py").write_text( + f"from chkit import define_config\nconfig = define_config({config!r})\n" + ) + (tmp_path / "schema.py").write_text(render_schema_file([before])) + runner = CliRunner() + try: + assert runner.invoke(app, ["generate", "--json"], catch_exceptions=False).exit_code == 0 + client.command(to_create_sql(before)) + snapshot = tmp_path / "meta" / "snapshot.json" + original = snapshot.read_text() + (tmp_path / "schema.py").write_text(render_schema_file([after])) + args = ["generate", "--reconcile", "--table", f"{before.database}.{name}", "--json"] + result = runner.invoke(app, args) + assert result.exit_code != 0 + assert snapshot.read_text() == original + client.command( + f"ALTER TABLE {before.database}.{name} DROP COLUMN label, ADD COLUMN label String ALIAS toString(id)" + ) + result = runner.invoke(app, [*args, "--dryrun"], catch_exceptions=False) + assert result.exit_code == 0, result.output + assert snapshot.read_text() == original + result = runner.invoke(app, args, catch_exceptions=False) + assert result.exit_code == 0, result.output + assert json.loads(result.output)["verified"] is True + result = runner.invoke(app, ["generate", "--dryrun", "--json"], catch_exceptions=False) + assert result.exit_code == 0, result.output + assert json.loads(result.output)["operationCount"] == 0 + finally: + client.command(f"DROP TABLE IF EXISTS {before.database}.{name} SYNC") diff --git a/packages/cli/src/commands/drift/compare.ts b/packages/cli/src/commands/drift/compare.ts index 6e8c20ab..3d4b6c20 100644 --- a/packages/cli/src/commands/drift/compare.ts +++ b/packages/cli/src/commands/drift/compare.ts @@ -6,6 +6,8 @@ import { isIndexProjection, normalizeProjectionIndex, normalizeSQLFragment, + renderDefault, + sqlExpressionFingerprint, type ColumnDefinition, type ProjectionDefinition, type SkipIndexDefinition, @@ -185,19 +187,9 @@ export function summarizeDriftReasons(input: { } function normalizeColumnShape(column: ColumnDefinition): string { - const normalizeDefaultValue = (value: string): string => { - const normalized = normalizeSQLFragment(value) - const quoted = normalized.match(/^'(.*)'$/) - if (!quoted) return normalized - return (quoted[1] ?? '').replace(/''/g, "'") - } - - const normalizedDefault = (() => { - if (column.default === undefined) return '' - const asString = String(column.default) - if (asString.startsWith('fn:')) return normalizeDefaultValue(asString.slice(3)) - return normalizeDefaultValue(asString) - })() + const normalizedDefault = column.default === undefined + ? '' + : sqlExpressionFingerprint(renderDefault(column.default)) const parts = [ `type=${String(column.type).trim()}`, `nullable=${column.nullable ? '1' : '0'}`, @@ -275,7 +267,13 @@ function normalizeEngine(value: string | undefined): string { export function compareTableShape(expected: TableDefinition, actual: ActualTableShape): TableDriftDetail | null { const columnDiff = diffByName( expected.columns, - actual.columns, + // system.columns stores SQL, whereas schema strings are literals unless fn:-prefixed. + actual.columns.map((column) => ({ + ...column, + default: typeof column.default === 'string' && !column.default.startsWith('fn:') + ? `fn:${column.default}` + : column.default, + })), (column: ColumnDefinition) => column.name, normalizeColumnShape ) diff --git a/packages/cli/src/commands/generate/command.ts b/packages/cli/src/commands/generate/command.ts index 5bdbdbeb..5860c0c6 100644 --- a/packages/cli/src/commands/generate/command.ts +++ b/packages/cli/src/commands/generate/command.ts @@ -1,5 +1,9 @@ +import { writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { compareTableShape } from '../drift/compare.js' +import { reconcileColumnExpressions } from './reconcile.js' import { generateArtifacts, generateEmptyMigration } from '@chkit/codegen' -import { applyOnClusterToPlan, ChxValidationError, planDiff } from '@chkit/core' +import { applyOnClusterToPlan, assertValidDefinitions, createSnapshot, ChxValidationError, planDiff } from '@chkit/core' import { defineFlags, typedFlags, type ChxPluginCommand } from '../../plugins.js' import { resolveDirs } from '../../runtime/config.js' @@ -50,6 +54,7 @@ const GENERATE_FLAGS = defineFlags([ { name: '--rename-column', type: 'string[]', description: 'Explicit column rename mapping', placeholder: '' }, { name: '--rename-dictionary', type: 'string[]', description: 'Explicit dictionary rename mapping', placeholder: '' }, { name: '--dryrun', type: 'boolean', description: 'Print plan without writing artifacts' }, + { name: '--reconcile', type: 'boolean', description: 'Verify live column expressions and update their snapshot after a manual migration (requires --table)' }, { name: '--empty', type: 'boolean', description: 'Scaffold a blank manual migration (no schema diff, snapshot untouched)' }, ] as const) @@ -70,6 +75,10 @@ async function cmdGenerate(ctx: import('../../plugins.js').ChxPluginCommandConte const planMode = f['--dryrun'] === true const jsonMode = f['--json'] === true const emptyMode = f['--empty'] === true + const reconcileMode = f['--reconcile'] === true + if (reconcileMode && (!tableSelector || emptyMode || f['--rename-column'] || f['--rename-table'] || f['--rename-dictionary'])) { + throw new Error('--reconcile requires --table and cannot be combined with --empty or rename flags.') + } debug('generate', `flags: name=${migrationName ?? '(auto)'}, dryrun=${planMode}, json=${jsonMode}, empty=${emptyMode}`) @@ -106,6 +115,30 @@ async function cmdGenerate(ctx: import('../../plugins.js').ChxPluginCommandConte definitions, }) + if (reconcileMode) { + assertValidDefinitions(definitions) + const previous = await readSnapshot(dirs.metaDir) + if (!previous) throw new Error('Snapshot not found; reconciliation requires an existing snapshot.') + const scope = resolveTableScope(tableSelector, tableKeysFromDefinitions(definitions)) + if (!scope.matchCount) throw new Error('No tables matched --table; snapshot unchanged.') + const reconciled = reconcileColumnExpressions(previous.definitions, definitions, scope.matchedTables) + if (!ctx.pluginContext.hasExecutor) throw new Error('clickhouse config is required for --reconcile.') + const selected = definitions.filter((item) => item.kind === 'table' && scope.matchedTables.includes(`${item.database}.${item.name}`)) + const actual = await ctx.pluginContext.executor.listTableDetails([...new Set(selected.map((item) => item.database))]) + for (const expected of selected) { + if (expected.kind !== 'table') continue + const live = actual.find((item) => item.database === expected.database && item.name === expected.name) + if (!live || compareTableShape(expected, live)) { + throw new Error(`Live table ${expected.database}.${expected.name} does not match the schema; apply and verify the manual migration before --reconcile. Snapshot unchanged.`) + } + } + const snapshotFile = join(dirs.metaDir, 'snapshot.json') + if (!planMode) await writeFile(snapshotFile, `${JSON.stringify(createSnapshot(reconciled), null, 2)}\n`, 'utf8') + if (jsonMode) emitJson('generate', { mode: 'reconcile', verified: true, dryrun: planMode, snapshotFile, tables: scope.matchedTables }) + else console.log(`${planMode ? 'Verified' : 'Reconciled'} column expressions for ${scope.matchedTables.join(', ')} against live ClickHouse. No migration generated.`) + return 0 + } + const renameTableValues = f['--rename-table'] ?? [] const renameColumnValues = f['--rename-column'] ?? [] const renameDictionaryValues = f['--rename-dictionary'] ?? [] @@ -204,7 +237,10 @@ async function cmdGenerate(ctx: import('../../plugins.js').ChxPluginCommandConte // post-pass, after all plan transforms (renames, plugins, scope filtering). plan = applyOnClusterToPlan(plan, config.clickhouse?.cluster) - const dictionaryPasswordWarnings = detectDictionaryPasswordWarnings(plan) + const dictionaryPasswordWarnings = [ + ...detectDictionaryPasswordWarnings(plan), + ...plan.operations.flatMap((operation) => operation.warning ? [operation.warning] : []), + ] if (planMode) { emitGeneratePlanOutput(plan, jsonMode, resolvedScope, dictionaryPasswordWarnings) diff --git a/packages/cli/src/commands/generate/reconcile.ts b/packages/cli/src/commands/generate/reconcile.ts new file mode 100644 index 00000000..f231f3d7 --- /dev/null +++ b/packages/cli/src/commands/generate/reconcile.ts @@ -0,0 +1,56 @@ +import { + canonicalizeDefinitions, + type SchemaDefinition, + type TableDefinition, +} from '@chkit/core' + +/** Adopt only expression metadata; every other schema edit still needs generation. */ +export function reconcileColumnExpressions( + previous: SchemaDefinition[], + next: SchemaDefinition[], + selected: string[], +): SchemaDefinition[] { + const current = canonicalizeDefinitions(next) + const result = canonicalizeDefinitions(previous) + for (const key of selected) { + const index = result.findIndex( + (definition) => + definition.kind === 'table' && + `${definition.database}.${definition.name}` === key, + ) + const before = result[index] + const after = current.find( + (definition): definition is TableDefinition => + definition.kind === 'table' && + `${definition.database}.${definition.name}` === key, + ) + if (before?.kind !== 'table' || !after) + throw new Error( + `Cannot reconcile ${key}: table must exist in both the snapshot and schema.`, + ) + const candidate: TableDefinition = { + ...before, + columns: before.columns.map((column) => { + const updated = after.columns.find((item) => item.name === column.name) + const { default: _default, defaultKind: _kind, ...rest } = column + return { + ...rest, + ...(updated?.default !== undefined + ? { default: updated.default } + : {}), + ...(updated?.defaultKind ? { defaultKind: updated.defaultKind } : {}), + } + }), + } + if ( + JSON.stringify(canonicalizeDefinitions([candidate])) !== + JSON.stringify([after]) + ) { + throw new Error( + `Cannot reconcile ${key}: --reconcile accepts only column expression/kind changes. Generate other schema changes separately.`, + ) + } + result[index] = candidate + } + return result +} diff --git a/packages/cli/src/test/column-expression-safety.test.ts b/packages/cli/src/test/column-expression-safety.test.ts new file mode 100644 index 00000000..a6667bb2 --- /dev/null +++ b/packages/cli/src/test/column-expression-safety.test.ts @@ -0,0 +1,103 @@ +import { expect, test } from 'bun:test' +import { planDiff, table } from '@chkit/core' +import { compareTableShape } from '../commands/drift/compare.js' +import { reconcileColumnExpressions } from '../commands/generate/reconcile.js' + +const definition = (value?: string | number | boolean) => + table({ + database: 'default', + name: 'events', + engine: 'MergeTree()', + primaryKey: ['id'], + orderBy: ['id'], + columns: [ + { name: 'id', type: 'UInt32' }, + { name: 'value', type: 'String', default: value }, + ], + }) + +for (const [expected, actual, equal] of [ + ["fn:concat('a b', toString(id))", "concat('a b', toString(id))", false], + ['fn:toString(id+1)', 'toString(id + 1)', true], + ['toString(id)', 'toString(id)', false], + ['toString(id)', "'toString(id)'", true], + [' a b ', "' a b '", true], + ["O'Reilly", "'O\\'Reilly'", true], + ['\\n', "'\\\\n'", true], + ['\\n', "'\\n'", false], + ['', undefined, false], + [false, 'false', true], + [0, '0', true], + ["fn:concat('a', `id`)", "concat('a', id)", true], + ['fn:toString(id /* comment */ +1)', 'toString(id + 1)', true], + ["fn:concat('/* a */', id)", "concat('/* b */', id)", false], +] as const) { + test(`default comparison ${JSON.stringify(expected)} vs ${JSON.stringify(actual)}`, () => { + const def = definition(expected) + const result = compareTableShape(def, { + columns: definition(actual).columns, + engine: 'MergeTree()', + primaryKey: 'id', + orderBy: 'id', + settings: {}, + indexes: [], + projections: [], + }) + expect(result === null).toBe(equal) + }) +} + +test('reconciliation adopts only selected expression metadata and permits the next generate', () => { + const before = definition('fn:toString(id)') + const after = { + ...before, + columns: before.columns.map((col) => + col.name === 'value' ? { ...col, defaultKind: 'ALIAS' as const } : col, + ), + } + const unrelated = { ...before, name: 'other' } + const reconciled = reconcileColumnExpressions( + [before, unrelated], + [after, { ...unrelated, comment: 'pending' }], + ['default.events'], + ) + expect(planDiff(reconciled, [after, unrelated]).operations).toEqual([]) + expect(() => + reconcileColumnExpressions( + [before], + [{ ...after, engine: 'Log' }], + ['default.events'], + ), + ).toThrow('only column expression/kind changes') + expect(() => + reconcileColumnExpressions([], [after], ['default.events']), + ).toThrow('both the snapshot and schema') +}) + +test('stored expression changes warn about historical values; computed aliases do not', () => { + const before = definition('old') + const after = definition('new') + expect(planDiff([before], [after]).operations[0]?.warning).toContain( + 'does not rewrite stored historical values', + ) + const asAlias = (def: ReturnType) => ({ + ...def, + columns: def.columns.map((col) => + col.name === 'value' ? { ...col, defaultKind: 'ALIAS' as const } : col, + ), + }) + expect( + planDiff([asAlias(before)], [asAlias(after)]).operations[0]?.warning, + ).toBeUndefined() +}) + +test('historical value warnings are persisted in migration SQL', async () => { + const { generateArtifacts } = await import('@chkit/codegen') + const { mkdtemp, readFile, rm } = await import('node:fs/promises') + const dir = await mkdtemp('/tmp/chkit-expression-warning-') + try { + const after = definition('new') + const result = await generateArtifacts({ definitions: [after], migrationsDir: `${dir}/migrations`, metaDir: `${dir}/meta`, plan: planDiff([definition('old')], [after]) }) + expect(await readFile(result.migrationFile ?? '', 'utf8')).toContain('-- Warning: Changing the expression') + } finally { await rm(dir, { recursive: true, force: true }) } +}) diff --git a/packages/cli/src/test/column-expressions.e2e.test.ts b/packages/cli/src/test/column-expressions.e2e.test.ts index c112ca2b..3226fe17 100644 --- a/packages/cli/src/test/column-expressions.e2e.test.ts +++ b/packages/cli/src/test/column-expressions.e2e.test.ts @@ -1,5 +1,5 @@ import { expect, test } from 'bun:test' -import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { createClient } from '@clickhouse/client' @@ -14,6 +14,7 @@ import { generateTypeArtifacts, generateIngestArtifacts, } from '../../../plugin-codegen/src/index.js' +import { createFixture, runCli } from './testkit.test.js' import { getRequiredEnv } from './e2e-testkit.js' test('column expressions survive create, pull, drift, inserts and ALTER on live ClickHouse', async () => { @@ -52,7 +53,13 @@ test('column expressions survive create, pull, drift, inserts and ALTER on live ], }) const query = async (sql: string) => - (await client.query({ query: sql, format: 'JSONEachRow' })).json() + ( + await client.query({ + query: sql, + format: 'JSONEachRow', + clickhouse_settings: { output_format_json_quote_64bit_integers: 1 }, + }) + ).json() const columns = async () => ( await query( @@ -111,6 +118,31 @@ test('column expressions survive create, pull, drift, inserts and ALTER on live `SELECT day, label, toString(size) AS size FROM ${def.database}.${name}`, ), ).toEqual([{ day: '2026-01-01', label: '2026-01-01', size: '3' }]) + const generatedPath = join(dir, 'types.ts') + await writeFile( + generatedPath, + generateTypeArtifacts({ + definitions: [def], + options: { emitZod: true }, + }).content.replace("'zod'", JSON.stringify(import.meta.resolve('zod'))), + ) + const models = await import(generatedPath) + const readSchema = + models[Object.keys(models).find((key) => key.endsWith('RowSchema')) ?? ''] + const explicitSchema = + models[ + Object.keys(models).find((key) => key.endsWith('RowExplicitSchema')) ?? + '' + ] + const star = (await query(`SELECT * FROM ${def.database}.${name}`))[0] + const full = ( + await query( + `SELECT id, ts, day, label, size FROM ${def.database}.${name}`, + ) + )[0] + expect(readSchema.parse(star)).toBeDefined() + expect(explicitSchema.safeParse(star).success).toBe(false) + expect(explicitSchema.safeParse(full).success).toBe(true) await expect( client.command({ query: `INSERT INTO ${def.database}.${name} (id, ts, day) VALUES (2, '2026-01-01 12:00:00', '2000-01-01')`, @@ -216,8 +248,14 @@ test('generated ingest helpers use insert shapes, while rows exclude ephemeral i const insert = types .split('export type DefaultEventsRowInsert = {')[1] ?.split('}')[0] - expect(read).toContain('computed: number') - expect(read).toContain('label: string') + expect(read).not.toContain('computed:') + expect(read).not.toContain('label:') + const explicit = types + .split('export type DefaultEventsRowExplicit = {')[1] + ?.split('}')[0] + expect(explicit).toContain('computed: number') + expect(explicit).toContain('label: string') + expect(explicit).not.toContain('raw:') expect(read).not.toContain('raw:') expect(insert).toContain('raw: string') expect(insert).not.toContain('computed:') @@ -230,3 +268,75 @@ test('generated ingest helpers use insert shapes, while rows exclude ephemeral i expect(ingest).toContain('DefaultEventsRowInsertSchema.parse(row)') expect(ingest).toContain('function ingestDefaultEvents(') }) + +test('manual conversion reconciles only after live verification, then generate is a no-op', async () => { + const env = getRequiredEnv() + const client = createClient({ + url: env.clickhouseUrl, + username: env.clickhouseUser, + password: env.clickhousePassword, + database: env.clickhouseDatabase, + }) + const name = `reconcile_expr_${Date.now()}` + const before = table({ + database: env.clickhouseDatabase, + name, + engine: 'MergeTree()', + primaryKey: ['id'], + orderBy: ['id'], + columns: [ + { name: 'id', type: 'UInt32' }, + { name: 'label', type: 'String', default: 'fn:toString(id)' }, + ], + }) + const fixture = await createFixture( + `export default [${JSON.stringify(before)}]`, + ) + const args = ['generate', '--config', fixture.configPath, '--json'] + try { + await writeFile( + fixture.configPath, + `export default ${JSON.stringify({ schema: [fixture.schemaPath], metaDir: fixture.metaDir, migrationsDir: fixture.migrationsDir, clickhouse: { url: env.clickhouseUrl, username: env.clickhouseUser, password: env.clickhousePassword, database: env.clickhouseDatabase } })}`, + ) + expect(runCli(args).exitCode).toBe(0) + await client.command({ query: toCreateSQL(before) }) + const snapshotPath = join(fixture.metaDir, 'snapshot.json') + const oldSnapshot = await readFile(snapshotPath, 'utf8') + const after = { + ...before, + columns: before.columns.map((col) => + col.name === 'label' ? { ...col, defaultKind: 'ALIAS' } : col, + ), + } + await writeFile( + fixture.schemaPath, + `export default [${JSON.stringify(after)}]`, + ) + const reconcile = [ + ...args, + '--reconcile', + '--table', + `${before.database}.${name}`, + ] + expect(runCli(reconcile).exitCode).not.toBe(0) + expect(await readFile(snapshotPath, 'utf8')).toBe(oldSnapshot) + await client.command({ + query: `ALTER TABLE ${before.database}.${name} DROP COLUMN label, ADD COLUMN label String ALIAS toString(id)`, + }) + const preview = runCli([...reconcile, '--dryrun']) + expect(preview.exitCode).toBe(0) + expect(await readFile(snapshotPath, 'utf8')).toBe(oldSnapshot) + const result = runCli(reconcile) + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout).verified).toBe(true) + const next = runCli([...args, '--dryrun']) + expect(next.exitCode).toBe(0) + expect(JSON.parse(next.stdout).operationCount).toBe(0) + } finally { + await client.command({ + query: `DROP TABLE IF EXISTS ${before.database}.${name} SYNC`, + }) + await client.close() + await rm(fixture.dir, { recursive: true, force: true }) + } +}, 30_000) diff --git a/packages/codegen/src/index.ts b/packages/codegen/src/index.ts index 1a46c106..6c8cfa1a 100644 --- a/packages/codegen/src/index.ts +++ b/packages/codegen/src/index.ts @@ -126,7 +126,11 @@ function buildMigrationContent(input: { ) const body = input.plan.operations - .map((op) => [`-- operation: ${op.type} key=${op.key} risk=${op.risk}`, op.sql].join('\n')) + .map((op) => [ + `-- operation: ${op.type} key=${op.key} risk=${op.risk}`, + ...(op.warning ? [`-- Warning: ${op.warning.replace(/[\r\n]/g, ' ')}`] : []), + op.sql, + ].join('\n')) .join('\n\n') const withHints = [...header, ...renameHints] diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 2af4c46e..77cd4c05 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -20,8 +20,8 @@ export { } from './text-index.js' export { splitTopLevelComma } from './key-clause.js' export { isIndexProjection, normalizeProjectionIndex } from './projection.js' -export { normalizeEngine, normalizeSQLFragment } from './sql-normalizer.js' -export { renderDictionarySQL, toCreateSQL } from './sql.js' +export { normalizeEngine, normalizeSQLFragment, sqlExpressionFingerprint } from './sql-normalizer.js' +export { renderDefault, renderDictionarySQL, toCreateSQL } from './sql.js' export { applyOnClusterToPlan, onClusterClause } from './on-cluster.js' export { canonicalizeCodec, diff --git a/packages/core/src/model-types.ts b/packages/core/src/model-types.ts index e5d0450b..19a1a051 100644 --- a/packages/core/src/model-types.ts +++ b/packages/core/src/model-types.ts @@ -386,6 +386,7 @@ export interface MigrationOperation { key: string risk: RiskLevel sql: string + warning?: string } export interface ColumnRenameSuggestion { diff --git a/packages/core/src/planner.ts b/packages/core/src/planner.ts index a2a54a3f..f92aadc5 100644 --- a/packages/core/src/planner.ts +++ b/packages/core/src/planner.ts @@ -412,7 +412,7 @@ function diffTables(oldDef: TableDefinition, newDef: TableDefinition): TableDiff const oldKind = oldItem.defaultKind ?? 'DEFAULT' const newKind = newItem.defaultKind ?? 'DEFAULT' if (oldKind !== newKind && [oldKind, newKind].some((kind) => kind === 'ALIAS' || kind === 'EPHEMERAL')) { - throw new Error(`Cannot automatically change column ${newDef.database}.${newDef.name}.${name} from ${oldKind} to ${newKind}; use an explicit manual migration for storage-kind changes`) + throw new Error(`Cannot automatically change column ${newDef.database}.${newDef.name}.${name} from ${oldKind} to ${newKind}; use an explicit manual migration for storage-kind changes, then run generate --reconcile --table ${newDef.database}.${newDef.name} after applying it`) } const sql = isCodecRemoval(oldItem, newItem) ? renderAlterRemoveCodec(newDef, name) @@ -422,6 +422,9 @@ function diffTables(oldDef: TableDefinition, newDef: TableDefinition): TableDiff key: `table:${newDef.database}.${newDef.name}:column:${name}`, risk: 'caution', sql, + ...((oldKind === 'DEFAULT' || oldKind === 'MATERIALIZED' || newKind === 'DEFAULT' || newKind === 'MATERIALIZED') && (oldItem.default !== newItem.default || oldKind !== newKind) + ? { warning: `Changing the expression for ${newDef.database}.${newDef.name}.${name} does not rewrite stored historical values. Review a separate MATERIALIZE COLUMN migration if a rewrite is required; never reconstruct values from discarded EPHEMERAL inputs.` } + : {}), }) } for (const column of columnDiff.removed) { diff --git a/packages/core/src/sql-normalizer.ts b/packages/core/src/sql-normalizer.ts index 6a107164..83b29445 100644 --- a/packages/core/src/sql-normalizer.ts +++ b/packages/core/src/sql-normalizer.ts @@ -1,4 +1,10 @@ import { isKafkaEngine, normalizeKafkaEngine } from './kafka.js' +import { textExpressionFingerprint, textSQLFingerprint } from './text-index-sql.js' + +/** Compare expression tokens while preserving quoted values and identifier case. */ +export function sqlExpressionFingerprint(value: string): string { + return textSQLFingerprint(textExpressionFingerprint(value)) +} export function normalizeSQLFragment(value: string): string { return value.replace(/\s+/g, ' ').trim() diff --git a/packages/core/src/sql.ts b/packages/core/src/sql.ts index 7fe0190c..02bc7600 100644 --- a/packages/core/src/sql.ts +++ b/packages/core/src/sql.ts @@ -17,10 +17,10 @@ import { renderProjectionBody } from './projection.js' import { TEXT_INDEX_GRANULARITY, renderTextIndexType } from './text-index.js' import { assertValidDefinitions } from './validate.js' -function renderDefault(value: string | number | boolean): string { +export function renderDefault(value: string | number | boolean): string { if (typeof value === 'string') { if (value.startsWith('fn:')) return value.slice(3) - return `'${value.replace(/'/g, "''")}'` + return `'${value.replace(/\\/g, "\\\\").replace(/'/g, "''")}'` } return String(value) } diff --git a/packages/plugin-backfill/src/planner.test.ts b/packages/plugin-backfill/src/planner.test.ts index 12ec4926..83fe6b98 100644 --- a/packages/plugin-backfill/src/planner.test.ts +++ b/packages/plugin-backfill/src/planner.test.ts @@ -37,6 +37,7 @@ function createMockQuery(opts: { const columnRows = opts.columnRows ?? [{ name: 'event_time', type: 'DateTime' }] return async (sql: string) => { + if (sql.includes('SELECT name, default_kind')) return [{ name: 'id', default_kind: '' }] as T[] if (sql.includes('SELECT 1 FROM')) return [{ ok: 1 }] as T[] if (sql.includes('FROM system.parts')) return partitions as T[] if (sql.includes('FROM system.tables')) return [{ sorting_key: sortingKey }] as T[] @@ -79,6 +80,7 @@ function createSourceScopedMockQuery(opts: { const table = opts.sourceTable return async (sql: string) => { + if (sql.includes('SELECT name, default_kind')) return [{ name: 'id', default_kind: '' }] as T[] if (sql.includes('SELECT 1 FROM')) return [{ ok: 1 }] as T[] if (sql.includes('FROM system.parts')) { return (sql.includes(`table = '${table}'`) ? partitions : []) as T[] @@ -578,3 +580,22 @@ test('MV replay omits computed columns and rejects unrecoverable ephemeral input await rm(dir, { recursive: true, force: true }) } }) + +for (const [rows, message] of [ + [[{ name: 'raw', default_kind: 'EPHEMERAL' }], 'cannot reconstruct EPHEMERAL'], + [[], 'Cannot verify live target column kinds'], + [[{ name: 'raw' }], 'Cannot verify live target column kinds'], + [[{ name: 'raw', default_kind: 'FUTURE' }], 'Cannot verify live target column kinds'], +] as const) { + test(`live target metadata blocks unsafe schema fallback: ${JSON.stringify(rows)}`, async () => { + const dir = await mkdtemp(join(tmpdir(), 'chkit-backfill-unsafe-')) + try { + await expect(buildBackfillPlan({ + opts: PlanSchema.parse({ target: 'app.events' }), + configPath: join(dir, 'config.ts'), + config: { metaDir: join(dir, 'meta'), schema: [join(dir, 'missing.ts')] }, + clickhouseQuery: async () => [...rows] as T[], + })).rejects.toThrow(message) + } finally { await rm(dir, { recursive: true, force: true }) } + }) +} diff --git a/packages/plugin-backfill/src/planner.ts b/packages/plugin-backfill/src/planner.ts index b6a8ca5b..1658160c 100644 --- a/packages/plugin-backfill/src/planner.ts +++ b/packages/plugin-backfill/src/planner.ts @@ -65,6 +65,26 @@ async function detectBackfillStrategy(input: { } } +/** Missing, unreadable or unsupported metadata must never bypass the safety gate. */ +export async function assertBackfillTargetSafe(input: { + database: string + table: string + query: (sql: string, settings?: Record) => Promise + querySettings?: Record +}): Promise { + const quote = (value: string) => `'${value.replaceAll('\\', '\\\\').replaceAll("'", "\\'")}'` + const rows = await input.query<{ name: string; default_kind: string }>( + `SELECT name, default_kind FROM system.columns WHERE database = ${quote(input.database)} AND table = ${quote(input.table)} ORDER BY position`, + input.querySettings + ) + if (!rows.length || rows.some((column) => !column.name || !['', 'DEFAULT', 'MATERIALIZED', 'ALIAS', 'EPHEMERAL'].includes(column.default_kind))) { + throw new BackfillConfigError('Cannot verify live target column kinds; automatic backfill is blocked. Check metadata access and use an explicit INSERT if needed.') + } + if (rows.some((column) => column.default_kind === 'EPHEMERAL')) { + throw new BackfillConfigError('Automatic backfill cannot reconstruct EPHEMERAL inputs; use an explicit INSERT with an input column mapping.') + } +} + export async function buildBackfillPlan(input: { opts: PlanOptions configPath: string @@ -82,14 +102,16 @@ export async function buildBackfillPlan(input: { // Detect the execution strategy before chunk planning: an mv_replay backfill // sizes its chunks against the MV *source* (the table its SELECT reads), // because the injected chunk conditions run against that source — not the - // target, which is legitimately empty when bootstrapping an aggregate. Only - // the copy path introspects the target itself. + // target, which is legitimately empty when bootstrapping an aggregate. Target column safety is checked separately for both paths. const strategy = await detectBackfillStrategy({ schema: input.config.schema, configDir: dirname(input.configPath), database, table, }) + await assertBackfillTargetSafe({ + database, table, query: input.clickhouseQuery, querySettings: input.querySettings, + }) const replaySource = strategy.mvReplayQueries ? resolveMvReplaySource(strategy.mvs) : undefined const chunkSource = replaySource ?? { database, table } diff --git a/packages/plugin-backfill/src/plugin.ts b/packages/plugin-backfill/src/plugin.ts index 4fca65be..c519ad9c 100644 --- a/packages/plugin-backfill/src/plugin.ts +++ b/packages/plugin-backfill/src/plugin.ts @@ -30,7 +30,7 @@ import { type StatusOptions, } from './options.js' import { planPayload, statusPayload, cancelPayload, doctorPayload } from './payload.js' -import { buildBackfillPlan } from './planner.js' +import { assertBackfillTargetSafe, buildBackfillPlan } from './planner.js' import { evaluateBackfillCheck } from './check.js' import { cancelBackfillRun, getBackfillDoctorReport, getBackfillStatus } from './queries.js' import { @@ -116,6 +116,9 @@ async function runBackfill(input: { const db = createClickHouseExecutor(input.clickhouse) try { + const [database, table] = plan.target.split('.') + if (!database || !table) throw new BackfillConfigError('Invalid backfill target.') + await assertBackfillTargetSafe({ database, table, query: (sql, settings) => db.query(sql, settings) }) const runState: BackfillRunState = { planId: plan.planId, target: plan.target, diff --git a/packages/plugin-codegen/src/generators/type-artifacts.ts b/packages/plugin-codegen/src/generators/type-artifacts.ts index 5eb9017c..d3dacc8d 100644 --- a/packages/plugin-codegen/src/generators/type-artifacts.ts +++ b/packages/plugin-codegen/src/generators/type-artifacts.ts @@ -270,7 +270,7 @@ function renderTableInterface( ): { lines: string[]; findings: CodegenFinding[] } { const path = `${table.database}.${table.name}` const read = renderFieldsInterface( - table.columns.filter((column) => column.defaultKind !== 'EPHEMERAL'), + table.columns.filter((column) => !column.defaultKind || column.defaultKind === 'DEFAULT'), interfaceName, path, options ) const insertName = insertTypeName(table, interfaceName) @@ -279,7 +279,14 @@ function renderTableInterface( table.columns.filter((column) => column.defaultKind !== 'MATERIALIZED' && column.defaultKind !== 'ALIAS'), insertName, path, options ) - return { lines: [...read.lines, '', ...insert.lines], findings: [...read.findings, ...insert.findings] } + const explicit = renderFieldsInterface( + table.columns.filter((column) => column.defaultKind !== 'EPHEMERAL'), + `${interfaceName}Explicit`, path, options + ) + return { + lines: [...read.lines, '', ...explicit.lines, '', ...insert.lines], + findings: [...read.findings, ...explicit.findings, ...insert.findings], + } } function renderDictionaryInterface( From bcbea8dc841c119019d0c64e283066b9527621af Mon Sep 17 00:00:00 2001 From: KeKs0r Date: Sun, 27 Sep 2026 00:04:53 -0700 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=90=9B=20Harden=20expression=20checks?= =?UTF-8?q?=20and=20test=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vex-Session: session-3bdf6c82f39781ebe0b0ef8f --- bun.lock | 3 +++ .../src/chkit/cli/commands/drift_compare.py | 4 ++-- .../src/chkit/cli/commands/generate_reconcile.py | 4 ++-- chkit_python/src/chkit/clickhouse/introspect.py | 9 ++++++--- chkit_python/src/chkit/core/sql.py | 6 +++--- chkit_python/src/chkit_plugin_backfill/plugin.py | 15 ++++++++------- chkit_python/tests/test_column_expressions.py | 9 +++++++++ chkit_python/tests/test_column_expressions_e2e.py | 4 ++++ packages/cli/package.json | 3 +++ .../cli/src/test/column-expressions.e2e.test.ts | 8 ++++++++ .../clickhouse/src/column-expressions.test.ts | 7 +++++++ packages/clickhouse/src/index.ts | 9 +++++++-- 12 files changed, 62 insertions(+), 19 deletions(-) diff --git a/bun.lock b/bun.lock index 918afc4c..0e7431ee 100644 --- a/bun.lock +++ b/bun.lock @@ -59,6 +59,9 @@ "fast-glob": "^3.3.2", "p-retry": "^7.1.1", }, + "devDependencies": { + "@clickhouse/client": "^1.11.0", + }, "optionalDependencies": { "@chkit/plugin-obsessiondb": "workspace:*", }, diff --git a/chkit_python/src/chkit/cli/commands/drift_compare.py b/chkit_python/src/chkit/cli/commands/drift_compare.py index 8e5b4db2..829f60fa 100644 --- a/chkit_python/src/chkit/cli/commands/drift_compare.py +++ b/chkit_python/src/chkit/cli/commands/drift_compare.py @@ -29,7 +29,7 @@ TableDefinition, ) from chkit.core.projection import is_index_projection, normalize_projection_index -from chkit.core.sql import _render_default +from chkit.core.sql import render_default from chkit.core.sql_normalizer import normalize_engine, normalize_sql_fragment from chkit.core.text_index import render_text_index_type, text_index_fingerprint from chkit.core.text_index_sql import text_expression_fingerprint, text_sql_fingerprint @@ -220,7 +220,7 @@ def summarize_drift_reasons( def _normalize_column_shape(column: ColumnDefinition) -> str: normalized_default = ( "" if column.default is None - else text_sql_fingerprint(text_expression_fingerprint(_render_default(column.default))) + else text_sql_fingerprint(text_expression_fingerprint(render_default(column.default))) ) parts = [ diff --git a/chkit_python/src/chkit/cli/commands/generate_reconcile.py b/chkit_python/src/chkit/cli/commands/generate_reconcile.py index b1a3da69..78ebb9d2 100644 --- a/chkit_python/src/chkit/cli/commands/generate_reconcile.py +++ b/chkit_python/src/chkit/cli/commands/generate_reconcile.py @@ -3,7 +3,7 @@ from __future__ import annotations from chkit.core.canonical import canonicalize_definitions -from chkit.core.model import SchemaDefinition, TableDefinition +from chkit.core.model import ColumnDefinition, SchemaDefinition, TableDefinition def reconcile_column_expressions( @@ -35,7 +35,7 @@ def reconcile_column_expressions( raise ValueError( f"Cannot reconcile {key}: table must exist in both the snapshot and schema." ) - columns = [] + columns: list[ColumnDefinition] = [] for column in before.columns: updated = next((item for item in after.columns if item.name == column.name), None) columns.append( diff --git a/chkit_python/src/chkit/clickhouse/introspect.py b/chkit_python/src/chkit/clickhouse/introspect.py index ea8aae82..d76d3f1e 100644 --- a/chkit_python/src/chkit/clickhouse/introspect.py +++ b/chkit_python/src/chkit/clickhouse/introspect.py @@ -48,9 +48,10 @@ SkipIndexText, SkipIndexTokenBF, ) +from chkit.core.sql import render_default from chkit.core.sql_normalizer import normalize_sql_fragment from chkit.core.text_index import parse_text_index_params -from chkit.core.text_index_sql import normalize_text_index_sql +from chkit.core.text_index_sql import normalize_text_index_sql, text_sql_fingerprint SchemaObjectKind: TypeAlias = Literal["table", "view", "materialized_view", "dictionary"] @@ -155,8 +156,10 @@ def normalize_column_from_system_row(row: SystemColumnRow) -> ColumnDefinition: # Preserve whitespace inside SQL string literals when pulling expressions. default_value = row.default_expression.strip() - escaped_type = row.type.replace("'", "\\'") - if kind == "EPHEMERAL" and default_value == f"defaultValueOfTypeName('{escaped_type}')": + if kind == "EPHEMERAL" and default_value is not None and ( + text_sql_fingerprint(str(default_value)) + == text_sql_fingerprint(f"defaultValueOfTypeName({render_default(row.type)})") + ): default_value = None codec_steps = parse_codec(row.compression_codec) comment = row.comment.strip() if row.comment is not None else None diff --git a/chkit_python/src/chkit/core/sql.py b/chkit_python/src/chkit/core/sql.py index 36b892d0..5ca0e02b 100644 --- a/chkit_python/src/chkit/core/sql.py +++ b/chkit_python/src/chkit/core/sql.py @@ -58,7 +58,7 @@ def _normalize_projection(projection: ProjectionInput) -> ProjectionDefinition: return projection -def _render_default(value: str | int | float | bool) -> str: +def render_default(value: str | int | float | bool) -> str: if isinstance(value, str): if value.startswith("fn:"): return value[3:] @@ -73,7 +73,7 @@ def _render_column(col: ColumnDefinition) -> str: type_text = f"Nullable({col.type})" if col.nullable else f"{col.type}" out = f"`{col.name}` {type_text}" if col.default is not None: - out += f" {col.default_kind or 'DEFAULT'} {_render_default(col.default)}" + out += f" {col.default_kind or 'DEFAULT'} {render_default(col.default)}" elif col.default_kind == "EPHEMERAL": out += " EPHEMERAL" if col.comment is not None and len(col.comment) > 0: @@ -254,7 +254,7 @@ def _render_dictionary_attribute(attr: DictionaryAttribute) -> str: if attr.expression is not None: out += f" EXPRESSION {attr.expression}" elif attr.default is not None: - out += f" DEFAULT {_render_default(attr.default)}" + out += f" DEFAULT {render_default(attr.default)}" if attr.hierarchical: out += " HIERARCHICAL" if attr.bidirectional: diff --git a/chkit_python/src/chkit_plugin_backfill/plugin.py b/chkit_python/src/chkit_plugin_backfill/plugin.py index 6061b45e..729813bf 100644 --- a/chkit_python/src/chkit_plugin_backfill/plugin.py +++ b/chkit_python/src/chkit_plugin_backfill/plugin.py @@ -154,6 +154,13 @@ def query_status( def query(self, statement: str) -> object: return self._client().query(statement) + def query_rows( + self, statement: str, settings: QuerySettings | None = None, + ) -> list[dict[str, object]]: + return self._client().query( + statement, dict(settings) if settings is not None else None + ).rows + def close(self) -> None: with self._clients_lock: clients = list(self._clients) @@ -231,13 +238,7 @@ def _run_backfill( # noqa: PLR0915 — mirrors TS runBackfill try: database, table = plan.target.split(".") - assert_backfill_target_safe( - database=database, - table=table, - query=lambda sql, settings: ( - db._client().query(sql, dict(settings) if settings is not None else None).rows - ), - ) + assert_backfill_target_safe(database=database, table=table, query=db.query_rows) run_state = BackfillRunState( plan_id=plan.plan_id, target=plan.target, diff --git a/chkit_python/tests/test_column_expressions.py b/chkit_python/tests/test_column_expressions.py index 904da6f3..0a4b328e 100644 --- a/chkit_python/tests/test_column_expressions.py +++ b/chkit_python/tests/test_column_expressions.py @@ -299,3 +299,12 @@ def test_backfill_live_safety_gate(rows: list[dict[str, object]], message: str) assert_backfill_target_safe( database="default", table="events", query=lambda sql, settings: rows ) + + +def test_synthetic_ephemeral_default_escaped_type() -> None: + col = normalize_column_from_system_row(SystemColumnRow( + database="default", table="events", name="raw", type="Enum8('a\\b' = 1)", + position=1, default_kind="EPHEMERAL", + default_expression="defaultValueOfTypeName('Enum8(\\'a\\\\b\\' = 1)')", + )) + assert col.default is None diff --git a/chkit_python/tests/test_column_expressions_e2e.py b/chkit_python/tests/test_column_expressions_e2e.py index 5e4ec025..860fa1cb 100644 --- a/chkit_python/tests/test_column_expressions_e2e.py +++ b/chkit_python/tests/test_column_expressions_e2e.py @@ -21,6 +21,7 @@ ) from chkit.core.planner import plan_diff from chkit.core.sql import to_create_sql +from chkit_plugin_backfill.planner import assert_backfill_target_safe from chkit_plugin_codegen import generate_type_artifacts from tests.e2e_testkit import get_required_env @@ -71,6 +72,9 @@ def test_expression_column_lifecycle(ch_client: Any) -> None: order_by="id", ) assert compare_table_shape(definition, actual) is None + with pytest.raises(Exception, match="cannot reconstruct EPHEMERAL"): + assert_backfill_target_safe(database=database, table=name, + query=lambda sql, settings: list(client.query(sql).named_results())) pulled = _introspected_table_to_definition(actual) assert pulled is not None namespace: dict[str, Any] = {} diff --git a/packages/cli/package.json b/packages/cli/package.json index e1816efb..a7c7dbfa 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -48,5 +48,8 @@ }, "optionalDependencies": { "@chkit/plugin-obsessiondb": "workspace:*" + }, + "devDependencies": { + "@clickhouse/client": "^1.11.0" } } diff --git a/packages/cli/src/test/column-expressions.e2e.test.ts b/packages/cli/src/test/column-expressions.e2e.test.ts index 3226fe17..921801ef 100644 --- a/packages/cli/src/test/column-expressions.e2e.test.ts +++ b/packages/cli/src/test/column-expressions.e2e.test.ts @@ -15,6 +15,8 @@ import { generateIngestArtifacts, } from '../../../plugin-codegen/src/index.js' import { createFixture, runCli } from './testkit.test.js' +import { buildBackfillPlan } from '../../../plugin-backfill/src/planner.js' +import { PlanSchema } from '../../../plugin-backfill/src/options.js' import { getRequiredEnv } from './e2e-testkit.js' test('column expressions survive create, pull, drift, inserts and ALTER on live ClickHouse', async () => { @@ -87,6 +89,12 @@ test('column expressions survive create, pull, drift, inserts and ALTER on live try { await client.command({ query: toCreateSQL(def) }) expect(compareTableShape(def, await actual())).toBeNull() + await expect(buildBackfillPlan({ + opts: PlanSchema.parse({ target: `${def.database}.${name}` }), + configPath: join(dir, 'config.ts'), + config: { metaDir: join(dir, 'meta'), schema: [join(dir, 'missing.ts')] }, + clickhouseQuery: query, + })).rejects.toThrow('cannot reconstruct EPHEMERAL inputs') const pulled = { ...def, columns: (await columns()).map((column) => ({ diff --git a/packages/clickhouse/src/column-expressions.test.ts b/packages/clickhouse/src/column-expressions.test.ts index 29d2c9c3..9a6a5445 100644 --- a/packages/clickhouse/src/column-expressions.test.ts +++ b/packages/clickhouse/src/column-expressions.test.ts @@ -49,3 +49,10 @@ test('unknown expression kinds fail explicitly instead of losing metadata', () = }), ).toThrow('Unsupported column default kind') }) + +test('synthetic EPHEMERAL defaults compare SQL literals with quotes and backslashes', () => { + const type = "Enum8('a\\b' = 1)" + const expression = "defaultValueOfTypeName('Enum8(\\'a\\\\b\\' = 1)')" + const column = normalizeColumnFromSystemRow({ database: 'default', table: 'events', name: 'raw', type, position: 1, default_kind: 'EPHEMERAL', default_expression: expression }) + expect(column.default).toBeUndefined() +}) diff --git a/packages/clickhouse/src/index.ts b/packages/clickhouse/src/index.ts index 8666ff42..5f1eff2c 100644 --- a/packages/clickhouse/src/index.ts +++ b/packages/clickhouse/src/index.ts @@ -3,6 +3,8 @@ import { type ChxConfig, type ColumnDefinition, normalizeSQLFragment, + renderDefault, + sqlExpressionFingerprint, type ProjectionDefinition, parseCodec, type SkipIndexDefinition, @@ -201,8 +203,11 @@ export function normalizeColumnFromSystemRow( defaultValue = row.default_expression.trim() } // ClickHouse synthesizes this expression for a bare EPHEMERAL column. - const implicitEphemeralDefault = `defaultValueOfTypeName('${row.type.replace(/'/g, "\\'")}')` - if (defaultKind === 'EPHEMERAL' && defaultValue === implicitEphemeralDefault) { + if ( + defaultKind === 'EPHEMERAL' && defaultValue !== undefined && + sqlExpressionFingerprint(String(defaultValue)) === + sqlExpressionFingerprint(`defaultValueOfTypeName(${renderDefault(row.type)})`) + ) { defaultValue = undefined } const codecSteps = parseCodec(row.compression_codec) From c07a2a10d7f66c5cb07915a210ebc0574b493815 Mon Sep 17 00:00:00 2001 From: KeKs0r Date: Sun, 27 Sep 2026 11:50:52 -0700 Subject: [PATCH 5/5] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Defer=20snapshot=20rec?= =?UTF-8?q?onciliation=20workflow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vex-Session: session-3bdf6c82f39781ebe0b0ef8f --- .changeset/column-expression-kinds.md | 4 +- apps/docs/src/content/docs/cli/generate.md | 34 ------ .../src/content/docs/schema/dsl-reference.mdx | 7 +- chkit_python/CHANGELOG.md | 4 +- .../src/chkit/cli/commands/generate.py | 106 ++++-------------- .../chkit/cli/commands/generate_reconcile.py | 56 --------- chkit_python/src/chkit/core/planner.py | 49 +++++--- .../src/chkit_plugin_backfill/plugin.py | 17 ++- chkit_python/tests/test_column_expressions.py | 26 +---- .../tests/test_column_expressions_e2e.py | 73 ------------ packages/cli/src/commands/generate/command.ts | 35 +----- .../cli/src/commands/generate/reconcile.ts | 56 --------- .../src/test/column-expression-safety.test.ts | 28 ----- .../src/test/column-expressions.e2e.test.ts | 75 +------------ packages/core/src/column-expressions.test.ts | 4 +- packages/core/src/planner.ts | 2 +- 16 files changed, 84 insertions(+), 492 deletions(-) delete mode 100644 chkit_python/src/chkit/cli/commands/generate_reconcile.py delete mode 100644 packages/cli/src/commands/generate/reconcile.ts diff --git a/.changeset/column-expression-kinds.md b/.changeset/column-expression-kinds.md index 221eb688..9d5fa574 100644 --- a/.changeset/column-expression-kinds.md +++ b/.changeset/column-expression-kinds.md @@ -10,6 +10,6 @@ Support `MATERIALIZED`, `ALIAS`, and `EPHEMERAL` column expressions with `defaultKind`, preserving kinds through SQL rendering, pull, snapshots, and drift. Keep existing defaults and snapshots stable; use the existing `fn:` prefix for SQL expressions and allow expressionless `EPHEMERAL` columns. -Generate separate row and insert shapes for tables with special column kinds. Exclude generated columns from automatic backfill insert projections and reject automatic backfills that cannot reconstruct ephemeral inputs. Emit explicit removal of stored expressions, require manual migrations for storage-kind conversions involving `ALIAS` or `EPHEMERAL`, and never automatically rewrite historical materialized values. +Generate separate row and insert shapes for tables with special column kinds. Exclude generated columns from automatic backfill insert projections and reject automatic backfills that cannot reconstruct ephemeral inputs. Emit explicit removal of stored expressions, reject automatic storage-kind conversions involving `ALIAS` or `EPHEMERAL`, and never automatically rewrite historical materialized values. -Compare defaults with quote-aware SQL tokens, preserving literal whitespace and distinguishing SQL expressions from string literals. Generate `Row` for default `SELECT *`, `RowExplicit` for all readable columns, and `RowInsert` for writes. Verify live column kinds during backfill planning and local execution, fail closed on unavailable metadata, and provide `generate --reconcile --table` for verified snapshot adoption after manual column migrations. Surface historical-value warnings in CLI output and migration files. +Compare defaults with quote-aware SQL tokens, preserving literal whitespace and distinguishing SQL expressions from string literals. Generate `Row` for default `SELECT *`, `RowExplicit` for all readable columns, and `RowInsert` for writes. Verify live column kinds during backfill planning and local execution, and fail closed on unavailable metadata. Surface historical-value warnings in CLI output and migration files. diff --git a/apps/docs/src/content/docs/cli/generate.md b/apps/docs/src/content/docs/cli/generate.md index 471f9354..ba0ce188 100644 --- a/apps/docs/src/content/docs/cli/generate.md +++ b/apps/docs/src/content/docs/cli/generate.md @@ -24,7 +24,6 @@ chkit generate [flags] | `--rename-dictionary ` | string | — | Explicit dictionary rename: `old_db.old_dict=new_db.new_dict` | | `--table ` | string | — | Scope operations to matching tables | | `--dryrun` | boolean | `false` | Print the plan without writing any files | -| `--reconcile` | boolean | `false` | Verify live column expressions and reconcile their snapshot; requires `--table` | | `--empty` | boolean | `false` | Scaffold a blank manual migration without diffing the schema | Global flags documented on [CLI Overview](/cli/overview/#global-flags). @@ -95,39 +94,6 @@ The stub carries the standard migration header (with `operation-count: 0`) plus `chkit migrate` picks the file up like any other migration and applies it in filename order. Write your SQL into the stub *before* applying it — editing a migration after it has run triggers a checksum mismatch. -### Reconcile manual column changes - -When changing a column to or from `ALIAS` or `EPHEMERAL`, write and review an explicit -migration that handles the storage change. Preserve any stored values you need -before converting a column to a non-stored kind. - -1. Update the column's expression/kind in the schema. Keep other edits to that table - for a separate migration. -2. Create a manual SQL file in the configured `migrationsDir`. In TypeScript, - `chkit generate --empty --name convert_column` creates the stub. In Python, - create a timestamped `.sql` file there directly. Include any cluster clauses - required by your deployment; manual SQL runs verbatim. -3. Review and apply the migration through `chkit migrate --apply`. -4. Preview snapshot reconciliation with - `chkit generate --reconcile --table analytics.events --dryrun`. -5. Run `chkit generate --reconcile --table analytics.events`, then regenerate models - with `chkit codegen` if you use the codegen plugin. Commit the schema, migration, - snapshot, and regenerated models together. - -`--reconcile` works in both languages and requires a live ClickHouse connection, -`--table`, and an existing snapshot. It verifies the selected tables against the -current schema before writing anything. Only column expression/kind metadata is -adopted: other schema changes are rejected, and unselected snapshot objects are -preserved. It generates no SQL and cannot be combined with rename flags or -`--empty`. `--dryrun` verifies the live schema without updating the snapshot. - -A missing table, metadata query failure, or schema mismatch leaves the snapshot -unchanged. After successful reconciliation, another `generate --dryrun` should show -no operations for the converted column. This verifies schema metadata, not the -correctness or completeness of your data conversion; verify the data before -reconciling. Run reconciliation against the intended migration environment and -avoid concurrent DDL during the check. - ### Codegen integration If the codegen plugin is configured with `runOnGenerate: true` (the default), `chkit generate` automatically runs codegen after writing migration artifacts. A codegen failure causes `generate` to fail. diff --git a/apps/docs/src/content/docs/schema/dsl-reference.mdx b/apps/docs/src/content/docs/schema/dsl-reference.mdx index 63b5c64e..f68e9a63 100644 --- a/apps/docs/src/content/docs/schema/dsl-reference.mdx +++ b/apps/docs/src/content/docs/schema/dsl-reference.mdx @@ -334,9 +334,10 @@ COLUMN ...` only when you need that rewrite and the required inputs still exist. Values derived from discarded `EPHEMERAL` inputs cannot be reconstructed this way. Removing a `DEFAULT` or `MATERIALIZED` expression emits an explicit `REMOVE` clause. -Automatic kind conversions involving `ALIAS` or `EPHEMERAL` are rejected. Use a -[manual migration and verified snapshot reconciliation](/cli/generate/#reconcile-manual-column-changes). -The planner never silently drops and recreates a column for these conversions. +Converting an existing column to or from `ALIAS` or `EPHEMERAL` is not supported +by the migration generator. This release does not provide snapshot adoption for +manually applied conversions. The planner rejects these changes instead of dropping +and recreating columns. Generated `Row` models describe the default `SELECT *` result, excluding `MATERIALIZED`, `ALIAS`, and `EPHEMERAL`. Tables using special kinds also get: diff --git a/chkit_python/CHANGELOG.md b/chkit_python/CHANGELOG.md index 4fd36049..e3545f76 100644 --- a/chkit_python/CHANGELOG.md +++ b/chkit_python/CHANGELOG.md @@ -6,7 +6,7 @@ - Compare column defaults with quote-aware SQL tokens and correctly escape literal backslashes. - Generate separate `Row` (default `SELECT *`), `RowExplicit`, and `RowInsert` models. - Check live column metadata before backfill planning and local execution; block unknown metadata and unrecoverable `EPHEMERAL` inputs. -- Warn about unchanged historical values in migration output and SQL. Support `generate --reconcile --table` to verify manually applied column expression/kind changes and adopt only those snapshot changes. +- Warn about unchanged historical values in migration output and SQL. - Add `SkipIndexText` for full-text index generation, introspection, pull, and drift. Preserve quoted SQL literals, normalize ClickHouse’s fixed granularity, and reject @@ -26,7 +26,7 @@ - Support `default_kind` / `defaultKind` for `DEFAULT`, `MATERIALIZED`, `ALIAS`, and `EPHEMERAL` columns through SQL rendering, introspection, pull, snapshots, and drift. Existing defaults and snapshots remain compatible. Use `fn:` for SQL expressions; expressionless `EPHEMERAL` is supported. - Generate separate read/insert models for tables with special column kinds, and make backfill projections respect generated columns. Automatic backfills with ephemeral inputs require explicit SQL input mappings. -- Emit explicit removal of stored column expressions; require manual migrations for kind conversions involving `ALIAS` or `EPHEMERAL`. Expression changes never automatically materialize historical data. +- Emit explicit removal of stored column expressions; reject automatic kind conversions involving `ALIAS` or `EPHEMERAL`. Expression changes never automatically materialize historical data. ## 0.2.0 — 2026-08-10 diff --git a/chkit_python/src/chkit/cli/commands/generate.py b/chkit_python/src/chkit/cli/commands/generate.py index ea53dffc..de54792a 100644 --- a/chkit_python/src/chkit/cli/commands/generate.py +++ b/chkit_python/src/chkit/cli/commands/generate.py @@ -23,7 +23,6 @@ from chkit.cli.commands.dictionary_password_warnings import ( detect_dictionary_password_warnings, ) -from chkit.cli.commands.drift_compare import compare_table_shape from chkit.cli.commands.generate_plan_pipeline import ( apply_explicit_dictionary_renames, apply_explicit_table_renames, @@ -31,7 +30,6 @@ assert_cli_column_mappings_resolvable, build_explicit_column_rename_suggestions, ) -from chkit.cli.commands.generate_reconcile import reconcile_column_expressions from chkit.cli.commands.generate_rename_mappings import ( ColumnRenameMapping, DictionaryRenameMapping, @@ -68,16 +66,8 @@ resolve_table_scope, table_keys_from_definitions, ) -from chkit.clickhouse.client import ClickHouseClient -from chkit.clickhouse.introspect import list_table_details from chkit.core.canonical import canonicalize_definitions -from chkit.core.model import ( - ChxConfigEnv, - ChxResolvedConfig, - ChxValidationError, - SchemaDefinition, - TableDefinition, -) +from chkit.core.model import ChxConfigEnv, ChxResolvedConfig, ChxValidationError, SchemaDefinition from chkit.core.on_cluster import apply_on_cluster_to_plan from chkit.core.planner import plan_diff from chkit.core.snapshot import create_snapshot @@ -142,7 +132,10 @@ def _run_codegen_integration( ) exit_code = plugin_runtime.run_plugin_command("codegen", "codegen", ctx) if exit_code != 0: - msg = f'Plugin "codegen" failed in generate integration with exit code {exit_code}.' + msg = ( + f'Plugin "codegen" failed in generate integration with exit ' + f"code {exit_code}." + ) raise typer.Exit(code=1) from RuntimeError(msg) @@ -249,14 +242,20 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 list[str] | None, typer.Option( "--rename-table", - help=("Explicit table rename mapping old_db.old_table=new_db.new_table. Repeatable."), + help=( + "Explicit table rename mapping old_db.old_table=new_db.new_table. " + "Repeatable." + ), ), ] = None, rename_column: Annotated[ list[str] | None, typer.Option( "--rename-column", - help=("Explicit column rename mapping db.table.old_column=new_column. Repeatable."), + help=( + "Explicit column rename mapping db.table.old_column=new_column. " + "Repeatable." + ), ), ] = None, rename_dictionary: Annotated[ @@ -264,18 +263,11 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 typer.Option( "--rename-dictionary", help=( - "Explicit dictionary rename mapping old_db.old_dict=new_db.new_dict. Repeatable." + "Explicit dictionary rename mapping old_db.old_dict=new_db.new_dict. " + "Repeatable." ), ), ] = None, - reconcile: Annotated[ - bool, - typer.Option( - "--reconcile", - help=("Verify live column expressions and update their snapshot " - "after a manual migration (requires --table)."), - ), - ] = False, dryrun: Annotated[ bool, typer.Option("--dryrun", help="Print plan without writing artifacts."), @@ -285,10 +277,6 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 typer.Option("--json", help="Emit a JSON-formatted summary."), ] = False, ) -> None: - if reconcile and (not table_selector or rename_column or rename_table or rename_dictionary): - raise typer.BadParameter( - "--reconcile requires --table and cannot be combined with rename flags." - ) config = load_config(config_path, ChxConfigEnv(command="generate")) plugin_runtime = load_plugin_runtime( [p for p in config.plugins if isinstance(p, ChxPlugin)] @@ -342,62 +330,6 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 previous = read_snapshot(meta_dir) old_defs = list(previous.definitions) if previous is not None else [] - if reconcile: - if previous is None: - raise typer.BadParameter( - "Snapshot not found; reconciliation requires an existing snapshot." - ) - scope = resolve_table_scope(table_selector, table_keys_from_definitions(canonical)) - if not scope.match_count: - raise typer.BadParameter("No tables matched --table; snapshot unchanged.") - reconciled = reconcile_column_expressions(old_defs, canonical, list(scope.matched_tables)) - if config.clickhouse is None: - raise typer.BadParameter("clickhouse config is required for --reconcile.") - selected = [ - item - for item in canonical - if isinstance(item, TableDefinition) - and f"{item.database}.{item.name}" in scope.matched_tables - ] - with ClickHouseClient.connect(config.clickhouse) as client: - actual = list_table_details(client, sorted({item.database for item in selected})) - for expected in selected: - live = next( - ( - item - for item in actual - if item.database == expected.database and item.name == expected.name - ), - None, - ) - if live is None or compare_table_shape(expected, live): - raise typer.BadParameter( - f"Live table {expected.database}.{expected.name} does not match the schema; " - "apply and verify the manual migration before --reconcile. Snapshot unchanged." - ) - snapshot_path = meta_dir / "snapshot.json" - if not dryrun: - write_snapshot(meta_dir, create_snapshot(reconciled)) - if output_json: - typer.echo( - json.dumps( - { - "mode": "reconcile", - "verified": True, - "dryrun": dryrun, - "snapshotFile": str(snapshot_path), - "tables": scope.matched_tables, - } - ) - ) - else: - typer.echo( - f"{'Verified' if dryrun else 'Reconciled'} column expressions " - "against live ClickHouse. " - "No migration generated." - ) - return - ( remapped_old_defs, active_table_mappings, @@ -418,7 +350,9 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 ) table_scope = resolve_table_scope(table_selector, available_keys) if table_scope.enabled and table_scope.match_count == 0: - warning = f'No tables matched selector "{table_scope.selector or ""}". No changes planned.' + warning = ( + f'No tables matched selector "{table_scope.selector or ""}". No changes planned.' + ) if output_json: typer.echo( json.dumps( @@ -491,7 +425,9 @@ def run( # noqa: PLR0911, PLR0912, PLR0915, PLR0917 # filtering) — so plugin-injected SQL is also covered. ``migrate`` never # re-runs this: the clause is baked into the migration file at generate # time and applied verbatim. - plan = apply_on_cluster_to_plan(plan, config.clickhouse.cluster if config.clickhouse else None) + plan = apply_on_cluster_to_plan( + plan, config.clickhouse.cluster if config.clickhouse else None + ) dictionary_password_warnings = detect_dictionary_password_warnings(plan) + [ op.warning for op in plan.operations if op.warning diff --git a/chkit_python/src/chkit/cli/commands/generate_reconcile.py b/chkit_python/src/chkit/cli/commands/generate_reconcile.py deleted file mode 100644 index 78ebb9d2..00000000 --- a/chkit_python/src/chkit/cli/commands/generate_reconcile.py +++ /dev/null @@ -1,56 +0,0 @@ -"""Narrow snapshot adoption after a manually applied column expression migration.""" - -from __future__ import annotations - -from chkit.core.canonical import canonicalize_definitions -from chkit.core.model import ColumnDefinition, SchemaDefinition, TableDefinition - - -def reconcile_column_expressions( - previous: list[SchemaDefinition], - next_: list[SchemaDefinition], - selected: list[str], -) -> list[SchemaDefinition]: - current = canonicalize_definitions(next_) - result = canonicalize_definitions(previous) - for key in selected: - index = next( - ( - i - for i, item in enumerate(result) - if isinstance(item, TableDefinition) and f"{item.database}.{item.name}" == key - ), - None, - ) - before = result[index] if index is not None else None - after = next( - ( - item - for item in current - if isinstance(item, TableDefinition) and f"{item.database}.{item.name}" == key - ), - None, - ) - if not isinstance(before, TableDefinition) or after is None or index is None: - raise ValueError( - f"Cannot reconcile {key}: table must exist in both the snapshot and schema." - ) - columns: list[ColumnDefinition] = [] - for column in before.columns: - updated = next((item for item in after.columns if item.name == column.name), None) - columns.append( - column.model_copy( - update={ - "default": updated.default if updated else None, - "default_kind": updated.default_kind if updated else None, - } - ) - ) - candidate = before.model_copy(update={"columns": columns}) - if canonicalize_definitions([candidate]) != [after]: - raise ValueError( - f"Cannot reconcile {key}: --reconcile accepts only column expression/kind changes. " - "Generate other schema changes separately." - ) - result[index] = candidate - return result diff --git a/chkit_python/src/chkit/core/planner.py b/chkit_python/src/chkit/core/planner.py index e13e6ddc..f5e86378 100644 --- a/chkit_python/src/chkit/core/planner.py +++ b/chkit_python/src/chkit/core/planner.py @@ -81,7 +81,10 @@ def _push_drop( type="drop_dictionary", key=definition_key(definition), risk=risk, - sql=(f"DROP DICTIONARY IF EXISTS {definition.database}.{definition.name};"), + sql=( + f"DROP DICTIONARY IF EXISTS " + f"{definition.database}.{definition.name};" + ), ) ) return @@ -245,8 +248,13 @@ def _is_codec_removal(old: ColumnDefinition, new: ColumnDefinition) -> bool: return _column_identity_without_codec(old) == _column_identity_without_codec(new) -def _render_rename_column_suggestion_sql(table: TableDefinition, from_: str, to: str) -> str: - return f"ALTER TABLE {table.database}.{table.name} RENAME COLUMN `{from_}` TO `{to}`;" +def _render_rename_column_suggestion_sql( + table: TableDefinition, from_: str, to: str +) -> str: + return ( + f"ALTER TABLE {table.database}.{table.name} " + f"RENAME COLUMN `{from_}` TO `{to}`;" + ) def _infer_column_rename_suggestions( @@ -438,8 +446,7 @@ def _diff_tables( f"Cannot automatically change column {new.database}.{new.name}." f"{column_change.name} " f"from {old_kind} to {new_kind}; " - "use an explicit manual migration for storage-kind changes, then run " - f"generate --reconcile --table {new.database}.{new.name} after applying it" + "storage-kind conversions involving ALIAS or EPHEMERAL are not supported" ) sql = ( render_alter_remove_codec(new, column_change.name) @@ -523,10 +530,10 @@ def _diff_tables( list(old.projections or []), list(new.projections or []), lambda p: p.name, - lambda left, right: ( - json.dumps(left.model_dump(mode="json"), sort_keys=True, default=str) - == json.dumps(right.model_dump(mode="json"), sort_keys=True, default=str) - ), + lambda left, right: json.dumps( + left.model_dump(mode="json"), sort_keys=True, default=str + ) + == json.dumps(right.model_dump(mode="json"), sort_keys=True, default=str), ) for projection in projection_diff.added: ops.append( @@ -541,7 +548,10 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_drop_projection", - key=(f"table:{new.database}.{new.name}:projection:{projection_change.name}"), + key=( + f"table:{new.database}.{new.name}:projection:" + f"{projection_change.name}" + ), risk="caution", sql=render_alter_drop_projection(new, projection_change.name), ) @@ -549,7 +559,10 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_add_projection", - key=(f"table:{new.database}.{new.name}:projection:{projection_change.name}"), + key=( + f"table:{new.database}.{new.name}:projection:" + f"{projection_change.name}" + ), risk="caution", sql=render_alter_add_projection(new, projection_change.new_item), ) @@ -570,7 +583,10 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_reset_setting", - key=(f"table:{new.database}.{new.name}:setting:{setting_change.key}"), + key=( + f"table:{new.database}.{new.name}:setting:" + f"{setting_change.key}" + ), risk="caution", sql=render_alter_reset_setting(new, setting_change.key), ) @@ -579,9 +595,14 @@ def _diff_tables( ops.append( MigrationOperation( type="alter_table_modify_setting", - key=(f"table:{new.database}.{new.name}:setting:{setting_change.key}"), + key=( + f"table:{new.database}.{new.name}:setting:" + f"{setting_change.key}" + ), risk="caution", - sql=render_alter_modify_setting(new, setting_change.key, setting_change.value), + sql=render_alter_modify_setting( + new, setting_change.key, setting_change.value + ), ) ) diff --git a/chkit_python/src/chkit_plugin_backfill/plugin.py b/chkit_python/src/chkit_plugin_backfill/plugin.py index 729813bf..f316853f 100644 --- a/chkit_python/src/chkit_plugin_backfill/plugin.py +++ b/chkit_python/src/chkit_plugin_backfill/plugin.py @@ -427,7 +427,8 @@ def clickhouse_query( sort_keys = output.plan.chunk_plan.table.sort_keys primary_sort_key = sort_keys[0] if sort_keys else None sort_key_label = ( - f", sort key: {primary_sort_key.name} ({primary_sort_key.category})" + f", sort key: {primary_sort_key.name}" + f" ({primary_sort_key.category})" if primary_sort_key is not None else "" ) @@ -618,7 +619,8 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ChxPluginCommand( name="plan", description=( - "Build a deterministic backfill plan and persist immutable plan state" + "Build a deterministic backfill plan and persist immutable" + " plan state" ), run=_guarded(_plan, "plan", "Backfill plan"), flags=list(PLAN_FLAGS), @@ -634,7 +636,10 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ), ChxPluginCommand( name="run", - description=("Execute a planned backfill with async query submission and polling"), + description=( + "Execute a planned backfill with async query submission" + " and polling" + ), run=_guarded(_run, "run", "Backfill run"), flags=list(RUN_FLAGS), ), @@ -653,7 +658,8 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ChxPluginCommand( name="cancel", description=( - "Cancel an in-progress backfill run and prevent further chunk execution" + "Cancel an in-progress backfill run and prevent further" + " chunk execution" ), run=_guarded(_cancel, "cancel", "Backfill cancel"), flags=list(PLAN_ID_FLAGS), @@ -661,7 +667,8 @@ def wrapped(ctx: ChxPluginCommandContext) -> int: ChxPluginCommand( name="doctor", description=( - "Provide actionable remediation steps for failed or pending backfill runs" + "Provide actionable remediation steps for failed or pending" + " backfill runs" ), run=_guarded(_doctor, "doctor", "Backfill doctor"), flags=list(PLAN_ID_FLAGS), diff --git a/chkit_python/tests/test_column_expressions.py b/chkit_python/tests/test_column_expressions.py index 0a4b328e..2215b9df 100644 --- a/chkit_python/tests/test_column_expressions.py +++ b/chkit_python/tests/test_column_expressions.py @@ -9,7 +9,6 @@ from chkit import ColumnDefinition, table from chkit.cli.commands.drift_compare import compare_table_shape -from chkit.cli.commands.generate_reconcile import reconcile_column_expressions from chkit.cli.commands.pull import _introspected_table_to_definition from chkit.cli.commands.pull_render import render_schema_file from chkit.clickhouse.introspect import ( @@ -135,11 +134,11 @@ def test_remove_expression_with_type_change(kind: str) -> None: @pytest.mark.parametrize("kind", ["ALIAS", "EPHEMERAL"]) -def test_storage_kind_changes_require_manual_migration(kind: str) -> None: +def test_storage_kind_changes_are_rejected(kind: str) -> None: virtual = definition(default_kind=kind, default="fn:toDate(ts)") - with pytest.raises(ValueError, match="explicit manual migration"): + with pytest.raises(ValueError, match="storage-kind conversions involving ALIAS or EPHEMERAL are not supported"): plan_diff([definition()], [virtual]) - with pytest.raises(ValueError, match="explicit manual migration"): + with pytest.raises(ValueError, match="storage-kind conversions involving ALIAS or EPHEMERAL are not supported"): plan_diff([virtual], [definition()]) @@ -255,25 +254,6 @@ def test_expression_comparison_preserves_literals(expected: Any, actual: Any, eq assert (result is None) == equal -def test_reconcile_only_selected_expression_metadata() -> None: - - before = definition(default="fn:toDate(ts)") - after = definition(default="fn:toDate(ts)", default_kind="ALIAS") - unrelated = before.model_copy(update={"name": "other"}) - reconciled = reconcile_column_expressions( - [before, unrelated], - [after, unrelated.model_copy(update={"engine": "Log"})], - ["default.events"], - ) - assert plan_diff(reconciled, [after, unrelated]).operations == [] - with pytest.raises(ValueError, match="only column expression/kind changes"): - reconcile_column_expressions( - [before], [after.model_copy(update={"engine": "Log"})], ["default.events"] - ) - with pytest.raises(ValueError, match="both the snapshot and schema"): - reconcile_column_expressions([], [after], ["default.events"]) - - def test_stored_expression_warning() -> None: plan = plan_diff([definition(default="fn:toDate(ts)")], [definition(default="fn:today()")]) assert "does not rewrite stored historical values" in (plan.operations[0].warning or "") diff --git a/chkit_python/tests/test_column_expressions_e2e.py b/chkit_python/tests/test_column_expressions_e2e.py index 860fa1cb..b025ef12 100644 --- a/chkit_python/tests/test_column_expressions_e2e.py +++ b/chkit_python/tests/test_column_expressions_e2e.py @@ -2,18 +2,15 @@ from __future__ import annotations -import json from typing import Any from uuid import uuid4 import pytest -from typer.testing import CliRunner from chkit import table from chkit.cli.commands.drift_compare import compare_table_shape from chkit.cli.commands.pull import _introspected_table_to_definition from chkit.cli.commands.pull_render import render_schema_file -from chkit.cli.main import app from chkit.clickhouse.introspect import ( IntrospectedTable, SystemColumnRow, @@ -23,7 +20,6 @@ from chkit.core.sql import to_create_sql from chkit_plugin_backfill.planner import assert_backfill_target_safe from chkit_plugin_codegen import generate_type_artifacts -from tests.e2e_testkit import get_required_env def test_expression_column_lifecycle(ch_client: Any) -> None: @@ -127,72 +123,3 @@ def test_expression_column_lifecycle(ch_client: Any) -> None: ).result_rows == [("",)] finally: client.command(f"DROP TABLE IF EXISTS {target} SYNC") - - -def test_manual_conversion_reconciliation(ch_client: Any, tmp_path: Any, monkeypatch: Any) -> None: - - - - env = get_required_env() - client = ch_client._client - name = f"reconcile_py_{uuid4().hex}" - before = table( - database=env.clickhouse_database, - name=name, - engine="MergeTree()", - primary_key=["id"], - order_by=["id"], - columns=[ - {"name": "id", "type": "UInt32"}, - {"name": "label", "type": "String", "default": "fn:toString(id)"}, - ], - ) - after = before.model_copy( - update={ - "columns": [ - before.columns[0], - before.columns[1].model_copy(update={"default_kind": "ALIAS"}), - ] - } - ) - monkeypatch.chdir(tmp_path) - config = { - "schema": "./schema.py", - "metaDir": "./meta", - "migrationsDir": "./migrations", - "clickhouse": { - "url": env.clickhouse_url, - "username": env.clickhouse_user, - "password": env.clickhouse_password, - "database": env.clickhouse_database, - }, - } - (tmp_path / "clickhouse.config.py").write_text( - f"from chkit import define_config\nconfig = define_config({config!r})\n" - ) - (tmp_path / "schema.py").write_text(render_schema_file([before])) - runner = CliRunner() - try: - assert runner.invoke(app, ["generate", "--json"], catch_exceptions=False).exit_code == 0 - client.command(to_create_sql(before)) - snapshot = tmp_path / "meta" / "snapshot.json" - original = snapshot.read_text() - (tmp_path / "schema.py").write_text(render_schema_file([after])) - args = ["generate", "--reconcile", "--table", f"{before.database}.{name}", "--json"] - result = runner.invoke(app, args) - assert result.exit_code != 0 - assert snapshot.read_text() == original - client.command( - f"ALTER TABLE {before.database}.{name} DROP COLUMN label, ADD COLUMN label String ALIAS toString(id)" - ) - result = runner.invoke(app, [*args, "--dryrun"], catch_exceptions=False) - assert result.exit_code == 0, result.output - assert snapshot.read_text() == original - result = runner.invoke(app, args, catch_exceptions=False) - assert result.exit_code == 0, result.output - assert json.loads(result.output)["verified"] is True - result = runner.invoke(app, ["generate", "--dryrun", "--json"], catch_exceptions=False) - assert result.exit_code == 0, result.output - assert json.loads(result.output)["operationCount"] == 0 - finally: - client.command(f"DROP TABLE IF EXISTS {before.database}.{name} SYNC") diff --git a/packages/cli/src/commands/generate/command.ts b/packages/cli/src/commands/generate/command.ts index 5860c0c6..d28b62cd 100644 --- a/packages/cli/src/commands/generate/command.ts +++ b/packages/cli/src/commands/generate/command.ts @@ -1,9 +1,5 @@ -import { writeFile } from 'node:fs/promises' -import { join } from 'node:path' -import { compareTableShape } from '../drift/compare.js' -import { reconcileColumnExpressions } from './reconcile.js' import { generateArtifacts, generateEmptyMigration } from '@chkit/codegen' -import { applyOnClusterToPlan, assertValidDefinitions, createSnapshot, ChxValidationError, planDiff } from '@chkit/core' +import { applyOnClusterToPlan, ChxValidationError, planDiff } from '@chkit/core' import { defineFlags, typedFlags, type ChxPluginCommand } from '../../plugins.js' import { resolveDirs } from '../../runtime/config.js' @@ -54,7 +50,6 @@ const GENERATE_FLAGS = defineFlags([ { name: '--rename-column', type: 'string[]', description: 'Explicit column rename mapping', placeholder: '' }, { name: '--rename-dictionary', type: 'string[]', description: 'Explicit dictionary rename mapping', placeholder: '' }, { name: '--dryrun', type: 'boolean', description: 'Print plan without writing artifacts' }, - { name: '--reconcile', type: 'boolean', description: 'Verify live column expressions and update their snapshot after a manual migration (requires --table)' }, { name: '--empty', type: 'boolean', description: 'Scaffold a blank manual migration (no schema diff, snapshot untouched)' }, ] as const) @@ -75,10 +70,6 @@ async function cmdGenerate(ctx: import('../../plugins.js').ChxPluginCommandConte const planMode = f['--dryrun'] === true const jsonMode = f['--json'] === true const emptyMode = f['--empty'] === true - const reconcileMode = f['--reconcile'] === true - if (reconcileMode && (!tableSelector || emptyMode || f['--rename-column'] || f['--rename-table'] || f['--rename-dictionary'])) { - throw new Error('--reconcile requires --table and cannot be combined with --empty or rename flags.') - } debug('generate', `flags: name=${migrationName ?? '(auto)'}, dryrun=${planMode}, json=${jsonMode}, empty=${emptyMode}`) @@ -115,30 +106,6 @@ async function cmdGenerate(ctx: import('../../plugins.js').ChxPluginCommandConte definitions, }) - if (reconcileMode) { - assertValidDefinitions(definitions) - const previous = await readSnapshot(dirs.metaDir) - if (!previous) throw new Error('Snapshot not found; reconciliation requires an existing snapshot.') - const scope = resolveTableScope(tableSelector, tableKeysFromDefinitions(definitions)) - if (!scope.matchCount) throw new Error('No tables matched --table; snapshot unchanged.') - const reconciled = reconcileColumnExpressions(previous.definitions, definitions, scope.matchedTables) - if (!ctx.pluginContext.hasExecutor) throw new Error('clickhouse config is required for --reconcile.') - const selected = definitions.filter((item) => item.kind === 'table' && scope.matchedTables.includes(`${item.database}.${item.name}`)) - const actual = await ctx.pluginContext.executor.listTableDetails([...new Set(selected.map((item) => item.database))]) - for (const expected of selected) { - if (expected.kind !== 'table') continue - const live = actual.find((item) => item.database === expected.database && item.name === expected.name) - if (!live || compareTableShape(expected, live)) { - throw new Error(`Live table ${expected.database}.${expected.name} does not match the schema; apply and verify the manual migration before --reconcile. Snapshot unchanged.`) - } - } - const snapshotFile = join(dirs.metaDir, 'snapshot.json') - if (!planMode) await writeFile(snapshotFile, `${JSON.stringify(createSnapshot(reconciled), null, 2)}\n`, 'utf8') - if (jsonMode) emitJson('generate', { mode: 'reconcile', verified: true, dryrun: planMode, snapshotFile, tables: scope.matchedTables }) - else console.log(`${planMode ? 'Verified' : 'Reconciled'} column expressions for ${scope.matchedTables.join(', ')} against live ClickHouse. No migration generated.`) - return 0 - } - const renameTableValues = f['--rename-table'] ?? [] const renameColumnValues = f['--rename-column'] ?? [] const renameDictionaryValues = f['--rename-dictionary'] ?? [] diff --git a/packages/cli/src/commands/generate/reconcile.ts b/packages/cli/src/commands/generate/reconcile.ts deleted file mode 100644 index f231f3d7..00000000 --- a/packages/cli/src/commands/generate/reconcile.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { - canonicalizeDefinitions, - type SchemaDefinition, - type TableDefinition, -} from '@chkit/core' - -/** Adopt only expression metadata; every other schema edit still needs generation. */ -export function reconcileColumnExpressions( - previous: SchemaDefinition[], - next: SchemaDefinition[], - selected: string[], -): SchemaDefinition[] { - const current = canonicalizeDefinitions(next) - const result = canonicalizeDefinitions(previous) - for (const key of selected) { - const index = result.findIndex( - (definition) => - definition.kind === 'table' && - `${definition.database}.${definition.name}` === key, - ) - const before = result[index] - const after = current.find( - (definition): definition is TableDefinition => - definition.kind === 'table' && - `${definition.database}.${definition.name}` === key, - ) - if (before?.kind !== 'table' || !after) - throw new Error( - `Cannot reconcile ${key}: table must exist in both the snapshot and schema.`, - ) - const candidate: TableDefinition = { - ...before, - columns: before.columns.map((column) => { - const updated = after.columns.find((item) => item.name === column.name) - const { default: _default, defaultKind: _kind, ...rest } = column - return { - ...rest, - ...(updated?.default !== undefined - ? { default: updated.default } - : {}), - ...(updated?.defaultKind ? { defaultKind: updated.defaultKind } : {}), - } - }), - } - if ( - JSON.stringify(canonicalizeDefinitions([candidate])) !== - JSON.stringify([after]) - ) { - throw new Error( - `Cannot reconcile ${key}: --reconcile accepts only column expression/kind changes. Generate other schema changes separately.`, - ) - } - result[index] = candidate - } - return result -} diff --git a/packages/cli/src/test/column-expression-safety.test.ts b/packages/cli/src/test/column-expression-safety.test.ts index a6667bb2..9b1559bb 100644 --- a/packages/cli/src/test/column-expression-safety.test.ts +++ b/packages/cli/src/test/column-expression-safety.test.ts @@ -1,7 +1,6 @@ import { expect, test } from 'bun:test' import { planDiff, table } from '@chkit/core' import { compareTableShape } from '../commands/drift/compare.js' -import { reconcileColumnExpressions } from '../commands/generate/reconcile.js' const definition = (value?: string | number | boolean) => table({ @@ -47,33 +46,6 @@ for (const [expected, actual, equal] of [ }) } -test('reconciliation adopts only selected expression metadata and permits the next generate', () => { - const before = definition('fn:toString(id)') - const after = { - ...before, - columns: before.columns.map((col) => - col.name === 'value' ? { ...col, defaultKind: 'ALIAS' as const } : col, - ), - } - const unrelated = { ...before, name: 'other' } - const reconciled = reconcileColumnExpressions( - [before, unrelated], - [after, { ...unrelated, comment: 'pending' }], - ['default.events'], - ) - expect(planDiff(reconciled, [after, unrelated]).operations).toEqual([]) - expect(() => - reconcileColumnExpressions( - [before], - [{ ...after, engine: 'Log' }], - ['default.events'], - ), - ).toThrow('only column expression/kind changes') - expect(() => - reconcileColumnExpressions([], [after], ['default.events']), - ).toThrow('both the snapshot and schema') -}) - test('stored expression changes warn about historical values; computed aliases do not', () => { const before = definition('old') const after = definition('new') diff --git a/packages/cli/src/test/column-expressions.e2e.test.ts b/packages/cli/src/test/column-expressions.e2e.test.ts index 921801ef..b03a669a 100644 --- a/packages/cli/src/test/column-expressions.e2e.test.ts +++ b/packages/cli/src/test/column-expressions.e2e.test.ts @@ -1,5 +1,5 @@ import { expect, test } from 'bun:test' -import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { createClient } from '@clickhouse/client' @@ -14,7 +14,6 @@ import { generateTypeArtifacts, generateIngestArtifacts, } from '../../../plugin-codegen/src/index.js' -import { createFixture, runCli } from './testkit.test.js' import { buildBackfillPlan } from '../../../plugin-backfill/src/planner.js' import { PlanSchema } from '../../../plugin-backfill/src/options.js' import { getRequiredEnv } from './e2e-testkit.js' @@ -276,75 +275,3 @@ test('generated ingest helpers use insert shapes, while rows exclude ephemeral i expect(ingest).toContain('DefaultEventsRowInsertSchema.parse(row)') expect(ingest).toContain('function ingestDefaultEvents(') }) - -test('manual conversion reconciles only after live verification, then generate is a no-op', async () => { - const env = getRequiredEnv() - const client = createClient({ - url: env.clickhouseUrl, - username: env.clickhouseUser, - password: env.clickhousePassword, - database: env.clickhouseDatabase, - }) - const name = `reconcile_expr_${Date.now()}` - const before = table({ - database: env.clickhouseDatabase, - name, - engine: 'MergeTree()', - primaryKey: ['id'], - orderBy: ['id'], - columns: [ - { name: 'id', type: 'UInt32' }, - { name: 'label', type: 'String', default: 'fn:toString(id)' }, - ], - }) - const fixture = await createFixture( - `export default [${JSON.stringify(before)}]`, - ) - const args = ['generate', '--config', fixture.configPath, '--json'] - try { - await writeFile( - fixture.configPath, - `export default ${JSON.stringify({ schema: [fixture.schemaPath], metaDir: fixture.metaDir, migrationsDir: fixture.migrationsDir, clickhouse: { url: env.clickhouseUrl, username: env.clickhouseUser, password: env.clickhousePassword, database: env.clickhouseDatabase } })}`, - ) - expect(runCli(args).exitCode).toBe(0) - await client.command({ query: toCreateSQL(before) }) - const snapshotPath = join(fixture.metaDir, 'snapshot.json') - const oldSnapshot = await readFile(snapshotPath, 'utf8') - const after = { - ...before, - columns: before.columns.map((col) => - col.name === 'label' ? { ...col, defaultKind: 'ALIAS' } : col, - ), - } - await writeFile( - fixture.schemaPath, - `export default [${JSON.stringify(after)}]`, - ) - const reconcile = [ - ...args, - '--reconcile', - '--table', - `${before.database}.${name}`, - ] - expect(runCli(reconcile).exitCode).not.toBe(0) - expect(await readFile(snapshotPath, 'utf8')).toBe(oldSnapshot) - await client.command({ - query: `ALTER TABLE ${before.database}.${name} DROP COLUMN label, ADD COLUMN label String ALIAS toString(id)`, - }) - const preview = runCli([...reconcile, '--dryrun']) - expect(preview.exitCode).toBe(0) - expect(await readFile(snapshotPath, 'utf8')).toBe(oldSnapshot) - const result = runCli(reconcile) - expect(result.exitCode).toBe(0) - expect(JSON.parse(result.stdout).verified).toBe(true) - const next = runCli([...args, '--dryrun']) - expect(next.exitCode).toBe(0) - expect(JSON.parse(next.stdout).operationCount).toBe(0) - } finally { - await client.command({ - query: `DROP TABLE IF EXISTS ${before.database}.${name} SYNC`, - }) - await client.close() - await rm(fixture.dir, { recursive: true, force: true }) - } -}, 30_000) diff --git a/packages/core/src/column-expressions.test.ts b/packages/core/src/column-expressions.test.ts index f39b650e..664b4bb0 100644 --- a/packages/core/src/column-expressions.test.ts +++ b/packages/core/src/column-expressions.test.ts @@ -95,10 +95,10 @@ describe('column expressions', () => { for (const defaultKind of ['ALIAS', 'EPHEMERAL'] as const) { const virtual = definition({ defaultKind, default: 'fn:toDate(ts)' }) expect(() => planDiff([definition()], [virtual])).toThrow( - 'explicit manual migration', + 'storage-kind conversions involving ALIAS or EPHEMERAL are not supported', ) expect(() => planDiff([virtual], [definition()])).toThrow( - 'explicit manual migration', + 'storage-kind conversions involving ALIAS or EPHEMERAL are not supported', ) } }) diff --git a/packages/core/src/planner.ts b/packages/core/src/planner.ts index f92aadc5..4c47ed5a 100644 --- a/packages/core/src/planner.ts +++ b/packages/core/src/planner.ts @@ -412,7 +412,7 @@ function diffTables(oldDef: TableDefinition, newDef: TableDefinition): TableDiff const oldKind = oldItem.defaultKind ?? 'DEFAULT' const newKind = newItem.defaultKind ?? 'DEFAULT' if (oldKind !== newKind && [oldKind, newKind].some((kind) => kind === 'ALIAS' || kind === 'EPHEMERAL')) { - throw new Error(`Cannot automatically change column ${newDef.database}.${newDef.name}.${name} from ${oldKind} to ${newKind}; use an explicit manual migration for storage-kind changes, then run generate --reconcile --table ${newDef.database}.${newDef.name} after applying it`) + throw new Error(`Cannot automatically change column ${newDef.database}.${newDef.name}.${name} from ${oldKind} to ${newKind}; storage-kind conversions involving ALIAS or EPHEMERAL are not supported`) } const sql = isCodecRemoval(oldItem, newItem) ? renderAlterRemoveCodec(newDef, name)