[Bug Fix] Remove incremental logic #97

fivetran-avinash · 2025-01-13T22:57:59Z

PR Overview

This PR will address the following Issue/Feature: [#95]

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

Source release is breaking, necessitating an upgrade.

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

Bug Fixes

Removed incremental logic in the following end models:
- shopify__discounts
- shopify__order_lines
- shopify__orders
- shopify__transactions
These models utilized the merge incremental strategy on BigQuery and Databricks, as we could not rely on a time series timestamp to impelment the insert_overwrite strategy. Using merge is a costly strategy, so it defeats the purpose of leveraging incremental logic.
There were also concerns about the incremental logic returning incorrect data in some end models. For example, if a repeat order within the new_vs_repeat CTE logic in shopify__orders was calculated within the specified incremental window but the new order was not in that same time period, it could be incorrectly processed as a new order.

Upstream Under-the-Hood Updates from `shopify_source` Package

(Affects Redshift only) Creates new shopify_union_data macro to accommodate Redshift's treatment of empty tables.
- For each staging model, if the source table is not found in any of your schemas, the package will create a empty table with 0 rows for non-Redshift warehouses and a table with 1 all-null row for Redshift destinations.
- This is necessary as Redshift will ignore explicit data casts when a table is completely empty and materialize every column as a varchar. This throws errors in downstream transformations in the shopify package. The 1 row will ensure that Redshift will respect the package's datatype casts.

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:

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

🇪🇺

fivetran-joemarkiewicz

@fivetran-avinash a few questions and change requests before approval.

fivetran-joemarkiewicz · 2025-01-21T15:47:15Z

models/shopify__discounts.sql

@@ -1,8 +1,7 @@
 {{
    config(
-        materialized='table' if target.type in ('bigquery', 'databricks', 'spark') else 'incremental',
+        materialized='table',


Do we need any of this configuration anymore now that the incremental strategy is being removed? Why do we need to define the cluster and unique_key if there is no incremental strategy leveraging them?

Same question on the other incremental updates in this PR.

Agreed. Removed.

fivetran-joemarkiewicz · 2025-01-21T15:55:26Z

CHANGELOG.md

+- These models utilized the `merge` incremental strategy on BigQuery and Databricks, as we could not rely on a time series timestamp to impelment the `insert_overwrite` strategy. Using `merge` is a costly strategy, so it defeats the purpose of leveraging incremental logic. 
+- There were also concerns about the incremental logic returning incorrect data in some end models. For example, if a repeat order within the `new_vs_repeat` CTE logic in `shopify__orders` was calculated within the specified incremental window but the new order was not in that same time period, it could be incorrectly processed as a new order.  


I worry this is bordering too much information that customers may not entirely understand this update. Here is a recommendation for making this a bit more succinct.

Suggested change

- These models utilized the `merge` incremental strategy on BigQuery and Databricks, as we could not rely on a time series timestamp to impelment the `insert_overwrite` strategy. Using `merge` is a costly strategy, so it defeats the purpose of leveraging incremental logic.

- There were also concerns about the incremental logic returning incorrect data in some end models. For example, if a repeat order within the `new_vs_repeat` CTE logic in `shopify__orders` was calculated within the specified incremental window but the new order was not in that same time period, it could be incorrectly processed as a new order.

- Incremental strategies were removed from these models due to potential inaccuracies with the `merge` strategy on BigQuery and Databricks. For instance, the `new_vs_repeat` field in `shopify__orders` could produce incorrect results during incremental runs. To ensure consistency, this logic was removed across all warehouses. If the previous incremental functionality was valuable to you, please consider opening a feature request to revisit this approach.

fivetran-joemarkiewicz · 2025-01-21T15:57:11Z

CHANGELOG.md

+
+## [Upstream Under-the-Hood Updates from `shopify_source` Package](https://github.com/fivetran/dbt_shopify_source/releases/tag/v0.15.0)
+- (Affects Redshift only) Creates new `shopify_union_data` macro to accommodate Redshift's treatment of empty tables.
+  - For each staging model, if the source table is not found in any of your schemas, the package will create a empty table with 0 rows for non-Redshift warehouses and a table with 1 all-`null` row for Redshift destinations.


This reads a bit strange. You mention above that this change only impacts Redshift users; however, the first part of this bullet mentions the behavior for every warehouse other than Redshift. Can we switch this around so the Redshift change is mentioned first and then we can say there should be no change in behavior for other warehouses.

Updated, as done in the source CHANGELOG.

fivetran-joemarkiewicz · 2025-01-21T15:57:23Z

packages.yml

+    revision: bugfix/redshift-limit-one
+    warn-unpinned: false


Reminder to swap before release

CHANGELOG.md

packages.yml

fivetran-avinash

@fivetran-joemarkiewicz Changes applied. This is ready for re-review!

fivetran-joemarkiewicz

Looks good, but one change needed in the README to bump the version range of the dbt_shopify package. Once that's applied this will be good for release review.

fivetran-joemarkiewicz · 2025-01-22T16:54:12Z

README.md

The package version for this package is still referencing the old v0.15.0 for dbt_shopify. This needs to be updated to reflect the latest version range.

fivetran-avinash

@fivetran-joemarkiewicz Oops. Updated!

fivetran-catfritz · 2025-01-22T18:35:47Z

CHANGELOG.md

+  - `shopify__order_lines`
+  - `shopify__orders`
+  - `shopify__transactions`
+- Incremental strategies were removed from these models due to potential inaccuracies with the `merge` strategy on BigQuery and Databricks. For instance, the `new_vs_repeat` field in `shopify__orders` could produce incorrect results during incremental runs. To ensure consistency, this logic was removed across all warehouses. If the previous incremental functionality was valuable to you, please consider opening a feature request to revisit this approach.


My understanding was that it was the incremental strategy in general and not limited to merge, especially since we had the models in question materializing as tables by default for BQ and Databricks, and delete+insert uses the same unique-key logic. Is this not the case? Unless I am mistaken, I would recommend generalizing it a bit:

Suggested change

- Incremental strategies were removed from these models due to potential inaccuracies with the `merge` strategy on BigQuery and Databricks. For instance, the `new_vs_repeat` field in `shopify__orders` could produce incorrect results during incremental runs. To ensure consistency, this logic was removed across all warehouses. If the previous incremental functionality was valuable to you, please consider opening a feature request to revisit this approach.

- Incremental strategies were removed from these models due to potential inaccuracies with the incremental strategy. For instance, the `new_vs_repeat` field in `shopify__orders` could produce incorrect results during incremental runs. If the previous incremental functionality was valuable to you, please consider opening a feature request to revisit this approach.

Ah, I wasn't certain, as the majority of the conversation centered around merge. I've tweaked this a little, let me know if this looks good!

fivetran-catfritz

lgtm!

* MagicBot/add-model-counts updates * Update README.md --------- Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>

Remove incremental logic

17a46d3

fivetran-avinash self-assigned this Jan 13, 2025

fivetran-avinash added 2 commits January 13, 2025 15:34

Remove incremental logic

7610760

Remove incremental logic

318fc27

fivetran-avinash marked this pull request as ready for review January 14, 2025 18:44

fivetran-avinash added 2 commits January 14, 2025 10:45

Remove incremental logic

56f243d

Remove incremental logic

a96514b

fivetran-joemarkiewicz self-requested a review January 21, 2025 15:42

fivetran-joemarkiewicz requested changes Jan 21, 2025

View reviewed changes

fivetran-joemarkiewicz reviewed Jan 21, 2025

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

fivetran-avinash commented Jan 22, 2025

View reviewed changes

packages.yml Outdated Show resolved Hide resolved

fivetran-avinash added 2 commits January 22, 2025 01:24

PR fixes

d8b48ad

schema

8904f43

fivetran-avinash commented Jan 22, 2025

View reviewed changes

fivetran-joemarkiewicz approved these changes Jan 22, 2025

View reviewed changes

README

0838104

fivetran-avinash commented Jan 22, 2025

View reviewed changes

fivetran-catfritz reviewed Jan 22, 2025

View reviewed changes

CHANGELOG

57101e2

fivetran-catfritz approved these changes Jan 22, 2025

View reviewed changes

fivetran-avinash and others added 2 commits January 22, 2025 13:58

Update packages.yml

a2b83e0

Feature: Add Quickstart Model Counts to README (#96)

3fa1b1c

* MagicBot/add-model-counts updates * Update README.md --------- Co-authored-by: Avinash Kunnath <108772760+fivetran-avinash@users.noreply.github.com>

fivetran-avinash merged commit d9df704 into main Jan 22, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug Fix] Remove incremental logic #97

[Bug Fix] Remove incremental logic #97

fivetran-avinash commented Jan 13, 2025 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jan 21, 2025

fivetran-joemarkiewicz Jan 21, 2025

fivetran-avinash Jan 22, 2025

fivetran-joemarkiewicz Jan 21, 2025

fivetran-avinash Jan 22, 2025

fivetran-joemarkiewicz Jan 21, 2025

fivetran-avinash Jan 22, 2025

fivetran-joemarkiewicz Jan 21, 2025

fivetran-avinash left a comment

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jan 22, 2025

fivetran-avinash left a comment

fivetran-catfritz Jan 22, 2025

fivetran-avinash Jan 22, 2025

fivetran-catfritz left a comment

		- These models utilized the `merge` incremental strategy on BigQuery and Databricks, as we could not rely on a time series timestamp to impelment the `insert_overwrite` strategy. Using `merge` is a costly strategy, so it defeats the purpose of leveraging incremental logic.
		- There were also concerns about the incremental logic returning incorrect data in some end models. For example, if a repeat order within the `new_vs_repeat` CTE logic in `shopify__orders` was calculated within the specified incremental window but the new order was not in that same time period, it could be incorrectly processed as a new order.

	- These models utilized the `merge` incremental strategy on BigQuery and Databricks, as we could not rely on a time series timestamp to impelment the `insert_overwrite` strategy. Using `merge` is a costly strategy, so it defeats the purpose of leveraging incremental logic.
	- There were also concerns about the incremental logic returning incorrect data in some end models. For example, if a repeat order within the `new_vs_repeat` CTE logic in `shopify__orders` was calculated within the specified incremental window but the new order was not in that same time period, it could be incorrectly processed as a new order.
	- Incremental strategies were removed from these models due to potential inaccuracies with the `merge` strategy on BigQuery and Databricks. For instance, the `new_vs_repeat` field in `shopify__orders` could produce incorrect results during incremental runs. To ensure consistency, this logic was removed across all warehouses. If the previous incremental functionality was valuable to you, please consider opening a feature request to revisit this approach.

[Bug Fix] Remove incremental logic #97

[Bug Fix] Remove incremental logic #97

Conversation

fivetran-avinash commented Jan 13, 2025 • edited Loading

PR Overview

Bug Fixes

PR Checklist

Basic Validation

Detailed Validation

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

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

fivetran-catfritz left a comment

Choose a reason for hiding this comment

fivetran-avinash commented Jan 13, 2025 •

edited

Loading