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

Fix table comment updates #750

Merged
merged 6 commits into from
Aug 12, 2024
Merged

Fix table comment updates #750

merged 6 commits into from
Aug 12, 2024

Conversation

henlue
Copy link
Contributor

@henlue henlue commented Jul 30, 2024

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

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-databricks next" section.

@benc-db
Copy link
Collaborator

benc-db commented Jul 30, 2024

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
Copy link
Collaborator

benc-db commented Jul 30, 2024

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
Copy link
Collaborator

benc-db commented Aug 2, 2024

Running functional tests now.

@benc-db
Copy link
Collaborator

benc-db commented Aug 2, 2024

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

Signed-off-by: Hendrik Lüdemann <hen.luedemann@gmail.com>
Signed-off-by: Hendrik Lüdemann <hen.luedemann@gmail.com>
Signed-off-by: Hendrik Lüdemann <hen.luedemann@gmail.com>
@henlue
Copy link
Contributor Author

henlue commented Aug 6, 2024

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

@benc-db
Copy link
Collaborator

benc-db commented Aug 6, 2024

Rerunning functional tests

Signed-off-by: Hendrik Lüdemann <hen.luedemann@gmail.com>
@henlue
Copy link
Contributor Author

henlue commented Aug 7, 2024

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

@benc-db
Copy link
Collaborator

benc-db commented Aug 9, 2024

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
Copy link
Collaborator

benc-db commented Aug 12, 2024

Does this by any chance fix: #763?

benc-db
benc-db previously approved these changes Aug 12, 2024
@benc-db benc-db merged commit 198b569 into databricks:main Aug 12, 2024
18 checks passed
@henlue
Copy link
Contributor Author

henlue commented Aug 13, 2024

Does this by any chance fix: #763?

No that issue still persists

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relation comments are not being updated for incremental models
2 participants