feature/databricks-sql-warehouse-compatibility #121

fivetran-joemarkiewicz · 2024-04-02T15:31:07Z

PR Overview

This PR will address the following Issue/Feature: Issue #120

This PR will result in the following new package version: v1.7.1

This will not impact existing users who are not using Databricks SQL Warehouse runtimes. A SQL Warehouse runtime user will never have seen success. Therefore, this fix is not breaking and should ensure they may now see success.

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

Bug Fixes

Users leveraging the Databricks SQL Warehouse runtime were previously unable to run the fivetran_platform__audit_table model due to an incompatible incremental strategy. As such, the following updates have been made:

A new macro is_databricks_sql_warehouse() has been added to determine if a databricks runtime is a SQL Warehouse runtime for Databricks. This macro will return a boolean of true if the runtime is determined to be SQL Warehouse and false if it is any other runtime or destination.

The above macro is used in determining the incremental strategy within the fivetran_platform__audit_table. For Databricks SQL Warehouses, there will be no incremental strategy used. All other destination runtime strategies are not impacted with this change.

For the SQL Warehouse runtime, the best incremental strategy we could elect to use is the merge strategy. However, we do not have full confidence in the resulting data integrity of the output model when leveraging this strategy. Therefore, we opted for the model to replicate a full create or replace behavior for the time being.

Under the Hood

Added integration testing pipeline for Databricks SQL Warehouse.

Applied modifications to the integration testing pipeline to account for jobs being run on both Databricks All Purpose Cluster and SQL Warehouse runtimes.

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:

For validating these changes I wanted to ensure the following in relation to the fivetran_platform__audit_table model:

No incremental strategy is being used for SQL Warehouse destinations
The incremental strategy created for all other warehouses is unchanged

See the validations below:

SQL Warehouse doesn't have any incremental strategy associated with it. You can see there is no incremental strategy used as the incremental run compiled code does not include the where statement that would identify the incremental strategy is being used.
I was then able to verify for all other warehouses that the incremental strategy was working as expected. You can see the compiled code for all the warehouses is returning the expected results.
- BigQuery
- Databricks (All Purpose Cluster)
  - I have been unable to run this locally. However, we can see from buildkite that this looks to be using the incremental strategy as expected. We can see this in the finished statement where it mentions there was 1 incremental model executed.
- Postgres
- Snowflake
- Redshift
- SQL Server
  - I am unable to run this locally, but same as Databricks (All Purpose Clusters) we are able to see the incremental model is being captured in the BuildKite run

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

🧱

fivetran-joemarkiewicz · 2024-04-02T20:01:04Z

macros/is_databricks_sql_warehouse.sql

+{% macro is_databricks_sql_warehouse(target) %}
+    {% if target.type in ('databricks','spark') %}
+        {% set re = modules.re %}
+        {% set path_match = target.http_path %}
+        {% set regex_pattern = "/sql/.+/warehouses/" %}
+        {% set match_result = re.search(regex_pattern, path_match) %}
+        {% if match_result %}
+            {{ return(True) }}
+        {% else %}
+            {{ return(False) }}
+        {% endif %}
+    {% else %}
+        {{ return(False) }}
+    {% endif %}
+{% endmacro %}


I also have an open question on dbt Slack which asks if there is a better way to do this natively using the dbt-databricks adapter.

It seems per dbt Slack that this is the preferred route.

fivetran-joemarkiewicz · 2024-04-02T23:18:12Z

.buildkite/scripts/run_models.sh

+if [ "$db" = "databricks-sql" ]; then
+dbt seed --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db" --full-refresh
+dbt compile --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db"
+dbt run --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db" --full-refresh
+dbt run --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db"
+dbt test --vars '{fivetran_platform_schema: sqlw_tests}' --target "$db"
+dbt run --vars '{fivetran_platform_schema: sqlw_tests, fivetran_platform__usage_pricing: true}' --target "$db" --full-refresh
+dbt run --vars '{fivetran_platform_schema: sqlw_tests, fivetran_platform__usage_pricing: true}' --target "$db"
+dbt test --target "$db"
+dbt run --vars '{fivetran_platform_schema: sqlw_tests, fivetran_platform__credits_pricing: false, fivetran_platform__usage_pricing: true}' --target "$db" --full-refresh
+dbt run --vars '{fivetran_platform__credits_pricing: false, fivetran_platform__usage_pricing: true}' --target "$db"
+dbt test --vars '{fivetran_platform_schema: sqlw_tests}'  --target "$db"
+dbt run --vars '{fivetran_platform_schema: sqlw_tests, fivetran_platform__usage_pricing: false, fivetran_platform_using_destination_membership: false, fivetran_platform_using_user: false}' --target "$db" --full-refresh
+dbt run --vars '{fivetran_platform_schema: sqlw_tests, fivetran_platform__usage_pricing: false, fivetran_platform_using_destination_membership: false, fivetran_platform_using_user: false}' --target "$db"
+dbt test --vars '{fivetran_platform_schema: sqlw_tests}'  --target "$db"
+else


This is all necessary as the SQL Warehouse still uses the same hive_metastore and comes into conflict with the All Purpose Cluster when they both run in parallel with the same schema.

Therefore, we need to explicitly use a different schema for the two.

fivetran-joemarkiewicz · 2024-04-02T23:19:30Z

integration_tests/dbt_project.yml

@@ -24,7 +24,7 @@ vars:

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


This is an artifact of needing to use two different schemas for the Databricks jobs

fivetran-catfritz

I added the changes to the readme and changelog, and also had one small suggestion for the audit table config, but otherwise this is looking great!

I confirm I was able to reproduce the issue locally on Databricks SQL Warehouse, and it was resolved by this fix, with the audit model materialized as a table.

