bugfix/is-incremental-compatible #4

fivetran-joemarkiewicz · 2024-07-15T14:21:20Z

PR Overview

This PR will address the following Issue/Feature: Internal Ticket T-739367

This PR will result in the following new package version: v0.2.0

This will be a breaking change due to the nature of Databricks incremental runs resulting in a breaking change as the table format will change for all Databricks incremental models as well as non databricks runtimes will not leverage an incremental strategy for the impacted models.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

🚨 Breaking Changes 🚨

⚠️ Since the following changes result in the table format changing for Databricks users, we recommend running a --full-refresh after upgrading to this version to avoid possible incremental failures.

For Databricks All-Purpose clusters, the salesforce_marketing_cloud__events_enhanced model will now be materialized using the delta table format (previously parquet).

Delta tables are generally more performant than parquet and are also more widely available for Databricks users. Previously, the parquet file format was causing compilation issues on customers' managed tables.

Documentation

Added details to the README to highlight the incremental strategies used within the salesforce_marketing_cloud__events_enhanced model.

Under the Hood

The is_incremental_compatible macro has been added to the package. This macro will return true if the Databricks runtime being used is an all-purpose cluster or if any other non-Databricks supported destination is being used.

This update was applied as there are other Databricks runtimes (ie. sql warehouse, endpoint, and external runtime) which do not support the insert_overwrite incremental strategy used in the salesforce_marketing_cloud__events_enhanced model.

In addition to the above, for Databricks users the salesforce_marketing_cloud__events_enhanced model will now leverage the incremental strategy only if the Databricks runtime is all-purpose. Otherwise, all other Databricks runtimes will not leverage an incremental strategy.

Added validation tests to the integration_tests/ folder to ensure the consistency and integrity of the salesforce_marketing_cloud__events_enhanced model for subsequent updates.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned.
All necessary documentation and version upgrades have been applied.
docs were regenerated (unless this PR does not include any code or yml updates).
BuildKite integration tests are passing.
Detailed validation steps have been provided below.

Detailed Validation

Please share any and all of your validation steps:

Please see the validation tests passing below using the following variables for BigQuery:

vars:
  salesforce_marketing_cloud_database: <testing-project> 
  salesforce_marketing_cloud_schema: <testing-schema> 
  salesforce_marketing_cloud__link_enabled: false
  salesforce_marketing_cloud__list_enabled: false

Additionally, we can see the Databricks incremental logic is working as expected.

✅ Databricks Runtime is using the insert-overwrite incremental logic on subsequent runs as expected.
✅ Databricks SQL Warehouse (also represents the behavior for all non Databricks Runtimes) is not using the incremental logic on subsequent runs as expected.
Ensuring the other warehouses are not impacted, I can confirm that Snowflake, BigQuery, Redshift, and Postgres are all still using the appropriate incremental logic on subsequent runs.
- ✅ Snowflake
- ✅ BigQuery
- ✅ Postgres
- ✅ Redshift

If you had to summarize this PR in an emoji, which would it be?

🧱

fivetran-catfritz

@fivetran-joemarkiewicz Everything is running well locally! I ran full refresh and incremental runs on bigquery, databricks all purpose, and databricks sql, and confirm all run the expected incremental strategy.

I have some comments/questions for the integrations tests, so just let me know if you want to chat about those!

fivetran-catfritz · 2024-07-16T18:26:49Z

.buildkite/scripts/run_models.sh

@@ -16,12 +16,21 @@ db=$1
 echo `pwd`
 cd integration_tests
 dbt deps
+if [ "$db" = "databricks-sql" ]; then
+dbt seed --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db" --full-refresh


Suggested change

dbt seed --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db" --full-refresh

dbt seed --vars '{salesforce_marketing_cloud_schema: sfmc_sqlw_tests}' --target "$db" --full-refresh

The var here just needs to be updated--I think this will fix the table not found messages in the last buildkite run.
I would also make these vars specific for sfmc to keep separated from other packages sql warehouse schemas.

Ah yes 🤦 that was a copy/paste error on my end. Thanks!

fivetran-catfritz · 2024-07-16T18:29:59Z

