Bug/databricks sql incremental #49

fivetran-catfritz · 2024-07-24T21:18:40Z

PR Overview

This PR will address the following Issue/Feature:

[Feature] Update incremental strategy for Databricks SQL Warehouse #50
Updated lookback macro what we use in Netsuite.

This PR will result in the following new package version:

v0.10.0 since we are changing file formats and 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_incremental_compatible macro has been added to return true if the target warehouse supports our chosen incremental strategy.

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 consistency tests for models:

mixpanel__daily_events

mixpanel__event

mixpanel__monthly_events

mixpanel__sessions

Updated logic for macro mixpanel_lookback to align with logic used in similar macros in other packages.

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?

💃

* Update mixpanel_lookback.sql * update changelog

fivetran-joemarkiewicz

@fivetran-catfritz these changes look great! Just a few suggestions and one code update request which is the same as my HubSpot comment. Let me know if you have any questions.

Once these updates are applied the re-review should be good for approval!

fivetran-joemarkiewicz · 2024-07-26T20:28:25Z

CHANGELOG.md

@@ -1,3 +1,26 @@
+# dbt_mixpanel v0.10.0
+
+[PR #48](https://github.com/fivetran/dbt_mixpanel/pull/48) includes the following updates:


Update to the PR link reference

Suggested change

[PR #48](https://github.com/fivetran/dbt_mixpanel/pull/48) includes the following updates:

[PR #49](https://github.com/fivetran/dbt_mixpanel/pull/49) includes the following updates:

Thanks 😅. Updated.

fivetran-joemarkiewicz · 2024-07-26T20:29:17Z

CHANGELOG.md

+- For Databricks SQL Warehouses, incremental materialization will not be used due to the incompatibility of the `insert_overwrite` strategy.
+
+## Under the Hood
+- The `is_incremental_compatible` macro has been added to return `true` if the target warehouse supports our chosen incremental strategy.


Suggested change

- The `is_incremental_compatible` macro has been added to return `true` if the target warehouse supports our chosen incremental strategy.

- The `is_incremental_compatible` macro has been added and will return `true` if the target warehouse supports our chosen incremental strategy.

fivetran-joemarkiewicz · 2024-07-26T20:32:08Z

macros/mixpanel_lookback.sql

@@ -6,30 +6,13 @@

 {% macro default__mixpanel_lookback(from_date, datepart, interval, safety_date='2010-01-01')  %}


In Fivetran Log I noticed we provide default values for datepart and interval. Should we do the same here or is that not necessary?

Ahh good callout. It's not necessary in this package because I defined those variables, but it would make sense to keep things consistent, and I think having the defaults is the better version. Updated!

fivetran-catfritz

@fivetran-joemarkiewicz This one is ready for re-review as well!

fivetran-catfritz · 2024-07-29T16:55:48Z

CHANGELOG.md

@@ -1,3 +1,26 @@
+# dbt_mixpanel v0.10.0
+
+[PR #48](https://github.com/fivetran/dbt_mixpanel/pull/48) includes the following updates:


Thanks 😅. Updated.

fivetran-catfritz · 2024-07-29T16:56:40Z

CHANGELOG.md

+- For Databricks SQL Warehouses, incremental materialization will not be used due to the incompatibility of the `insert_overwrite` strategy.
+
+## Under the Hood
+- The `is_incremental_compatible` macro has been added to return `true` if the target warehouse supports our chosen incremental strategy.


fivetran-catfritz · 2024-07-29T16:58:41Z

macros/mixpanel_lookback.sql

@@ -6,30 +6,13 @@

 {% macro default__mixpanel_lookback(from_date, datepart, interval, safety_date='2010-01-01')  %}


Ahh good callout. It's not necessary in this package because I defined those variables, but it would make sense to keep things consistent, and I think having the defaults is the better version. Updated!

fivetran-joemarkiewicz

@fivetran-catfritz great work on this PR and thanks for addressing my review notes. This PR looks good for release review! I did encounter a few issues when running the validations with internal data, but they were pretty small and we have an easy workaround. Please take a look and address my remaining notes, but no need for those to be a blocker from initiating the release review process.

Great work!

integration_tests/tests/consistency/row_comparisons/consistency_sessions.sql

integration_tests/tests/consistency/row_comparisons/consistency_event.sql

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

fivetran-catfritz

Thank you! I have updated the tests, so now proceeding with release!

integration_tests/tests/consistency/row_comparisons/consistency_event.sql

integration_tests/tests/consistency/row_comparisons/consistency_sessions.sql

fivetran-reneeli

lgtm!

fivetran-catfritz added 4 commits July 23, 2024 10:16

MagicBot/2024-q3-updates updates

4504bd2

Update run_models.sh

4baf4c4

update packages

83a958d

update changelog & readme

3b408fe

fivetran-catfritz linked an issue Jul 24, 2024 that may be closed by this pull request

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

Closed

4 tasks

Update mixpanel_lookback.sql (#45)

70fae6b

* Update mixpanel_lookback.sql * update changelog

fivetran-catfritz self-assigned this Jul 24, 2024

regen docs

39769c2

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 24, 2024 22:12

fivetran-catfritz added 3 commits July 26, 2024 11:08

add consistency tests

8bcd8fc

revisions

6e28dd6

regen docs

e55b819

fivetran-joemarkiewicz requested changes Jul 26, 2024

View reviewed changes

fivetran-catfritz added 2 commits July 29, 2024 12:02

update dispatches

0808ff2

regen docs

f158894

fivetran-catfritz commented Jul 29, 2024

View reviewed changes

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

fivetran-joemarkiewicz approved these changes Jul 29, 2024

View reviewed changes

integration_tests/tests/consistency/row_comparisons/consistency_sessions.sql Outdated Show resolved Hide resolved

integration_tests/tests/consistency/row_comparisons/consistency_event.sql Outdated Show resolved Hide resolved

Apply suggestions from code review

02a89a1

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

fivetran-catfritz commented Jul 29, 2024

View reviewed changes

integration_tests/tests/consistency/row_comparisons/consistency_event.sql Outdated Show resolved Hide resolved

integration_tests/tests/consistency/row_comparisons/consistency_sessions.sql Outdated Show resolved Hide resolved

fivetran-reneeli approved these changes Jul 31, 2024

View reviewed changes

fivetran-catfritz merged commit 153590a into main Jul 31, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/databricks sql incremental #49

Bug/databricks sql incremental #49

fivetran-catfritz commented Jul 24, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 26, 2024

fivetran-catfritz Jul 29, 2024

fivetran-joemarkiewicz Jul 26, 2024

fivetran-catfritz Jul 29, 2024

fivetran-joemarkiewicz Jul 26, 2024

fivetran-catfritz Jul 29, 2024

fivetran-catfritz left a comment

fivetran-catfritz Jul 29, 2024

fivetran-catfritz Jul 29, 2024

fivetran-catfritz Jul 29, 2024

fivetran-joemarkiewicz left a comment

fivetran-catfritz left a comment

fivetran-reneeli left a comment

		@@ -1,3 +1,26 @@
		# dbt_mixpanel v0.10.0

		[PR #48](https://github.com/fivetran/dbt_mixpanel/pull/48) includes the following updates:

	[PR #48](https://github.com/fivetran/dbt_mixpanel/pull/48) includes the following updates:
	[PR #49](https://github.com/fivetran/dbt_mixpanel/pull/49) includes the following updates:

	- The `is_incremental_compatible` macro has been added to return `true` if the target warehouse supports our chosen incremental strategy.
	- The `is_incremental_compatible` macro has been added and will return `true` if the target warehouse supports our chosen incremental strategy.

		@@ -6,30 +6,13 @@

		{% macro default__mixpanel_lookback(from_date, datepart, interval, safety_date='2010-01-01') %}

Bug/databricks sql incremental #49

Bug/databricks sql incremental #49

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

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-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

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-reneeli left a comment

Choose a reason for hiding this comment

fivetran-catfritz commented Jul 24, 2024 •

edited

Loading