Skip to content

Commit 74dacfd

Browse files
benc-dbclaude
andauthored
fix: COMMENT ON syntax not supported prior to 16.1 (#1151)
### Description For users on DBR before 16.1, COMMENT ON COLUMN is not supported. We switched to COMMENT ON because it works with types other than table, but here we need to bifurcate. ### Checklist - [x] I have run this code in development and it appears to resolve the stated issue - [x] This PR includes tests, or tests are not required/relevant for this PR - [x] I have updated the `CHANGELOG.md` and added information about my change to the "dbt-databricks next" section. --------- Co-authored-by: Claude <[email protected]>
1 parent b0480b7 commit 74dacfd

File tree

3 files changed

+126
-3
lines changed

3 files changed

+126
-3
lines changed

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
## dbt-databricks 1.10.10 (TBD)
22

3+
### Fixes
4+
- Gate column comment syntax on DBR version for better compatibility ([1151](https://github.com/databricks/dbt-databricks/pull/1151))
5+
36
### Documentation
47
- Update Databricks Job documentation to match current terminology ([1145](https://github.com/databricks/dbt-databricks/pull/1145))
58

dbt/include/databricks/macros/adapters/persist_docs.sql

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,22 @@
1313
{% endmacro %}
1414

1515
{% macro comment_on_column_sql(column_path, escaped_comment) %}
16-
COMMENT ON COLUMN {{ column_path }} IS '{{ escaped_comment }}'
16+
{%- if adapter.compare_dbr_version(16, 1) >= 0 -%}
17+
COMMENT ON COLUMN {{ column_path }} IS '{{ escaped_comment }}'
18+
{%- else -%}
19+
{{ alter_table_change_column_comment_sql(column_path, escaped_comment) }}
20+
{%- endif -%}
21+
{% endmacro %}
22+
23+
{% macro alter_table_change_column_comment_sql(column_path, escaped_comment) %}
24+
{%- set parts = column_path.split('.') -%}
25+
{%- if parts|length >= 4 -%}
26+
{%- set table_path = parts[:-1] | join('.') -%}
27+
{%- set column_name = parts[-1] -%}
28+
ALTER TABLE {{ table_path }} ALTER COLUMN {{ column_name }} COMMENT '{{ escaped_comment }}'
29+
{%- else -%}
30+
{{ exceptions.raise_compiler_error("Invalid column path: " ~ column_path ~ ". Expected format: database.schema.table.column") }}
31+
{%- endif -%}
1732
{% endmacro %}
1833

1934
{% macro databricks__persist_docs(relation, model, for_relation, for_columns) -%}

tests/unit/macros/adapters/test_persist_docs_macros.py

Lines changed: 107 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,15 @@ def mock_model_with_columns(self):
2525

2626
return model
2727

28-
def test_comment_on_column_sql(self, template_bundle):
28+
def test_comment_on_column_sql_dbr_16_1_or_newer(self, template_bundle, context):
29+
"""Test COMMENT ON COLUMN syntax for DBR 16.1+"""
2930
column_path = "`test_db`.`test_schema`.`test_table`.id"
3031
escaped_comment = "This is a column comment"
3132

33+
# Mock adapter to return DBR 16.1+
34+
context["adapter"] = Mock()
35+
context["adapter"].compare_dbr_version = Mock(return_value=0) # >= 16.1
36+
3237
result = self.run_macro(
3338
template_bundle.template, "comment_on_column_sql", column_path, escaped_comment
3439
)
@@ -38,6 +43,64 @@ def test_comment_on_column_sql(self, template_bundle):
3843
"""
3944
self.assert_sql_equal(result, expected_sql)
4045

46+
def test_comment_on_column_sql_dbr_older_than_16_1(self, template_bundle, context):
47+
"""Test ALTER TABLE syntax for DBR < 16.1"""
48+
column_path = "`test_db`.`test_schema`.`test_table`.id"
49+
escaped_comment = "This is a column comment"
50+
51+
# Mock adapter to return DBR < 16.1
52+
context["adapter"] = Mock()
53+
context["adapter"].compare_dbr_version = Mock(return_value=-1) # < 16.1
54+
55+
result = self.run_macro(
56+
template_bundle.template, "comment_on_column_sql", column_path, escaped_comment
57+
)
58+
59+
expected_sql = """
60+
ALTER TABLE `test_db`.`test_schema`.`test_table`
61+
ALTER COLUMN id COMMENT 'This is a column comment'
62+
"""
63+
self.assert_sql_equal(result, expected_sql)
64+
65+
def test_alter_table_change_column_comment_sql(self, template_bundle):
66+
"""Test the legacy ALTER TABLE CHANGE COLUMN syntax"""
67+
column_path = "`test_db`.`test_schema`.`test_table`.id"
68+
escaped_comment = "This is a column comment"
69+
70+
result = self.run_macro(
71+
template_bundle.template,
72+
"alter_table_change_column_comment_sql",
73+
column_path,
74+
escaped_comment,
75+
)
76+
77+
expected_sql = """
78+
ALTER TABLE `test_db`.`test_schema`.`test_table`
79+
ALTER COLUMN id COMMENT 'This is a column comment'
80+
"""
81+
self.assert_sql_equal(result, expected_sql)
82+
83+
def test_alter_table_change_column_comment_sql_invalid_path(self, template_bundle, context):
84+
"""Test error handling for invalid column path"""
85+
column_path = "invalid_path"
86+
escaped_comment = "This is a column comment"
87+
88+
# Mock exceptions module
89+
context["exceptions"] = Mock()
90+
context["exceptions"].raise_compiler_error = Mock(side_effect=Exception("Test error"))
91+
92+
with pytest.raises(Exception, match="Test error"):
93+
self.run_macro(
94+
template_bundle.template,
95+
"alter_table_change_column_comment_sql",
96+
column_path,
97+
escaped_comment,
98+
)
99+
100+
context["exceptions"].raise_compiler_error.assert_called_once_with(
101+
"Invalid column path: invalid_path. Expected format: database.schema.table.column"
102+
)
103+
41104
def test_alter_relation_comment_sql(self, template_bundle, relation):
42105
result = self.run_macro(
43106
template_bundle.template,
@@ -82,7 +145,7 @@ def test_alter_relation_comment_sql_view(self, template_bundle):
82145
)
83146
self.assert_sql_equal(result, expected_sql)
84147

85-
def test_databricks__alter_column_comment_delta(
148+
def test_databricks__alter_column_comment_delta_dbr_16_1_plus(
86149
self, template_bundle, context, relation, mock_model_with_columns
87150
):
88151
context["config"] = Mock()
@@ -91,6 +154,9 @@ def test_databricks__alter_column_comment_delta(
91154
context["api"] = MagicMock()
92155
context["api"].Column.get_name = Mock(side_effect=lambda col: col["name"])
93156

157+
context["adapter"] = Mock()
158+
context["adapter"].compare_dbr_version = Mock(return_value=0) # >= 16.1
159+
94160
context["run_query_as"] = Mock()
95161

96162
self.run_macro_raw(
@@ -117,6 +183,45 @@ def test_databricks__alter_column_comment_delta(
117183
)
118184
self.assert_sql_equal(second_call, expected_second_sql)
119185

186+
def test_databricks__alter_column_comment_delta_dbr_older_than_16_1(
187+
self, template_bundle, context, relation, mock_model_with_columns
188+
):
189+
context["config"] = Mock()
190+
context["config"].get = Mock(return_value="delta")
191+
192+
context["api"] = MagicMock()
193+
context["api"].Column.get_name = Mock(side_effect=lambda col: col["name"])
194+
195+
context["adapter"] = Mock()
196+
context["adapter"].compare_dbr_version = Mock(return_value=-1) # < 16.1
197+
198+
context["run_query_as"] = Mock()
199+
200+
self.run_macro_raw(
201+
template_bundle.template,
202+
"databricks__alter_column_comment",
203+
relation,
204+
mock_model_with_columns.columns,
205+
)
206+
207+
assert context["run_query_as"].call_count == 2
208+
209+
call_args_list = context["run_query_as"].call_args_list
210+
211+
first_call = call_args_list[0][0][0]
212+
expected_first_sql = (
213+
"ALTER TABLE `some_database`.`some_schema`.`some_table` "
214+
"ALTER COLUMN id COMMENT 'Primary key'"
215+
)
216+
self.assert_sql_equal(first_call, expected_first_sql)
217+
218+
second_call = call_args_list[1][0][0]
219+
expected_second_sql = (
220+
"ALTER TABLE `some_database`.`some_schema`.`some_table` "
221+
"ALTER COLUMN value COMMENT 'Contains \\'quoted\\' text'"
222+
)
223+
self.assert_sql_equal(second_call, expected_second_sql)
224+
120225
def test_databricks__alter_column_comment_unsupported_format(
121226
self, template_bundle, context, relation, mock_model_with_columns
122227
):

0 commit comments

Comments
 (0)