integration_tests/ci/sample.profiles.yml

+      catalog: "{{ env_var('CI_DATABRICKS_DBT_CATALOG') }}"
+      host: "{{ env_var('CI_DATABRICKS_DBT_HOST') }}"
+      http_path: "{{ env_var('CI_DATABRICKS_SQL_DBT_HTTP_PATH') }}"
+      schema: sqlw_tests 


update to the schema name here too!

fivetran-catfritz · 2024-07-16T19:10:22Z

integration_tests/dbt_project.yml

@@ -21,6 +21,10 @@ vars:
    salesforce_marketing_cloud_link_send_identifier: "link_send_data"
    salesforce_marketing_cloud_triggered_send_identifier: "triggered_send_data"

+models: 
+  +schema: "{{ 'sqlw_tests' if target.name == 'databricks-sql' else 'salesforce_marketing_cloud' }}"


update to the schema name here too!

integration_tests/tests/consistency/consistency_events_enhanced.sql

fivetran-catfritz · 2024-07-16T19:22:45Z

.buildkite/scripts/run_models.sh

 dbt seed --target "$db" --full-refresh
 dbt run --target "$db" --full-refresh
 dbt test --target "$db"
 dbt run --target "$db"
 dbt test --target "$db"
 dbt run --vars '{salesforce_marketing_cloud__link_enabled: false, salesforce_marketing_cloud__list_enabled: false}' --full-refresh --target "$db"
 dbt test --target "$db"
-
+fi
 dbt run-operation fivetran_utils.drop_schemas_automation --target "$db"


In dbt_fivetran_log, do you recall why these lines were included in run_models.sh? It seems like all is functioning here without it, so just curious!

Those were just necessary for the Azure SQL Server support. Different from Databricks SQL Warehouse. No need for that here since we are not supporting Azure SQL Server in this model yet.

Makes sense--thank you!

README.md

Co-authored-by: fivetran-catfritz <[email protected]>

…d.sql Co-authored-by: fivetran-catfritz <[email protected]>

fivetran-joemarkiewicz · 2024-07-16T21:28:20Z

Thanks @fivetran-catfritz for the review and comments/suggestions! I just applied the updates and this is good for re-review. Let me know if you have any other comments.

fivetran-catfritz

So nice to have staging and transform all in one place. Thanks for the responses, and all lgtm!

fivetran-catfritz · 2024-07-16T22:44:27Z

.buildkite/scripts/run_models.sh

 dbt seed --target "$db" --full-refresh
 dbt run --target "$db" --full-refresh
 dbt test --target "$db"
 dbt run --target "$db"
 dbt test --target "$db"
 dbt run --vars '{salesforce_marketing_cloud__link_enabled: false, salesforce_marketing_cloud__list_enabled: false}' --full-refresh --target "$db"
 dbt test --target "$db"
-
+fi
 dbt run-operation fivetran_utils.drop_schemas_automation --target "$db"


Makes sense--thank you!

fivetran-avinash

@fivetran-joemarkiewicz Looking good! Just a few questions/suggestions before approving.

CHANGELOG.md

models/salesforce_marketing_cloud__events_enhanced.sql

fivetran-avinash · 2024-07-17T18:23:44Z

.buildkite/scripts/run_models.sh

@@ -16,12 +16,21 @@ db=$1
 echo `pwd`
 cd integration_tests
 dbt deps
+if [ "$db" = "databricks-sql" ]; then
+dbt seed --vars '{salesforce_marketing_cloud_schema: sfmc_sqlw_tests}' --target "$db" --full-refresh


Should there be a dbt compile statement after this line, similar to dbt_fivetran_log (link)?

A compile statement is not necessary. A compile will already be run as part of the run operation. The compile in Fivetran Log may have been for a particular reason, but I don't imagine we need to include it here.

fivetran-avinash · 2024-07-17T18:24:48Z

.buildkite/scripts/run_models.sh

