Restore drop_schema to adapter in jinja context (#1980) #1983
Conversation
Made a few related fixes to caching behavior w.r.t schemas Added a test
core/dbt/adapters/sql/impl.py
Outdated
| self.execute_macro(DROP_SCHEMA_MACRO_NAME, | ||
| kwargs=kwargs) | ||
| self.execute_macro(DROP_SCHEMA_MACRO_NAME, kwargs=kwargs) | ||
| self.commit_if_has_connection() |
There was a problem hiding this comment.
Do we want to commit here? I'm not sure a consumer of this method would expect a commit to occur, and I can imagine this being dangerous for certain uses!
There was a problem hiding this comment.
I thought we did because the create schema also commits. I sort of assumed they both should, I'll revert it.
There was a problem hiding this comment.
hmm, create_schema should probably not commit - these sprinkled-in transactional statements make it super hard to reason about what dbt is doing in different parts of the codebase.
I do think that committing a create schema feels "safer" than committing the drop - it's the destructive nature of this method that gave me pause. Reverting it sounds good to me -- ship it when the tests pass!
Fixes #1980
I can't remember if we removed this as an adapter method intentionally or accidentally, but it was a mistake either way.
As part of this I fixed up the cache's behavior when you do things to schemas (dropping a schema now cascades into dropping all its contents, as it should). There was an especially tricky bug with renaming relations where we effectively dropped the old relation's schema(!!!)
I added a test, it is basically a big pile of things you should never do, but it tests this functionality well enough. I made the test for each db as this seems like something easy to break on a per-db basis.