Fix table comment updates - Githubissues

databricks / dbt-databricks

A dbt adapter for Databricks.

https://databricks.com

Apache License 2.0

228 stars 119 forks source link

Fix table comment updates #750

Closed henlue closed 3 months ago

henlue commented 3 months ago

Resolves #732

Description

Update table comments for incremental models on incremental runs
Update table comments for snapshots on incremental runs
Create table comments during dbt clone
Check for config.persist_relation_docs() in persist_docs
Align black versions in black.ini and .pre-commit-config.yaml to fix failing pre-commit runs

The issue is only about the incremental models. While working on the PR I realized table comments are also not being updated for snapshots and are not created with dbt clone.

I think comments are also not being updated in materialized views and streaming tables. Since I don't have a good understanding of those materializations, I kept the original behavior by setting for_relation=False. The tests would also run through with for_relation=True, but this might lead to unnecessary queries that are being send to databricks on the first run of those relations.

Generally, the code would be simpler if relation comments are only persisted in persist_docs (using comment on ... queries) and not as part of create table statements. I assume they are persisted as part of the create statement to improve performance?

I've ran: tox -e unit tox -e integration-databricks-uc-sql-endpoint tox -e integration-databricks-uc-cluster

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.

benc-db commented 3 months ago

I think comments are also not being updated in materialized views and streaming tables. Since I don't have a good understanding of those materializations, I kept the original behavior by setting for_relation=False. The tests would also run through with for_relation=True, but this might lead to unnecessary queries that are being send to databricks on the first run of those relations.

They should already for MV/ST, but the mechanism is very different. Look at alter.sql for either. Core reason is that for MV/ST we have a mechanism for declaring how we want to handle changes (on_config_change), but I haven't convinced dbt-core yet that we should have the same mechanism for incremental tables.

benc-db commented 3 months ago

Basically, in the absence of on_config_change for other materialization types, you are assuming users want apply. I think that is the correct assumption. Thanks for PR, will review in the next day or so.

benc-db commented 3 months ago

Running functional tests now.

benc-db commented 3 months ago

Please take a look at the non-uc cluster tests; there are few that are failing.

henlue commented 3 months ago

I've fixed the tests. I was also able to get tox -e integration-databricks-cluster running to check them.

benc-db commented 3 months ago

Rerunning functional tests

henlue commented 3 months ago

I forgot to sign my latest commit and had to force push it again now.

benc-db commented 3 months ago

I forgot to sign my latest commit and had to force push it again now.

Apologies for delay, I'm on-call and haven't had much time to review PRs. Will get to this shortly.

benc-db commented 3 months ago

Does this by any chance fix: https://github.com/databricks/dbt-databricks/issues/763?

henlue commented 3 months ago

Does this by any chance fix: #763?

No that issue still persists