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

Support persisting docs for materialized_view materializations #98

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

morsapaes
Copy link
Contributor

@morsapaes morsapaes commented May 17, 2024

The macro that backs object-level persist_docs uses {{ relation.type }} to derive the object type in the COMMENT ON statement. This breaks for materialized_view materializations:

15:35:01  Database Error in model funds_movement (models/marts/funds_movement.sql)
15:35:01    Expected one of TABLE or VIEW or COLUMN or MATERIALIZED or SOURCE or SINK or INDEX or FUNCTION or CONNECTION or TYPE or SECRET or ROLE or DATABASE or SCHEMA or CLUSTER, found identifier "materialized_view"
15:35:01    LINE 5:   comment on materialized_view "marta"."public"."funds_moveme...
15:35:01                         ^
15:35:01    compiled Code at target/run/mz_get_started/models/marts/funds_movement.sql
15:35:01
15:35:01  Done. PASS=3 WARN=0 ERROR=2 SKIP=0 TOTAL=5

We've fixed this a while back for dbt-materialize (which shims dbt-postgres) in Materialize #21878, but it really should be fixed at the source!


Fixes #120, fixes #11.

@morsapaes morsapaes requested a review from a team as a code owner May 17, 2024 11:03
Copy link

cla-bot bot commented May 17, 2024

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @morsapaes

@morsapaes
Copy link
Contributor Author

Signed the CLA, but...not sure if some manual steps are needed to thread that through?

@cla-bot cla-bot bot added the cla:yes label Jun 26, 2024
Copy link
Contributor

@dataders dataders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morsapaes thanks! just did some stuff and the tests are running now.

I see that dbt-materialize inherits from dbt-postgres. Does this proposal fix both a postgres issue and materialize issue?

Two things. Can you:

  1. make a changelog entry with changie (docs)? Integration tests are blocked for that reason
  2. open (or link to) an issue on this repo? This is just for tracking purposes?

@morsapaes
Copy link
Contributor Author

Does this proposal fix both a postgres issue and materialize issue?

It fixes a dbt-postgres issue that is inherited by our adapter (and others shimming dbt-postgres). We put up a patch a while back in dbt-materialize (Materialize #21878), since it was blocking our users.

Two things. Can you:
make a changelog entry with changie (docs)? Integration tests are blocked for that reason
open (or link to) an issue on this repo? This is just for tracking purposes?

Opened #120 for tracking purposes, and marked this PR as fixing it. Will push a changelog entry shortly, too.

Thanks, @dataders!

@cla-bot cla-bot bot added the cla:yes label Jun 26, 2024
@mikealfare
Copy link
Contributor

Thanks for the contribution @morsapaes! Do you mind adding a test case for this? We want to at least make sure that a user can submit a materialized view model with a description. Bonus points for verifying that the materialized view has the expected comment.

@morsapaes
Copy link
Contributor Author

I will as soon as I have some bandwidth this week, @mikealfare.

@colin-rogers-dbt colin-rogers-dbt self-assigned this Jul 3, 2024
@colin-rogers-dbt
Copy link
Contributor

@morsapaes took a quick pass at some tests, we can open an issue to expand out base adapter persist docs tests to cover materialized views

@colin-rogers-dbt colin-rogers-dbt merged commit e7c4f79 into dbt-labs:main Jul 8, 2024
15 checks passed
@colin-rogers-dbt colin-rogers-dbt added the community This PR is from a community member label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes community This PR is from a community member ok to test
Projects
None yet
4 participants