Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 747 primary key autoname #774

Merged
merged 10 commits into from
Aug 20, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Fixes

- Persist table comments for incremental models, snapshots and dbt clone (thanks @henlue!) ([750](https://github.com/databricks/dbt-databricks/pull/750))
- Add relation identifier (i.e. table name) in auto generated constraint names, also adding the statement of table list for foreign keys (thanks @elca-anh!) ([774](https://github.com/databricks/dbt-databricks/pull/774))
- Update tblproperties on incremental runs. Note: only adds/edits. Deletes are too risky/complex for now ([765](https://github.com/databricks/dbt-databricks/pull/765))

## dbt-databricks 1.8.5 (August 6, 2024)
Expand Down
46 changes: 35 additions & 11 deletions dbt/include/databricks/macros/relations/constraints.sql
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,13 @@
{% endif %}

{% set name = constraint.get("name") %}
{% if not name and local_md5 %}
{{ exceptions.warn("Constraint of type " ~ type ~ " with no `name` provided. Generating hash instead.") }}
{%- set name = local_md5 (column.get("name", "") ~ ";" ~ expression ~ ";") -%}
{% 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 (relation.identifier ~ ";" ~ column.get("name", "") ~ ";" ~ expression ~ ";") -%}
{% else %}
{{ exceptions.raise_compiler_error("Constraint of type " ~ type ~ " with no `name` provided, and no md5 utility.") }}
{% endif %}
{% endif %}
{% set stmt = "alter table " ~ relation ~ " add constraint " ~ name ~ " check (" ~ expression ~ ");" %}
{% do statements.append(stmt) %}
Expand Down Expand Up @@ -148,9 +152,13 @@
{% set joined_names = quoted_names|join(", ") %}

{% set name = constraint.get("name") %}
{% if not name and local_md5 %}
{{ exceptions.warn("Constraint of type " ~ type ~ " with no `name` provided. Generating hash instead.") }}
{%- set name = local_md5("primary_key;" ~ column_names ~ ";") -%}
{% 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("primary_key;" ~ relation.identifier ~ ";" ~ column_names ~ ";") -%}
{% else %}
{{ exceptions.raise_compiler_error("Constraint of type " ~ type ~ " with no `name` provided, and no md5 utility.") }}
{% endif %}
{% endif %}
{% set stmt = "alter table " ~ relation ~ " add constraint " ~ name ~ " primary key(" ~ joined_names ~ ");" %}
{% do statements.append(stmt) %}
Expand All @@ -161,12 +169,18 @@
{% endif %}

{% set name = constraint.get("name") %}
{% if not name and local_md5 %}
{{ exceptions.warn("Constraint of type " ~ type ~ " with no `name` provided. Generating hash instead.") }}
{%- set name = local_md5("primary_key;" ~ column_names ~ ";") -%}
{% endif %}


{% if constraint.get('expression') %}

{% 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 ~ ";" ~ constraint.get('expression') ~ ";") -%}
{% else %}
{{ exceptions.raise_compiler_error("Constraint of type " ~ type ~ " with no `name` provided, and no md5 utility.") }}
{% endif %}
{% endif %}

{% set stmt = "alter table " ~ relation ~ " add constraint " ~ name ~ " foreign key" ~ constraint.get('expression') %}
{% else %}
{% set column_names = constraint.get("columns", []) %}
Expand All @@ -193,6 +207,16 @@
{% if not "." in parent %}
{% set parent = relation.schema ~ "." ~ parent%}
{% endif %}

{% 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 ~ ";") -%}
{% else %}
{{ exceptions.raise_compiler_error("Constraint of type " ~ type ~ " with no `name` provided, and no md5 utility.") }}
{% endif %}
{% endif %}

{% set stmt = "alter table " ~ relation ~ " add constraint " ~ name ~ " foreign key(" ~ joined_names ~ ") references " ~ parent %}
{% set parent_columns = constraint.get("parent_columns") %}
{% if parent_columns %}
Expand Down
38 changes: 36 additions & 2 deletions tests/unit/macros/relations/test_constraint_macros.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ def template_name(self) -> str:
def macro_folders_to_load(self) -> list:
return ["macros/relations", "macros"]

@pytest.fixture(scope="class", autouse=True)
def modify_context(self, default_context) -> None:
# Mock local_md5
default_context["local_md5"] = lambda s: f"hash({s})"

def render_constraints(self, template, *args):
return self.run_macro(template, "databricks_constraints_to_dbt", *args)

Expand Down Expand Up @@ -240,15 +245,16 @@ def test_macros_get_constraint_sql_check_named_constraint(self, template_bundle,
)
assert expected in r

def test_macros_get_constraint_sql_check_none_constraint(self, template_bundle, model):
def test_macros_get_constraint_sql_check_noname_constraint(self, template_bundle, model):
constraint = {
"type": "check",
"expression": "id != name",
}
r = self.render_constraint_sql(template_bundle, constraint, model)

expected = (
"['alter table `some_database`.`some_schema`.`some_table` add constraint None "
"['alter table `some_database`.`some_schema`.`some_table` "
"add constraint hash(some_table;;id != name;) "
"check (id != name);']"
) # noqa: E501
assert expected in r
Expand Down Expand Up @@ -307,6 +313,19 @@ def test_macros_get_constraint_sql_primary_key_with_name(self, template_bundle,
)
assert expected in r

def test_macros_get_constraint_sql_primary_key_noname(self, template_bundle, model):
constraint = {"type": "primary_key"}
column = {"name": "id"}

r = self.render_constraint_sql(template_bundle, constraint, model, column)

expected = (
'["alter table `some_database`.`some_schema`.`some_table` add constraint '
"hash(primary_key;some_table;['id'];) "
'primary key(id);"]'
)
assert expected in r

def test_macros_get_constraint_sql_foreign_key(self, template_bundle, model):
constraint = {
"type": "foreign_key",
Expand All @@ -323,6 +342,21 @@ def test_macros_get_constraint_sql_foreign_key(self, template_bundle, model):
)
assert expected in r

def test_macros_get_constraint_sql_foreign_key_noname(self, template_bundle, model):
constraint = {
"type": "foreign_key",
"columns": ["name"],
"parent": "parent_table",
}
r = self.render_constraint_sql(template_bundle, constraint, model)

expected = (
'["alter table `some_database`.`some_schema`.`some_table` add '
"constraint hash(foreign_key;some_table;['name'];some_schema.parent_table;) "
'foreign key(name) references some_schema.parent_table;"]'
)
assert expected in r

def test_macros_get_constraint_sql_foreign_key_parent_column(self, template_bundle, model):
constraint = {
"type": "foreign_key",
Expand Down
Loading