Bug/databricks sql incremental #144

fivetran-catfritz · 2024-07-24T17:13:26Z

PR Overview

This PR will address the following Issue/Feature:

T-746924

This PR will result in the following new package version:

v0.18.0 since we're changing materializations

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, we recommend running a --full-refresh after upgrading to this version to avoid possible incremental failures.

For Databricks All-Purpose clusters, incremental models 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. This will also prevent compilation issues on customers' managed tables.

For Databricks SQL Warehouses, incremental materialization will not be used due to the incompatibility of the insert_overwrite strategy.

Under the Hood

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

This update was applied as there have been other Databricks runtimes discovered (ie. an endpoint and external runtime) which do not support the insert_overwrite incremental strategy used.

Added integration testing for Databricks SQL Warehouse.

Added a bot to mark issues and PRs as stale if there is no activity for over 180 days.

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:

Consistency tests pass

Parquet file format

Before addressing the insert_overwrite incompatibility, our Databricks SQL instance is "managed", so I was getting the below error for the parquet file format.
Updating to delta format resolves this issue.

Insert-overwrite incremental strategy

Once the file format issue was resolved, I was able to reproduce the reported issue and get the below error.
Updating with the materialized='incremental' if is_incremental_compatible() else 'table' approach resolved this issue. When running a non-full-refresh run with a target of Databricks SQL, confirm there is no error and tables are created instead of and incremental run.

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

🤖

fivetran-joemarkiewicz

@fivetran-catfritz thanks for quickly turning around this PR! I have a few comments and requests in my review. Additionally, would you be able to add consistency tests for the end models modified in this review. Thanks!

CHANGELOG.md

integration_tests/dbt_project.yml

macros/is_incremental_compatible.sql

.github/workflows/stale.yml

fivetran-catfritz

@fivetran-joemarkiewicz Thanks for the suggestions. I have applied them and also added the consistency tests, which I also added a screencap that they pass. This is ready for re-review!

CHANGELOG.md

integration_tests/dbt_project.yml

.github/workflows/stale.yml

macros/is_incremental_compatible.sql

fivetran-joemarkiewicz

@fivetran-catfritz great work on this PR! I ran through the changes and your validations look great. No notes around the functionality of the PR, but I did come across one request that I realized when taking a look at your Mixpanel PR. I would like us to apply to the macro moving forward (adding the adapter.dispatch and the alias to when we call the macro).

Let me know once that update is applied and that re-review should be good for approval!

CHANGELOG.md

fivetran-joemarkiewicz · 2024-07-26T19:34:31Z

packages.yml

+  # - package: fivetran/hubspot_source
+  #   version: [">=0.15.0", "<0.16.0"]
+  - git: https://github.com/fivetran/dbt_hubspot_source.git
+    revision: update/get-ticket-property-history-columns
+    warn-unpinned: false


Reminder to swap before release

macros/is_incremental_compatible.sql

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

fivetran-catfritz

@fivetran-joemarkiewicz I have updated with your suggestions, and this is ready for re-review.

CHANGELOG.md

macros/is_incremental_compatible.sql

fivetran-joemarkiewicz

@fivetran-catfritz great work on this PR and for addressing my review notes. This PR looks good to go! I do have one open question that can be addressed as a team, but curious if you have any insight into it.

This will be good for release review! Be sure to swap the package dep before merging.

fivetran-joemarkiewicz · 2024-07-29T19:27:13Z

macros/is_incremental_compatible.sql

@@ -0,0 +1,35 @@
+{% macro is_incremental_compatible() -%}
+    {{ return(adapter.dispatch('is_incremental_compatible', 'hubspot') ()) }}


I know we will sometimes provide the return here, but other times we will not. Do you know what the difference is (if there is any)?

Looking at dbt_utils I can see they always provide a return here. However, a spot check on other package maintainers and our own Fivetran Utils I see that a mix of versions are provided. 🤔

Since we can see this is what is used by dbt_utils I would prefer we keep your code. However, I would like to explore what the difference between having return and not causes.

@fivetran-joemarkiewicz I recall having discussing this as a team a while back, and that the conclusion was we should use it, but I'm not sure what the reason was nor can I find a reference to it. I also realize that several of our macros don't use it and seem to function fine, and I've forgotten to use it recently as well.

fivetran-avinash · 2024-07-30T18:16:33Z

.buildkite/hooks/pre-command

+export CI_DATABRICKS_DBT_CATALOG=$(gcloud secrets versions access latest --secret="CI_DATABRICKS_DBT_CATALOG" --project="dbt-package-testing-363917")
+export CI_DATABRICKS_SQL_DBT_HTTP_PATH=$(gcloud secrets versions access latest --secret="CI_DATABRICKS_SQL_DBT_HTTP_PATH" --project="dbt-package-testing-363917")


Lines 26/27 are not present in the equivalent source package pre-command, should they be added or is this outside scope.

fivetran-avinash · 2024-07-30T18:22:26Z

.buildkite/pipeline.yml

+      bash .buildkite/scripts/run_models.sh databricks
+
+  - label: ":databricks: :database: Run Tests - Databricks SQL Warehouse"


Should we add this support to the source package as well or is this out of scope?