For good measure, I also ran this locally on SQL server since it was mentioned in your review notes, and all looks good there!

models/fivetran_platform__audit_table.sql

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

README.md

fivetran-joemarkiewicz · 2024-04-03T16:13:17Z

Thanks @fivetran-catfritz! I agree with your suggestion and committed it to the branch. I also really appreciate your README updates 🙏. I made one small wording change to be consistent with the terminology of the runtime name (Datarbricks All Purpose Cluster). Lastly, I regenerated the docs as the incremental file format change needed to be picked up in the docs.

Let me know if there are any other comments needed before approving.

fivetran-catfritz

Looks great--approved!

fivetran-jamie

left some mostly doc-related comments!

CHANGELOG.md

README.md

fivetran-jamie · 2024-04-03T19:33:04Z

macros/is_databricks_sql_warehouse.sql

@@ -0,0 +1,15 @@
+{% macro is_databricks_sql_warehouse(target) %}


we may want to add this to fivetran_utils in the future since Databricks SQL Warehouse folks using other packages with incremental models will have the same issue

I agree. This is something we will migrate there in the future.

CHANGELOG.md

Jamie review notes Co-authored-by: Jamie Rodriguez <[email protected]>

fivetran-reneeli

no other comments!

fivetran-joemarkiewicz added 2 commits April 2, 2024 10:30

feature/databricks-sql-warehouse-compatibility

137ed6c

run databricks sql

382dbe2

fivetran-joemarkiewicz self-assigned this Apr 2, 2024

fivetran-joemarkiewicz added 7 commits April 2, 2024 11:08

changelog pr ref and schema update

99d7f83

changes to incremental startegy selection

12ea509

databricks different schemas

c575861

sql warehouse specific test runs

b0ac2fc

adjustment for databricks sql and all other destinations

bd2175b

unique schema name to ensure drop wont conflict

d41ebf8

changelog reword

839c093

fivetran-joemarkiewicz commented Apr 2, 2024

View reviewed changes

fivetran-joemarkiewicz added 3 commits April 2, 2024 15:02

docs regen

4e87cfa

validations and docs regen

cf257e5

variable adjustment

37e5c52

fivetran-joemarkiewicz requested a review from fivetran-catfritz April 2, 2024 23:17

fivetran-joemarkiewicz commented Apr 2, 2024

View reviewed changes

fivetran-joemarkiewicz added 2 commits April 2, 2024 18:19

change cleanup

91ffac9

changelog update

aa43436

fivetran-joemarkiewicz marked this pull request as ready for review April 3, 2024 01:26

update readme & changelog

bd30997

fivetran-catfritz requested changes Apr 3, 2024

View reviewed changes

models/fivetran_platform__audit_table.sql Outdated Show resolved Hide resolved

Update models/fivetran_platform__audit_table.sql

2cc5a70

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

fivetran-joemarkiewicz commented Apr 3, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

fivetran-joemarkiewicz added 2 commits April 3, 2024 11:10

Update README.md

8aad434

docs regen after review

9df06fb

fivetran-joemarkiewicz requested a review from fivetran-catfritz April 3, 2024 16:13

fivetran-catfritz approved these changes Apr 3, 2024

View reviewed changes

spark removal from macro

064955d

fivetran-jamie reviewed Apr 3, 2024

View reviewed changes

Apply suggestions from code review

ce3abc9

Jamie review notes Co-authored-by: Jamie Rodriguez <[email protected]>

fivetran-jamie approved these changes Apr 3, 2024

View reviewed changes

changelog for fileformat addition

ba8ef14

fivetran-reneeli approved these changes Apr 3, 2024

View reviewed changes

fivetran-joemarkiewicz merged commit acb8b61 into main Apr 4, 2024
10 checks passed

fivetran-joemarkiewicz deleted the feature/databricks-sql-warehouse-compatibility branch April 4, 2024 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/databricks-sql-warehouse-compatibility #121

feature/databricks-sql-warehouse-compatibility #121

fivetran-joemarkiewicz commented Apr 2, 2024 •

edited

Loading

fivetran-joemarkiewicz Apr 2, 2024

fivetran-joemarkiewicz Apr 2, 2024

fivetran-joemarkiewicz Apr 2, 2024

fivetran-joemarkiewicz Apr 2, 2024

fivetran-catfritz left a comment

fivetran-joemarkiewicz commented Apr 3, 2024

fivetran-catfritz left a comment

fivetran-jamie left a comment

fivetran-jamie Apr 3, 2024

fivetran-joemarkiewicz Apr 3, 2024

fivetran-reneeli left a comment

		@@ -0,0 +1,15 @@
		{% macro is_databricks_sql_warehouse(target) %}

feature/databricks-sql-warehouse-compatibility #121

feature/databricks-sql-warehouse-compatibility #121

Conversation

fivetran-joemarkiewicz commented Apr 2, 2024 • edited Loading

PR Overview

Bug Fixes

Under the Hood

PR Checklist

Basic Validation

Detailed Validation

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

fivetran-joemarkiewicz Apr 2, 2024

Choose a reason for hiding this comment

fivetran-joemarkiewicz Apr 2, 2024

Choose a reason for hiding this comment

fivetran-joemarkiewicz Apr 2, 2024

Choose a reason for hiding this comment

fivetran-joemarkiewicz Apr 2, 2024

Choose a reason for hiding this comment

fivetran-catfritz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz commented Apr 3, 2024

fivetran-catfritz left a comment

Choose a reason for hiding this comment

fivetran-jamie left a comment

Choose a reason for hiding this comment

fivetran-jamie Apr 3, 2024

Choose a reason for hiding this comment

fivetran-joemarkiewicz Apr 3, 2024

Choose a reason for hiding this comment

fivetran-reneeli left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz commented Apr 2, 2024 •

edited

Loading