+dbt test --vars '{salesforce_marketing_cloud_schema: sfmc_sqlw_tests}' --target "$db"
+dbt run --vars '{salesforce_marketing_cloud_schema: sfmc_sqlw_tests}' --target "$db"
+dbt test --vars '{salesforce_marketing_cloud_schema: sfmc_sqlw_tests}' --target "$db"
+dbt run --vars '{salesforce_marketing_cloud_schema: sfmc_sqlw_tests, salesforce_marketing_cloud__link_enabled: false, salesforce_marketing_cloud__list_enabled: false}' --full-refresh --target "$db"


Should there be a dbt run (incremental) after the full-refresh, similar to fivetran_log? (link)

Yeah including an incremental run after a full refresh would be best here. Added to both sql warehouse and normal sections of the run_models.sh

README.md

Co-authored-by: Avinash Kunnath <[email protected]>

fivetran-avinash

Approved!

bugfix/is-incremental-compatible

2400fe5

fivetran-joemarkiewicz self-assigned this Jul 15, 2024

PR link ref in CHANGELOG

ddafee0

fivetran-joemarkiewicz marked this pull request as ready for review July 15, 2024 15:26

fivetran-catfritz requested changes Jul 16, 2024

View reviewed changes

fivetran-joemarkiewicz and others added 3 commits July 16, 2024 15:31

Update README.md

ccf5bf0

Co-authored-by: fivetran-catfritz <[email protected]>

Update integration_tests/tests/consistency/consistency_events_enhance…

15f94d8

…d.sql Co-authored-by: fivetran-catfritz <[email protected]>

review updates

763663e

fivetran-joemarkiewicz requested a review from fivetran-catfritz July 16, 2024 21:27

fivetran-catfritz approved these changes Jul 16, 2024

View reviewed changes

fivetran-avinash self-requested a review July 17, 2024 18:10

fivetran-avinash reviewed Jul 17, 2024

View reviewed changes

fivetran-joemarkiewicz and others added 2 commits July 17, 2024 13:29

Apply suggestions from code review

02da2aa

Co-authored-by: Avinash Kunnath <[email protected]>

address release review notes

3778c0e

fivetran-avinash approved these changes Jul 17, 2024

View reviewed changes

fivetran-joemarkiewicz merged commit 8cf68f5 into main Jul 17, 2024
9 checks passed

fivetran-joemarkiewicz deleted the bugfix/is-incremental-compatible branch July 17, 2024 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix/is-incremental-compatible #4

bugfix/is-incremental-compatible #4

fivetran-joemarkiewicz commented Jul 15, 2024 •

edited

Loading

fivetran-catfritz left a comment

fivetran-catfritz Jul 16, 2024

fivetran-joemarkiewicz Jul 16, 2024

fivetran-joemarkiewicz Jul 16, 2024

fivetran-catfritz Jul 16, 2024

fivetran-joemarkiewicz Jul 16, 2024

fivetran-catfritz Jul 16, 2024

fivetran-joemarkiewicz Jul 16, 2024

fivetran-catfritz Jul 16, 2024

fivetran-joemarkiewicz Jul 16, 2024

fivetran-catfritz Jul 16, 2024

fivetran-joemarkiewicz commented Jul 16, 2024

fivetran-catfritz left a comment

fivetran-catfritz Jul 16, 2024

fivetran-avinash left a comment

fivetran-avinash Jul 17, 2024

fivetran-joemarkiewicz Jul 17, 2024

fivetran-avinash Jul 17, 2024

fivetran-joemarkiewicz Jul 17, 2024

fivetran-avinash left a comment

	dbt seed --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db" --full-refresh
	dbt seed --vars '{salesforce_marketing_cloud_schema: sfmc_sqlw_tests}' --target "$db" --full-refresh

bugfix/is-incremental-compatible #4

bugfix/is-incremental-compatible #4

Conversation

fivetran-joemarkiewicz commented Jul 15, 2024 • edited Loading

PR Overview

🚨 Breaking Changes 🚨

Documentation

Under the Hood

PR Checklist

Basic Validation

Detailed Validation

If you had to summarize this PR in an emoji, which would it be?

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz commented Jul 16, 2024

fivetran-catfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz commented Jul 15, 2024 •

edited

Loading