integration_tests/ci/sample.profiles.yml

integration_tests/dbt_project.yml

fivetran-avinash

@fivetran-catfritz This looks mostly good! A few comments before approving.

(cc: @fivetran-joemarkiewicz if you are taking this on.)

fivetran-avinash

@fivetran-joemarkiewicz lgtm!

packages.yml

fivetran-catfritz added 3 commits July 23, 2024 17:06

MagicBot/2024-q3-updates

b3cc497

update pkgs

0752c43

update run_models

814f1a5

fivetran-catfritz self-assigned this Jul 24, 2024

fivetran-catfritz added 3 commits July 24, 2024 13:36

update changelog

f25523f

update yml

69fd883

regen docs

3a34a8f

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 24, 2024 20:35

fivetran-catfritz mentioned this pull request Jul 24, 2024

[Feature] Update incremental strategy for Databricks SQL Warehouse fivetran/dbt_mixpanel#50

Closed

4 tasks

fivetran-catfritz added 2 commits July 24, 2024 16:33

update readme

a985855

update readme

9210c9d

fivetran-joemarkiewicz requested changes Jul 25, 2024

View reviewed changes

fivetran-catfritz added 5 commits July 25, 2024 12:12

review updates

885e7ae

add consistency tests

b85f5c9

update changelog

863b92c

update yml

3993c9f

update yml

0b91f67

fivetran-catfritz commented Jul 25, 2024

View reviewed changes

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 25, 2024 17:41

fivetran-joemarkiewicz requested changes Jul 26, 2024

View reviewed changes

fivetran-joemarkiewicz mentioned this pull request Jul 26, 2024

Bug/databricks sql incremental fivetran/dbt_mixpanel#49

Merged

7 tasks

fivetran-catfritz and others added 3 commits July 29, 2024 10:48

Apply suggestions from code review

d96fd90

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

update macro dispatch

a2695a4

regen docs

df737d6

fivetran-catfritz commented Jul 29, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

macros/is_incremental_compatible.sql Outdated Show resolved Hide resolved

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 29, 2024 17:10

fivetran-joemarkiewicz approved these changes Jul 29, 2024

View reviewed changes

fivetran-avinash reviewed Jul 30, 2024

View reviewed changes

integration_tests/ci/sample.profiles.yml Show resolved Hide resolved

fivetran-avinash reviewed Jul 30, 2024

View reviewed changes

integration_tests/dbt_project.yml Show resolved Hide resolved

fivetran-avinash reviewed Jul 30, 2024

View reviewed changes

fivetran-joemarkiewicz requested a review from fivetran-avinash July 30, 2024 20:23

fivetran-avinash approved these changes Jul 30, 2024

View reviewed changes

fivetran-catfritz commented Jul 31, 2024

View reviewed changes

packages.yml Outdated Show resolved Hide resolved

Update packages.yml

c33f45d

fivetran-catfritz merged commit 86b93f6 into main Aug 1, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/databricks sql incremental #144

Bug/databricks sql incremental #144

fivetran-catfritz commented Jul 24, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-catfritz left a comment

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 26, 2024

fivetran-catfritz left a comment

fivetran-joemarkiewicz left a comment •

edited

Loading

fivetran-joemarkiewicz Jul 29, 2024

fivetran-catfritz Jul 29, 2024

fivetran-avinash Jul 30, 2024 •

edited

Loading

fivetran-joemarkiewicz Jul 30, 2024

fivetran-avinash Jul 30, 2024 •

edited

Loading

fivetran-joemarkiewicz Jul 30, 2024

fivetran-avinash left a comment

fivetran-avinash left a comment

		@@ -0,0 +1,35 @@
		{% macro is_incremental_compatible() -%}
		{{ return(adapter.dispatch('is_incremental_compatible', 'hubspot') ()) }}

		export CI_DATABRICKS_DBT_CATALOG=$(gcloud secrets versions access latest --secret="CI_DATABRICKS_DBT_CATALOG" --project="dbt-package-testing-363917")
		export CI_DATABRICKS_SQL_DBT_HTTP_PATH=$(gcloud secrets versions access latest --secret="CI_DATABRICKS_SQL_DBT_HTTP_PATH" --project="dbt-package-testing-363917")

		bash .buildkite/scripts/run_models.sh databricks

		- label: ":databricks: :database: Run Tests - Databricks SQL Warehouse"

Bug/databricks sql incremental #144

Bug/databricks sql incremental #144

Conversation

fivetran-catfritz commented Jul 24, 2024 • edited Loading

PR Overview

🚨 Breaking Changes 🚨

Under the Hood

PR Checklist

Basic Validation

Detailed Validation

Consistency tests pass

Parquet file format

Insert-overwrite incremental strategy

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

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 26, 2024

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment • edited Loading

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 29, 2024

Choose a reason for hiding this comment

fivetran-catfritz Jul 29, 2024

Choose a reason for hiding this comment

fivetran-avinash Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 30, 2024

Choose a reason for hiding this comment

fivetran-avinash Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 30, 2024

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-catfritz commented Jul 24, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment •

edited

Loading

fivetran-avinash Jul 30, 2024 •

edited

Loading

fivetran-avinash Jul 30, 2024 •

edited

Loading