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

[CT-242] Release notes for v0.21.0 misleading #4732

Closed
dorachan2000 opened this issue Feb 15, 2022 · 2 comments
Closed

[CT-242] Release notes for v0.21.0 misleading #4732

dorachan2000 opened this issue Feb 15, 2022 · 2 comments
Labels
user docs [docs.getdbt.com] Needs better documentation

Comments

@dorachan2000
Copy link

Hello.

I noticed that the "Breaking changes" section of the v0.21.0 release notes mentioned the following

Turn off transactions and turn on autocommit by default. Explicitly specify begin and commit for DML statements in incremental and snapshot materializations.

This is misleading because it suggests that dbt users need to modify their dbt code even though dbt still wraps DML statements in incremental materialization in "begin" and "commit". The expected behavior is verified with the debug output of dbt running an incremental materialization.

$ pip freeze | grep dbt
dbt-core==1.0.1
dbt-extractor==0.4.0
dbt-snowflake==1.0.0

------------------------------------------------

23:43:29.508174 [debug] [Thread-1  ]: On model.dataeng.sellside_conversion_events: begin;
23:43:29.738323 [debug] [Thread-1  ]: SQL status: SUCCESS 1 in 0.23 seconds
23:43:29.739150 [debug] [Thread-1  ]: Using snowflake connection "model.dataeng.sellside_conversion_events"
23:43:29.739755 [debug] [Thread-1  ]: On model.dataeng.sellside_conversion_events: delete from dataeng_stage.reviews.sellside_conversion_events
    where (network || '_' || conversion_date) in (
        select (network || '_' || conversion_date)
        from dataeng_stage.reviews.sellside_conversion_events__dbt_tmp
    );
23:43:31.396181 [debug] [Thread-1  ]: SQL status: SUCCESS 10 in 1.66 seconds
23:43:31.397025 [debug] [Thread-1  ]: Using snowflake connection "model.dataeng.sellside_conversion_events"
23:43:31.397649 [debug] [Thread-1  ]: On model.dataeng.sellside_conversion_events: insert into dataeng_stage.reviews.sellside_conversion_events ("NETWORK", "CONVERSION_DATE", "DATA_TZ", "SESSION_ID", "VERTICAL", "ADVERTISER", "EVENT", "REVENUE", "BUYSIDE_DIMENSIONS", "ROAS_TRACKING", "SOURCE_INFO", "LINEAGE")
    (
        select "NETWORK", "CONVERSION_DATE", "DATA_TZ", "SESSION_ID", "VERTICAL", "ADVERTISER", "EVENT", "REVENUE", "BUYSIDE_DIMENSIONS", "ROAS_TRACKING", "SOURCE_INFO", "LINEAGE"
        from dataeng_stage.reviews.sellside_conversion_events__dbt_tmp
    );
23:43:32.622635 [debug] [Thread-1  ]: SQL status: SUCCESS 10 in 1.22 seconds
23:43:32.623389 [debug] [Thread-1  ]: Using snowflake connection "model.dataeng.sellside_conversion_events"
23:43:32.624332 [debug] [Thread-1  ]: On model.dataeng.sellside_conversion_events: commit

I believe the release notes came directly from the pr comments but it should be modified to be something like the following

Turn off transactions and turn on autocommit by default for non-DML statements.
DML statements running outside of dbt materializations should be explicitly wraped in `begin` and `commit`.
Note that this may affect user-space code that depends on transactions.
@github-actions github-actions bot changed the title Release notes for v0.21.0 misleading [CT-242] Release notes for v0.21.0 misleading Feb 15, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 16, 2022

@dorachan2000 Thanks for this! I agree with your suggested language. I think we'll want to update:

I do believe we should keep this under Breaking Changes. This came up just the other day, where a user upgrading from v0.20 needed to adjust user-space code to account for this change: #4721

@jtcohen6 jtcohen6 added user docs [docs.getdbt.com] Needs better documentation and removed triage labels Feb 16, 2022
@jtcohen6 jtcohen6 self-assigned this Feb 16, 2022
@jtcohen6
Copy link
Contributor

Language I settled on:

  • Breaking: Turn off transactions and turn on autocommit by default. Within dbt materializations, wrap DML statements in explicit begin and commit. Note that it is not recommended to run DML statements outside of dbt materialization logic. If you do this, despite our recommendation, you will need to wrap those statements in explicit begin and commit. Note also that this may affect user-space code that depends on transactions, such as pre-hooks and post-hooks that specify transaction: true or transaction: false. We recommend removing those references to transactions. (#2748, #3480, #3510)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
Development

No branches or pull requests

2 participants