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

[Feature] dbt_clone sf implementation #664

Merged
merged 32 commits into from
Jul 11, 2023
Merged

Conversation

McKnight-42
Copy link
Contributor

@McKnight-42 McKnight-42 commented Jun 16, 2023

resolves #672

Description

adding the macros needed on adapter side for dbt_clone and trying to design adapter zone version of Clone tests from defer_state in dbt-labs/dbt-core#7881

Checklist

@McKnight-42 McKnight-42 self-assigned this Jun 16, 2023
@cla-bot cla-bot bot added the cla:yes label Jun 16, 2023
@McKnight-42 McKnight-42 requested a review from aranke June 16, 2023 21:10
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the dbt-snowflake contributing guide.

@McKnight-42 McKnight-42 requested a review from VersusFacit June 16, 2023 21:11
@McKnight-42
Copy link
Contributor Author

McKnight-42 commented Jun 16, 2023

currently when running the TestClonePossible::test_clone tests we failed at assert [r.message for r in results] == ["CREATE VIEW"] * 1 because adapter_response has the message as SUCCESS 1 or SUCCESS instead of a CREATE statement in snowflake

While I like the Base class -> forking Base classes for both scenarios we want to test against think I would like to separate it setup from the stuff found in defer_state as much as we can since we decided those tests aren't adapter specific and thus removed them. any thoughts on this @jtcohen6 @aranke

@aranke
Copy link
Member

aranke commented Jun 20, 2023

@McKnight-42 Separating the tests here from the boilerplate in defer_state makes sense, thanks for doing this.

I'll leave it up to you whether to test the messages or not, but we should test that we get 3 table materializations and 1 view materialization in Snowflake.

@McKnight-42
Copy link
Contributor Author

@McKnight-42 Separating the tests here from the boilerplate in defer_state makes sense, thanks for doing this.

I'll leave it up to you whether to test the messages or not, but we should test that we get 3 table materializations and 1 view materialization in Snowflake.

yeah figure once I get these tests working for snowflake I'll go remove the defer_state test in core and just create a dbt_clone adapter zone test we can pull into adapters to run.

Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

Love how short this PR has become.
LGTM!

@aranke aranke changed the title [Feeature] dbt_clone sf implementation [Feature] dbt_clone sf implementation Jun 22, 2023
@McKnight-42 McKnight-42 marked this pull request as ready for review June 22, 2023 20:04
@McKnight-42 McKnight-42 requested a review from a team as a code owner June 22, 2023 20:04
@McKnight-42
Copy link
Contributor Author

flagging that only failing tests are for dynamic tables currently.

from dbt.tests.adapter.dbt_clone.test_dbt_clone import BaseClonePossible


class TestSnowflakeClonePossible(BaseClonePossible):
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to test transient and copy_grants?

Copy link
Contributor Author

@McKnight-42 McKnight-42 Jul 11, 2023

Choose a reason for hiding this comment

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

this admittedly is more pulled from Jeremey's draft pr where he created a snowflake implementation where he does talk about the use case for transient https://github.com/dbt-labs/dbt-core/pull/7258/files#diff-073e6ed96ac92033f0b921e061b47226b87d4f358350a1ed94fc94165f247b7aR39

while in the docs does mention copy grants

@McKnight-42 McKnight-42 merged commit 641745f into main Jul 11, 2023
@McKnight-42 McKnight-42 deleted the feature/dbt-clone-sf branch July 11, 2023 22:37
philippe-boyd-maxa pushed a commit to maxa-ai/dbt-snowflake that referenced this pull request Nov 27, 2023
* update RELEASE_BRANCH env

* initial implementation of snowflake dbt_clone macros and test design attempts

* prove tests throw expected values if expected use caes bases on bool macro

* prove tests throw expected values if expected use caes bases on bool macro

* start to move towards importing adapter version of test

* macro name fix, file rename

* change state_relation to defer_relation to match change in dbt-core

* change pointer in dev-requirements and add changelog

* add clean_up method to drop alt schema names after tests run

* change pointer back to main

* clone transient table test

---------

Co-authored-by: Kshitij Aranke <kshitij.aranke@dbtlabs.com>
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-651] [Feature] dbt-snowflake Cloning from production
3 participants