diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a378c0d8..cc7018f3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - Fix managed Iceberg Python models failing with `MANAGED_TABLE_FORMAT` by emitting `.format("iceberg")` instead of the `parquet` sentinel from `resolve_file_format` (thanks @Divya-Kovvuru-0802!) ([#1593](https://github.com/databricks/dbt-databricks/pull/1593) resolves [#1591](https://github.com/databricks/dbt-databricks/issues/1591)) - Quote generated column identifiers in incremental strategies so non-ASCII column names no longer fail on subsequent runs ([#1595](https://github.com/databricks/dbt-databricks/pull/1595) resolves [#1594](https://github.com/databricks/dbt-databricks/issues/1594)) - Handle missing or empty view-definition metadata when creating materialized views from streaming tables or newly-created materialized views (thanks @aarushisingh04!) ([#1462](https://github.com/databricks/dbt-databricks/pull/1462) resolves [#1459](https://github.com/databricks/dbt-databricks/issues/1459)) +- Fix unnamed primary and foreign keys churning on every incremental run, and two or more unnamed foreign keys to the same parent failing with `DELTA_CONSTRAINT_ALREADY_EXISTS`. dbt now gives an unnamed PK/FK a deterministic name (its full identity, including a foreign key's referenced columns) generated identically on the create and incremental paths — including V2 inline `CREATE TABLE` and bare-parent FK configs — so the model and catalog agree and the diff is a no-op. Existing unnamed foreign keys are renamed once on the next incremental run (a no-op drop/re-add, no cascade). An unnamed key's `expression`/target edit is reconciled via that name; a named key's `expression` edit still needs `--full-refresh` (see #1552). ([#1561](https://github.com/databricks/dbt-databricks/pull/1561) resolves [#1333](https://github.com/databricks/dbt-databricks/issues/1333) and [#1344](https://github.com/databricks/dbt-databricks/issues/1344)) ### Under the Hood diff --git a/dbt/adapters/databricks/constraints.py b/dbt/adapters/databricks/constraints.py index 112fd77bf..647c5377d 100644 --- a/dbt/adapters/databricks/constraints.py +++ b/dbt/adapters/databricks/constraints.py @@ -1,3 +1,4 @@ +import hashlib from abc import ABC, abstractmethod from dataclasses import dataclass from typing import Any, ClassVar, Optional, TypeVar @@ -162,6 +163,30 @@ def is_enforced(constraint: ColumnLevelConstraint) -> bool: ] +def _local_md5(value: str) -> str: + # Same digest as Jinja local_md5. + return hashlib.md5(value.encode("utf-8")).hexdigest() + + +def synthesize_constraint_name(constraint: TypedConstraint, relation_identifier: str) -> str: + """Deterministic name for an unnamed PK/FK; mirrors relations/constraints.sql.""" + if isinstance(constraint, PrimaryKeyConstraint): + hash_input = f"primary_key;{relation_identifier};{constraint.columns};" + if constraint.expression: + hash_input += f"{constraint.expression};" + return _local_md5(hash_input) + if isinstance(constraint, ForeignKeyConstraint): + if constraint.expression: + return _local_md5(f"foreign_key;{relation_identifier};{constraint.expression};") + hash_input = f"foreign_key;{relation_identifier};{constraint.columns};{constraint.to};" + if constraint.to_columns: + hash_input += f"{constraint.to_columns};" + return _local_md5(hash_input) + raise DbtValidationError( + f"Cannot synthesize a name for constraint type: {type(constraint).__name__}" + ) + + def process_constraint(constraint: TypedConstraint) -> Optional[str]: if validate_constraint(constraint): return constraint.render() diff --git a/dbt/adapters/databricks/relation.py b/dbt/adapters/databricks/relation.py index 2a3491285..ebdc9da23 100644 --- a/dbt/adapters/databricks/relation.py +++ b/dbt/adapters/databricks/relation.py @@ -13,7 +13,13 @@ from dbt_common.exceptions import DbtRuntimeError from dbt_common.utils import filter_null_values -from dbt.adapters.databricks.constraints import TypedConstraint, process_constraint +from dbt.adapters.databricks.constraints import ( + ForeignKeyConstraint, + PrimaryKeyConstraint, + TypedConstraint, + process_constraint, + synthesize_constraint_name, +) from dbt.adapters.databricks.logging import logger from dbt.adapters.databricks.utils import remove_undefined @@ -262,6 +268,11 @@ def enrich(self, constraints: list[TypedConstraint]) -> "DatabricksRelation": return copy def render_constraints_for_create(self) -> str: + for constraint in self.create_constraints: + if constraint.name is None and isinstance( + constraint, (PrimaryKeyConstraint, ForeignKeyConstraint) + ): + constraint.name = synthesize_constraint_name(constraint, self.identifier) processed = map(process_constraint, self.create_constraints) return ", ".join(c for c in processed if c is not None) diff --git a/dbt/adapters/databricks/relation_configs/constraints.py b/dbt/adapters/databricks/relation_configs/constraints.py index 9410decdd..0b8dae77f 100644 --- a/dbt/adapters/databricks/relation_configs/constraints.py +++ b/dbt/adapters/databricks/relation_configs/constraints.py @@ -13,6 +13,7 @@ PrimaryKeyConstraint, TypedConstraint, parse_constraints, + synthesize_constraint_name, ) from dbt.adapters.databricks.relation_configs.base import ( DatabricksComponentConfig, @@ -251,6 +252,12 @@ def from_relation_config(cls, relation_config: RelationConfig) -> ConstraintsCon non_nulls, other_constraints = parse_constraints(columns, constraints) + for constraint in other_constraints: + if constraint.name is None and isinstance( + constraint, (PrimaryKeyConstraint, ForeignKeyConstraint) + ): + constraint.name = synthesize_constraint_name(constraint, relation_config.identifier) + return ConstraintsConfig( set_non_nulls=set(non_nulls), set_constraints=set(other_constraints), diff --git a/dbt/include/databricks/macros/relations/constraints.sql b/dbt/include/databricks/macros/relations/constraints.sql index c3e8dac12..752eab94b 100644 --- a/dbt/include/databricks/macros/relations/constraints.sql +++ b/dbt/include/databricks/macros/relations/constraints.sql @@ -225,6 +225,8 @@ {% set joined_names = quoted_names|join(", ") %} {% set parent = constraint.get('to') %} + {# Hash raw `to` so bare parents match synthesize_constraint_name. #} + {% set raw_parent = parent %} {% if not parent %} {{ exceptions.raise_compiler_error('No parent table defined for foreign key: ' ~ expression) }} {% endif %} @@ -233,17 +235,21 @@ {% set parent = parent_relation.render() %} {% endif %} + {% set parent_columns = constraint.get('to_columns') %} {% if not name %} {% if local_md5 %} {{ exceptions.warn("Constraint of type " ~ type ~ " with no `name` provided. Generating hash instead for relation " ~ relation.identifier) }} - {%- set name = local_md5("foreign_key;" ~ relation.identifier ~ ";" ~ column_names ~ ";" ~ parent ~ ";") -%} + {%- set hash_input = "foreign_key;" ~ relation.identifier ~ ";" ~ column_names ~ ";" ~ raw_parent ~ ";" -%} + {%- if parent_columns -%} + {%- set hash_input = hash_input ~ parent_columns ~ ";" -%} + {%- endif -%} + {%- set name = local_md5(hash_input) -%} {% else %} {{ exceptions.raise_compiler_error("Constraint of type " ~ type ~ " with no `name` provided, and no md5 utility.") }} - {% endif %} + {% endif %} {% endif %} {% set stmt = "alter table " ~ relation.render() ~ " add constraint " ~ name ~ " foreign key(" ~ joined_names ~ ") references " ~ parent %} - {% set parent_columns = constraint.get('to_columns') %} {% if parent_columns %} {% set quoted_parent_columns = [] %} {% for parent_column in parent_columns %} diff --git a/tests/functional/adapter/constraints/fixtures.py b/tests/functional/adapter/constraints/fixtures.py index 117bd7168..85e610d8b 100644 --- a/tests/functional/adapter/constraints/fixtures.py +++ b/tests/functional/adapter/constraints/fixtures.py @@ -379,7 +379,6 @@ constraints: - type: not_null - type: primary_key - name: pk_rely_parent expression: RELY - name: rely_child config: @@ -411,6 +410,55 @@ select 1 as parent_n, 10 as child_id """ +incremental_multiple_fk_schema_yml = """ +version: 2 +models: + - name: multi_fk_parent + config: + materialized: table + contract: + enforced: true + columns: + - name: id + data_type: int + constraints: + - type: not_null + - type: primary_key + name: pk_multi_fk_parent + - name: multi_fk_child + config: + materialized: incremental + unique_key: child_id + on_schema_change: append_new_columns + contract: + enforced: true + columns: + - name: child_id + data_type: int + - name: parent_a + data_type: int + constraints: + - type: foreign_key + to: ref('multi_fk_parent') + to_columns: ["id"] + - name: parent_b + data_type: int + constraints: + - type: foreign_key + to: ref('multi_fk_parent') + to_columns: ["id"] +""" + +incremental_multiple_fk_parent_sql = """ +select 1 as id +""" + +incremental_multiple_fk_child_sql = """ +-- depends_on: {{ ref('multi_fk_parent') }} + +select 1 as child_id, 1 as parent_a, 1 as parent_b +""" + def _incremental_contract_off_pk_schema_yml(enforced): return f""" @@ -463,3 +511,49 @@ def _incremental_contract_off_pk_schema_yml(enforced): - name: color data_type: string """ + +incremental_v2_unnamed_pk_cascade_schema_yml = """ +version: 2 +models: + - name: v2_unnamed_pk_parent + config: + materialized: incremental + unique_key: n + on_schema_change: append_new_columns + contract: + enforced: true + columns: + - name: n + data_type: int + constraints: + - type: not_null + - type: primary_key + - name: v2_unnamed_pk_child + config: + materialized: table + contract: + enforced: true + constraints: + - type: foreign_key + name: fk_v2_unnamed_pk_child + columns: ["parent_n"] + to: ref('v2_unnamed_pk_parent') + to_columns: ["n"] + columns: + - name: parent_n + data_type: int + constraints: + - type: not_null + - name: child_id + data_type: int +""" + +incremental_v2_unnamed_pk_parent_sql = """ +select 1 as n +""" + +incremental_v2_unnamed_pk_child_sql = """ +-- depends_on: {{ ref('v2_unnamed_pk_parent') }} + +select 1 as parent_n, 10 as child_id +""" diff --git a/tests/functional/adapter/constraints/test_constraints.py b/tests/functional/adapter/constraints/test_constraints.py index 485138208..6c6f716b5 100644 --- a/tests/functional/adapter/constraints/test_constraints.py +++ b/tests/functional/adapter/constraints/test_constraints.py @@ -222,10 +222,12 @@ def test_foreign_key_constraint(self, project): @pytest.mark.skip_profile("databricks_cluster") -class TestIncrementalRelyConstraintReconciliation: - """A RELY expression on a primary key cannot be read back from information_schema, so it - must not trigger constraint reconciliation on an incremental run. Otherwise the parent PK - is dropped with CASCADE every run, silently dropping the child's foreign key (#1513). +class TestIncrementalPrimaryKeyConstraintReconciliation: + """A primary key's non-round-trippable fields must not trigger reconciliation on an incremental + run. RELY (#1513) is not readable from information_schema, and an unnamed PK (#1333) is given a + server-assigned name the model lacks; treating either as a change drops the PK with CASCADE + every run, silently dropping the child's foreign key. The parent PK here is both unnamed and + RELY, so this guards both fixes. """ @pytest.fixture(scope="class") @@ -251,16 +253,62 @@ def _foreign_key_names(self, project): ) return {row[0] for row in rows} - def test_rely_pk_reconcile_keeps_dependent_foreign_key(self, project): + def test_unnamed_rely_pk_reconcile_keeps_dependent_foreign_key(self, project): util.run_dbt(["build"]) assert "fk_rely_child" in self._foreign_key_names(project) - # A plain incremental re-run of the parent must not reconcile its RELY PK. + # A plain incremental re-run of the parent must not reconcile its unnamed RELY PK. util.run_dbt(["run", "--select", "rely_parent"]) assert "fk_rely_child" in self._foreign_key_names(project) +@pytest.mark.skip_profile("databricks_cluster") +class TestIncrementalMultipleUnnamedForeignKeys: + """Two unnamed foreign keys to the same parent must both survive an incremental run. Without a + deterministic name, the model's name=None never matches the catalog's server-assigned name, so + the diff drops and re-adds both every run, and the unnamed re-adds collide on the server-derived + name with DELTA_CONSTRAINT_ALREADY_EXISTS (#1344). + """ + + @pytest.fixture(scope="class") + def project_config_update(self): + return {"flags": {"use_materialization_v2": False}} + + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": override_fixtures.incremental_multiple_fk_schema_yml, + "multi_fk_parent.sql": override_fixtures.incremental_multiple_fk_parent_sql, + "multi_fk_child.sql": override_fixtures.incremental_multiple_fk_child_sql, + } + + def _foreign_key_columns(self, project): + rows = project.run_sql( + """ + SELECT kcu.column_name + FROM {database}.information_schema.key_column_usage kcu + JOIN {database}.information_schema.table_constraints tc + ON kcu.constraint_name = tc.constraint_name + AND kcu.constraint_schema = tc.constraint_schema + WHERE tc.constraint_schema = '{schema}' + AND tc.table_name = 'multi_fk_child' + AND tc.constraint_type = 'FOREIGN KEY' + """, + fetch="all", + ) + return {row[0] for row in rows} + + def test_multiple_unnamed_fks_survive_incremental_run(self, project): + util.run_dbt(["build"]) + assert {"parent_a", "parent_b"} <= self._foreign_key_columns(project) + + # The incremental re-run must not drop and re-add the unnamed FKs (they would collide). + util.run_dbt(["run", "--select", "multi_fk_child"]) + + assert {"parent_a", "parent_b"} <= self._foreign_key_columns(project) + + @pytest.mark.skip_profile("databricks_cluster") class TestIncrementalContractOffPreservesConstraints(RerunSafeMixin): """Constraints are reconciled only when the contract is enforced; when unenforced, a no-op.""" @@ -306,3 +354,39 @@ def test_contract_off_incremental_preserves_existing_pk(self, project): # The existing PK must be left untouched, not silently dropped. assert "pk_contract_off" in self._primary_key_names(project) + + +@pytest.mark.skip_profile("databricks_cluster") +class TestV2IncrementalUnnamedPrimaryKeyReconciliation: + """V2: unnamed parent PK re-run must leave the dependent FK intact.""" + + @pytest.fixture(scope="class") + def project_config_update(self): + return {"flags": {"use_materialization_v2": True}} + + @pytest.fixture(scope="class") + def models(self): + return { + "schema.yml": override_fixtures.incremental_v2_unnamed_pk_cascade_schema_yml, + "v2_unnamed_pk_parent.sql": override_fixtures.incremental_v2_unnamed_pk_parent_sql, + "v2_unnamed_pk_child.sql": override_fixtures.incremental_v2_unnamed_pk_child_sql, + } + + def _foreign_key_names(self, project): + rows = project.run_sql( + """ + SELECT constraint_name + FROM {database}.information_schema.referential_constraints + WHERE constraint_schema = '{schema}' + """, + fetch="all", + ) + return {row[0] for row in rows} + + def test_unnamed_pk_reconcile_keeps_dependent_foreign_key(self, project): + util.run_dbt(["build"]) + assert "fk_v2_unnamed_pk_child" in self._foreign_key_names(project) + + util.run_dbt(["run", "--select", "v2_unnamed_pk_parent"]) + + assert "fk_v2_unnamed_pk_child" in self._foreign_key_names(project) diff --git a/tests/unit/macros/relations/test_constraint_macros.py b/tests/unit/macros/relations/test_constraint_macros.py index 0c2317881..3c5a4fe81 100644 --- a/tests/unit/macros/relations/test_constraint_macros.py +++ b/tests/unit/macros/relations/test_constraint_macros.py @@ -1,5 +1,14 @@ -import pytest +import re +import pytest +from dbt_common.contracts.constraints import ConstraintType + +from dbt.adapters.databricks.constraints import ( + ForeignKeyConstraint, + PrimaryKeyConstraint, + _local_md5, + synthesize_constraint_name, +) from tests.unit.macros.base import MacroTestBase @@ -346,7 +355,6 @@ def test_macros_get_constraint_sql_primary_key_noname_with_expression( r = self.render_constraint_sql(template_bundle, constraint, model, column) - # clean_sql() lowercases the rendered SQL, including the hash input echoed by the mock. expected = ( '["alter table `some_database`.`some_schema`.`some_table` add constraint ' "hash(primary_key;some_table;['id'];rely;) " @@ -380,7 +388,7 @@ def test_macros_get_constraint_sql_foreign_key_noname(self, template_bundle, mod expected = ( '["alter table `some_database`.`some_schema`.`some_table` add ' - "constraint hash(foreign_key;some_table;['name'];some_schema.parent_table;) " + "constraint hash(foreign_key;some_table;['name'];parent_table;) " 'foreign key(`name`) references some_schema.parent_table;"]' ) assert expected in r @@ -474,3 +482,127 @@ def test_macros_get_constraint_sql_custom_missing_expression(self, template_bund } r = self.render_constraint_sql(template_bundle, constraint, model) assert "raise_compiler_error" in r + + +class TestConstraintNameParity(MacroTestBase): + """Macro create-time names must match synthesize_constraint_name.""" + + @pytest.fixture + def template_name(self) -> str: + return "constraints.sql" + + @pytest.fixture + def macro_folders_to_load(self) -> list: + return ["macros/relations", "macros"] + + @pytest.fixture + def model(self): + columns = {name: {"name": name, "data_type": "int"} for name in ("a", "b", "c")} + return {"columns": columns} + + def _macro_name(self, template_bundle, constraint, *args): + template_bundle.context["local_md5"] = _local_md5 + rendered = self.run_macro_raw( + template_bundle.template, + "get_constraint_sql", + template_bundle.relation, + constraint, + *args, + ) + match = re.search(r"add constraint (\S+) ", rendered) + assert match, f"no constraint name found in rendered macro output: {rendered}" + return match.group(1) + + def test_parity__foreign_key_single_column(self, template_bundle, model): + constraint = { + "type": "foreign_key", + "columns": ["a"], + "to": "`c`.`s`.`parent`", + "to_columns": ["id"], + } + py_name = synthesize_constraint_name( + ForeignKeyConstraint( + type=ConstraintType.foreign_key, + columns=["a"], + to="`c`.`s`.`parent`", + to_columns=["id"], + ), + template_bundle.relation.identifier, + ) + assert self._macro_name(template_bundle, constraint, model) == py_name + + def test_parity__foreign_key_multiple_columns(self, template_bundle, model): + constraint = { + "type": "foreign_key", + "columns": ["a", "b"], + "to": "`c`.`s`.`parent`", + "to_columns": ["x", "y"], + } + py_name = synthesize_constraint_name( + ForeignKeyConstraint( + type=ConstraintType.foreign_key, + columns=["a", "b"], + to="`c`.`s`.`parent`", + to_columns=["x", "y"], + ), + template_bundle.relation.identifier, + ) + assert self._macro_name(template_bundle, constraint, model) == py_name + + def test_parity__foreign_key_bare_parent(self, template_bundle, model): + constraint = { + "type": "foreign_key", + "columns": ["a"], + "to": "parent", + "to_columns": ["id"], + } + py_name = synthesize_constraint_name( + ForeignKeyConstraint( + type=ConstraintType.foreign_key, + columns=["a"], + to="parent", + to_columns=["id"], + ), + template_bundle.relation.identifier, + ) + assert self._macro_name(template_bundle, constraint, model) == py_name + + def test_parity__foreign_key_expression_form(self, template_bundle, model): + constraint = { + "type": "foreign_key", + "columns": ["a"], + "expression": "(a) REFERENCES `c`.`s`.`parent`", + } + py_name = synthesize_constraint_name( + ForeignKeyConstraint( + type=ConstraintType.foreign_key, + columns=["a"], + expression="(a) REFERENCES `c`.`s`.`parent`", + ), + template_bundle.relation.identifier, + ) + assert self._macro_name(template_bundle, constraint, model) == py_name + + def test_parity__primary_key_single_column(self, template_bundle, model): + constraint = {"type": "primary_key", "columns": ["a"]} + py_name = synthesize_constraint_name( + PrimaryKeyConstraint(type=ConstraintType.primary_key, columns=["a"]), + template_bundle.relation.identifier, + ) + assert self._macro_name(template_bundle, constraint, model) == py_name + + def test_parity__primary_key_multiple_columns(self, template_bundle, model): + constraint = {"type": "primary_key", "columns": ["a", "b"]} + py_name = synthesize_constraint_name( + PrimaryKeyConstraint(type=ConstraintType.primary_key, columns=["a", "b"]), + template_bundle.relation.identifier, + ) + assert self._macro_name(template_bundle, constraint, model) == py_name + + def test_parity__primary_key_with_rely_expression(self, template_bundle, model): + constraint = {"type": "primary_key", "columns": ["a"], "expression": "RELY"} + py_name = synthesize_constraint_name( + PrimaryKeyConstraint(type=ConstraintType.primary_key, columns=["a"], expression="RELY"), + template_bundle.relation.identifier, + ) + assert self._macro_name(template_bundle, constraint, model) == py_name diff --git a/tests/unit/relation_configs/test_constraint.py b/tests/unit/relation_configs/test_constraint.py index 2a8ca3cf1..ce6e6654c 100644 --- a/tests/unit/relation_configs/test_constraint.py +++ b/tests/unit/relation_configs/test_constraint.py @@ -12,6 +12,7 @@ CheckConstraint, ForeignKeyConstraint, PrimaryKeyConstraint, + synthesize_constraint_name, ) from dbt.adapters.databricks.relation_configs.constraints import ( ConstraintsConfig, @@ -223,6 +224,42 @@ def test_from_relation_config__with_foreign_key_constraint(self): }, ) + def test_from_relation_config__unnamed_primary_key_gets_synthesized_name(self): + # An unnamed PK is given the deterministic name the create macro would assign, so the model + # side matches the catalog instead of churning (#1333). + model = self._make_model_with_contract( + identifier="my_model", + columns={}, + constraints=[ + ModelLevelConstraint(type=ConstraintType.primary_key, columns=["id"]), + ], + ) + spec = ConstraintsProcessor.from_relation_config(model) + (pk,) = spec.set_constraints + assert pk.name == synthesize_constraint_name( + PrimaryKeyConstraint(type=ConstraintType.primary_key, columns=["id"]), "my_model" + ) + assert pk.name is not None + + def test_from_relation_config__named_constraint_keeps_its_name(self): + # An explicitly named constraint is left untouched by name synthesis. + model = self._make_model_with_contract( + identifier="my_model", + columns={}, + constraints=[ + ModelLevelConstraint( + type=ConstraintType.foreign_key, + name="fk_explicit", + columns=["parent_id"], + to="`c`.`s`.`parent`", + to_columns=["id"], + ), + ], + ) + spec = ConstraintsProcessor.from_relation_config(model) + (fk,) = spec.set_constraints + assert fk.name == "fk_explicit" + def test_from_relation_config__with_non_null_constraint(self): model = self._make_model_with_contract( columns={ @@ -476,3 +513,84 @@ def test_get_diff__new_primary_key_retains_expression(self): other = ConstraintsConfig(set_non_nulls=set(), set_constraints=set()) diff = config.get_diff(other) assert {(c.name, c.expression) for c in diff.set_constraints} == {("pk_n", "RELY")} + + @staticmethod + def _model_with_fks(*fk_constraints): + model = Mock() + model.config.contract.enforced = True + model.identifier = "child" + model.columns = {} + model.constraints = list(fk_constraints) + return model + + @staticmethod + def _catalog_mirroring(model_config): + # Mimic from_relation_results for the catalog: the create macro stored each key under the + # same synthesized name, and information_schema round-trips columns/to/to_columns. + return ConstraintsConfig( + set_non_nulls=set(), + set_constraints={ + ForeignKeyConstraint( + type=ConstraintType.foreign_key, + name=fk.name, + columns=fk.columns, + to=fk.to, + to_columns=fk.to_columns, + ) + for fk in model_config.set_constraints + }, + ) + + def test_get_diff__unnamed_fk_matches_catalog_synthesized_name_is_noop(self): + # End-to-end: from_relation_config synthesizes the create-macro name, which equals what the + # catalog stored, so an unnamed FK is a no-op on an incremental run instead of churning + # (#1344). + model_config = ConstraintsProcessor.from_relation_config( + self._model_with_fks( + ModelLevelConstraint( + type=ConstraintType.foreign_key, + columns=["parent_id"], + to="`c`.`s`.`parent`", + to_columns=["id"], + ) + ) + ) + assert model_config.get_diff(self._catalog_mirroring(model_config)) is None + + # The no-op holds *because* the names agree: had the catalog kept the pre-fix server name, + # the diff would reconcile (drop + re-add), which is exactly the churn this fix removes. + server_named = ConstraintsConfig( + set_non_nulls=set(), + set_constraints={ + ForeignKeyConstraint( + type=ConstraintType.foreign_key, + name="child_parent_fk", + columns=["parent_id"], + to="`c`.`s`.`parent`", + to_columns=["id"], + ) + }, + ) + assert model_config.get_diff(server_named) is not None + + def test_get_diff__two_unnamed_fks_to_same_parent_is_noop(self): + # The exact #1344 shape: two unnamed FKs to one parent on different columns get distinct + # synthesized names (so the re-adds would not collide) and neither churns. + model_config = ConstraintsProcessor.from_relation_config( + self._model_with_fks( + ModelLevelConstraint( + type=ConstraintType.foreign_key, + columns=["a"], + to="`c`.`s`.`parent`", + to_columns=["id"], + ), + ModelLevelConstraint( + type=ConstraintType.foreign_key, + columns=["b"], + to="`c`.`s`.`parent`", + to_columns=["id"], + ), + ) + ) + assert len({c.name for c in model_config.set_constraints}) == 2 + assert model_config.get_diff(self._catalog_mirroring(model_config)) is None diff --git a/tests/unit/test_constraints.py b/tests/unit/test_constraints.py index 413533642..ec8994c4b 100644 --- a/tests/unit/test_constraints.py +++ b/tests/unit/test_constraints.py @@ -1,3 +1,4 @@ +import hashlib from unittest.mock import patch import pytest @@ -14,6 +15,7 @@ ForeignKeyConstraint, PrimaryKeyConstraint, TypedConstraint, + _local_md5, is_enforced, is_supported, parse_column_constraints, @@ -21,6 +23,7 @@ parse_constraints, parse_model_constraints, process_constraint, + synthesize_constraint_name, validate_constraint, ) from dbt.adapters.databricks.impl import DatabricksAdapter @@ -141,6 +144,72 @@ def test_foreign_key_constraint_render__with_expression(self): ) +class TestSynthesizeConstraintName: + """The model side must reproduce the deterministic name the create-time macro + (relations/constraints.sql) assigns to an unnamed PK/FK, so the catalog read matches and the + incremental diff is a no-op instead of churning + colliding (#1333, #1344).""" + + def test_local_md5_reproduces_real_dbt_local_md5(self): + # `f945e062...` is a name a live create-macro run emitted via dbt's `local_md5` for this + # exact input, so this pins our `_local_md5` wrapper to dbt's, independent of the scheme. + assert ( + _local_md5( + "foreign_key;multi_fk_child;['parent_b'];" + "`main`.`test17823128660395867514_test_constraints`.`multi_fk_parent`;" + ) + == "f945e062b14a8f9a207b7900720295f0" + ) + + def test_foreign_key_column_form_input_format(self): + # Column-form FK identity folds in `to_columns`, so two FKs on the same column to the same + # parent but different parent columns get distinct names instead of colliding (#1344). + fk = ForeignKeyConstraint( + type=ConstraintType.foreign_key, + name=None, + columns=["a", "b"], + to="`c`.`s`.`p`", + to_columns=["x", "y"], + ) + expected = hashlib.md5(b"foreign_key;child;['a', 'b'];`c`.`s`.`p`;['x', 'y'];").hexdigest() + assert synthesize_constraint_name(fk, "child") == expected + + def test_foreign_key_distinct_to_columns_get_distinct_names(self): + same = dict(type=ConstraintType.foreign_key, name=None, columns=["a"], to="`c`.`s`.`p`") + fk_x = ForeignKeyConstraint(to_columns=["x"], **same) + fk_y = ForeignKeyConstraint(to_columns=["y"], **same) + assert synthesize_constraint_name(fk_x, "child") != synthesize_constraint_name( + fk_y, "child" + ) + + def test_foreign_key_expression_form(self): + fk = ForeignKeyConstraint( + type=ConstraintType.foreign_key, + name=None, + columns=["n"], + expression="(n) REFERENCES `c`.`s`.`p`", + ) + expected = hashlib.md5(b"foreign_key;child;(n) REFERENCES `c`.`s`.`p`;").hexdigest() + assert synthesize_constraint_name(fk, "child") == expected + + def test_primary_key_no_expression(self): + pk = PrimaryKeyConstraint(type=ConstraintType.primary_key, name=None, columns=["n"]) + expected = hashlib.md5(b"primary_key;child;['n'];").hexdigest() + assert synthesize_constraint_name(pk, "child") == expected + + def test_primary_key_with_expression_includes_rely(self): + # The macro folds the expression into the PK hash, so a no-change RELY re-run matches + # (no churn) while a RELY edit changes the name and is reconciled. + pk_rely = PrimaryKeyConstraint( + type=ConstraintType.primary_key, name=None, columns=["n"], expression="RELY" + ) + expected = hashlib.md5(b"primary_key;child;['n'];RELY;").hexdigest() + assert synthesize_constraint_name(pk_rely, "child") == expected + pk_plain = PrimaryKeyConstraint(type=ConstraintType.primary_key, name=None, columns=["n"]) + assert synthesize_constraint_name(pk_rely, "child") != synthesize_constraint_name( + pk_plain, "child" + ) + + class TestConstraintsSupported: @pytest.mark.parametrize( "constraint_type, supported", diff --git a/tests/unit/test_relation.py b/tests/unit/test_relation.py index 748677a5a..87cb899f2 100644 --- a/tests/unit/test_relation.py +++ b/tests/unit/test_relation.py @@ -7,7 +7,9 @@ from dbt.adapters.databricks.constraints import ( CheckConstraint, CustomConstraint, + ForeignKeyConstraint, PrimaryKeyConstraint, + synthesize_constraint_name, ) from dbt.adapters.databricks.relation import ( MAX_CHARACTERS_IN_IDENTIFIER, @@ -412,7 +414,11 @@ def test_render_constraints_for_create__with_constraints( ): relation.add_constraint(custom_constraint) relation.add_constraint(pk_constraint) - assert relation.render_constraints_for_create() == "a > 1, PRIMARY KEY (a)" + expected_pk_name = synthesize_constraint_name(pk_constraint, relation.identifier) + assert ( + relation.render_constraints_for_create() + == f"a > 1, CONSTRAINT {expected_pk_name} PRIMARY KEY (a)" + ) class TestGetFunctionConfig: @@ -498,6 +504,45 @@ def test_none_identifier_is_allowed(self): assert rel.identifier is None +class TestRenderConstraintsForCreate: + """Unnamed PK/FK get synthesize_constraint_name on the V2 CREATE path.""" + + def test_unnamed_primary_key_gets_synthesized_name(self): + pk = PrimaryKeyConstraint(type=ConstraintType.primary_key, columns=["id"]) + expected = synthesize_constraint_name(pk, "child") + rel = DatabricksRelation.create(identifier="child", type="table") + + sql = rel.enrich([pk]).render_constraints_for_create() + + assert f"CONSTRAINT {expected}" in sql + + def test_unnamed_foreign_key_gets_synthesized_name(self): + fk = ForeignKeyConstraint( + type=ConstraintType.foreign_key, + columns=["parent_id"], + to="`c`.`s`.`parent`", + to_columns=["id"], + ) + expected = synthesize_constraint_name(fk, "child") + rel = DatabricksRelation.create(identifier="child", type="table") + + sql = rel.enrich([fk]).render_constraints_for_create() + + assert f"CONSTRAINT {expected}" in sql + + def test_explicitly_named_constraint_is_left_untouched(self): + rel = DatabricksRelation.create(identifier="child", type="table") + enriched = rel.enrich( + [ + PrimaryKeyConstraint( + type=ConstraintType.primary_key, name="pk_explicit", columns=["id"] + ) + ] + ) + sql = enriched.render_constraints_for_create() + assert "CONSTRAINT pk_explicit" in sql + + class TestDatabricksRenderLimited: def test_render_limited_with_empty_no_alias(self): relation = DatabricksRelation.create(