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

Respect transient config for dbt Python models #802

Merged
merged 7 commits into from
Oct 11, 2023

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Oct 11, 2023

resolves #776
branched from #777
No docs needed

Problem

Python models don't respect transient config.

  1. By default, SQL tables are created as transient (as long as not temporary).
  2. By default, Python tables are created as permanent because the table_type parameter isn't specified here.

A side problem is that we are currently using the create_temp_table parameter which is deprecated.

Solution

The changes in this PR will allow Python tables to respect the transient config - in line with SQL tables described in (1) above. It also stops using the deprecated create_temp_table parameter in favor of using table_type instead.

This PR adopts the interpretation that py_write_table is meant to be a private helper macro of the table materialization. As such, it's a "naive" helper rather than being stand-alone method.

Under that interpretation, we could probably justify making a backwards-incompatible change to the method signature. But out of an abundance of caution, this PR keeps py_write_table as backwards-compatible. The one caveat is that any usage of the old signature will not respect transient config.

Alternative solution

Alternatively, we could just go ahead and make the backwards-incompatible change and declare that py_write_table has always been a private macro only intended to be called from within the table materialization. It will just create whichever type of table it is passed from within the materialization.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc)

@cla-bot cla-bot bot added the cla:yes label Oct 11, 2023
@dbeatty10 dbeatty10 mentioned this pull request Oct 11, 2023
4 tasks
@mikealfare
Copy link
Contributor

@dbeatty10 While this is running, have you thought about what tests would be needed, or how existing testing would cover this change?

@dbeatty10
Copy link
Contributor Author

@dbeatty10 While this is running, have you thought about what tests would be needed, or how existing testing would cover this change?

I hadn't given the testing any thought.

Just poked around a little bit, and there are a couple tests for python models here.

But none of them appeared to verify the underlying table type that Snowflake is using. Tried a quick search for transient and temporary, and didn't see any other tests in which we are verifying that table type.

So up to you how to approach the testing here. One option would be to outline the cases we care about and do a set of manual tests to verify those cases.

@dbeatty10 dbeatty10 marked this pull request as ready for review October 11, 2023 21:57
@dbeatty10 dbeatty10 requested a review from a team as a code owner October 11, 2023 21:57
@mikealfare mikealfare merged commit 36fe646 into main Oct 11, 2023
23 checks passed
@mikealfare mikealfare deleted the dbeatty/transient-python-models branch October 11, 2023 23:54
philippe-boyd-maxa pushed a commit to maxa-ai/dbt-snowflake that referenced this pull request Nov 27, 2023
* add transient configs

* Backwards-compatible handling of `temporary` and `transient` configs for dbt python models

* Fix Jinja syntax errors

* add test for table_type on python models

---------

Co-authored-by: jeremyyeo <[email protected]>
Co-authored-by: colin-rogers-dbt <[email protected]>
Co-authored-by: Mike Alfare <[email protected]>
Co-authored-by: Mike Alfare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ADAP-901] [Bug] Not possible to set transient config on Python models
4 participants