Feature/performance enhancement #41

fivetran-catfritz · 2024-01-25T20:18:50Z

PR Overview

This PR will address the following Issue/Feature:

This PR will result in the following new package version:

v0.9.0
Made this breaking since we are deleting the stg_mixpanel__event_tmp table and changing stg_mixpanel__event to an incremental model.

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

🚨 Breaking Changes 🚨

Note: This update was made breaking since it will alter the materialization of existing models. While these changes do not necessitate a --full-refresh, it may be beneficial if you run into issues with this update.

Updated models with the following performance improvements:
- Update the incremental strategy for all models to insert_overwrite for BigQuery and Databricks and delete+insert for all other warehouses.
- Consolidated stg_mixpanel__event and stg_mixpanel__event_tmp into one incremental model. While this will increase storage, we opted to make this change to improve compute.

Feature Updates

Added cluster_by columns to the configs for incremental models. This will benefit Snowflake and BigQuery users.

PR Checklist

Basic Validation

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

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

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:

Ran full refreshes and and incremental runs in all the major warehouses.
See internal ticket for detailed validation.

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

⏲️

Update README.md

fivetran-joemarkiewicz

@fivetran-catfritz thanks for working through this and being diligent in ensuring the optimizations being applied to the incremental strategies are the gold standard!

As we just synced on this review, I found there were some inconsistencies in the aggregate end models (daily_events to be specific) that didn't seem to tie out. The staging model end end mixpanel__event model looked good, but we need to do a deeper dive into the other incremental models to ensure the incremental logic is working as we would expect.

Would you be able to apply the updates suggested in the comments below as well as update the incremental logic where needed to ensure the addition of new records in subsequent runs for the aggregate models work as we would expect. Thanks!

README.md

CHANGELOG.md

fivetran-catfritz

@fivetran-joemarkiewicz I have updated the docs as suggested. I have also updated the strategies a bit, so let me know if you want to chat about it!

README.md

jasongroob

thanks for making these updates! left a few comments / suggestions for you.

jasongroob · 2024-02-13T15:43:42Z

CHANGELOG.md

+>Note: This update was made breaking since it will alter the materialization of existing models. While these changes do not necessitate a `--full-refresh`, it may be beneficial if you run into issues with this update.
+- Updated models with the following performance improvements:
+  - Update the incremental strategy for all models to `insert_overwrite` for BigQuery and Databricks and `delete+insert` for all other warehouses.
+  - Removed `stg_mixpanel__event_tmp` in favor of `stg_mixpanel__event_tmp`, which is now an incremental model. While this will increase storage, this change was made to improve compute.


looks like the second table name is wrong. did you mean:

Removed stg_mixpanel__event_tmp in favor of stg_mixpanel__event, which is now ...

Oops, yes. 😄

jasongroob · 2024-02-13T15:45:01Z

CHANGELOG.md

+
+## Feature Updates
+- Added `cluster_by` columns to the configs for incremental models. This will benefit Snowflake and BigQuery users. 
+- Added column `dbt_run_date` to incremental models to improve accuracy and optimize downstream models. This date captures the date a record was added or updated by this package.


thank you! it's always helpful to have these timestamp columns.

jasongroob · 2024-02-13T15:54:59Z

README.md


-We recognize there are some limitations with these strategies, particularly around updated records in the past which cause duplicates, and are assessing using a different strategy in the future.
+`insert_overwrite` is our preferred incremental strategy because it will be able to properly handle updates to records that exist outside the immediate incremental window. That is, because it leverages partitions, `insert_overwrite` will appropriately update existing rows that have been changed upstream instead of inserting duplicates of them--all without requiring a full table scan.


can you define immediate incremental window? if the queries only pull in data from the last X days, do you mean changes that occurred to records >X days ago?

Thanks for the doc comments. I am going to rework this tomorrow!

jasongroob · 2024-02-13T15:57:45Z

README.md


-> For either of these strategies, we highly recommend that users periodically run a `--full-refresh` to ensure a high level of data quality.
+`delete+insert` is our second-choice as it resembles `insert_overwrite` but lacks partitions. This strategy works most of the time and appropriately handles incremental loads that do not contain changes to past records. However, if a past record has been updated and is outside of the incremental window, `delete+insert` will insert a duplicate record. 😱


it might be worth qualifying preferred and second-choice as it depends on the data platform. As an example, Snowflake doesn't support insert_overwrite. I don't think you mean to imply that Snowflake is using a lesser methodology.

https://docs.getdbt.com/docs/build/incremental-models#about-incremental_strategy

jasongroob · 2024-02-13T17:29:38Z

models/mixpanel__daily_events.sql

-    -- only return the most recent day of data
-    where date_day >= coalesce( (select max(date_day)  from {{ this }} ), '2010-01-01')
-
+    where date_day >= {{ mixpanel.lookback(from_date="max(dbt_run_date)") }}


won't this potentially exclude some late arriving data that occurred prior to the max(dbt_run_date)?

Yes, thank you for catching. I ended up scrapping using dbt_run_date in the incremental strategy and updated to use the 7-day lookback to be where date_day >= {{ mixpanel.mixpanel_lookback(from_date="max(date_day)", interval=var('lookback_window', 7), datepart='day') }}

awesome! nice improvement.

jasongroob · 2024-02-13T17:31:57Z

macros/lookback.sql

@@ -0,0 +1,9 @@
+{% macro lookback(from_date, datepart='day', interval=7, default_start_date='2010-01-01') %}


having '2010-01-01' be a project variable could help if someone wants to have their own custom start date. 14 years is a LOT of data to include by default.

I realize calling this value default_start_date is a bit misleading. The 2010-01-01 is only meant as a super cautious failsafe in case the of null values in an incremental run, which I imagine would rarely occur. I'll rename this. The actual start date of events being brought in can be set by variable date_range_start. Thanks for the callout!

jasongroob · 2024-02-13T19:51:14Z

models/mixpanel__event.sql

-    -- events are only eligible for de-duping if they occurred on the same calendar day 
-    occurred_at >= coalesce((select cast( max(date_day) as {{ dbt.type_timestamp() }} ) from {{ this }} ), '2010-01-01')
+    {% if is_incremental() %}
+    dbt_run_date >= {{ mixpanel.lookback(from_date="max(dbt_run_date)", interval=1) }}


maybe move the default interval period to a project variable? i assume one day will be enough but maybe there's scenarios where more is needed.

Thanks @jasongroob that is a great idea. I have have created a variable lookback_windows with a default of value 7 days. In your experience with mixpanel, would that be a more appropriate length?

yeah, i think that should be fine. i don't have a good sense of how late new data can arrive.

jasongroob · 2024-02-13T20:04:45Z

models/staging/stg_mixpanel__event.sql

do you know the impacts on compute / storage of moving this table from ephemeral to incremental? if the mixpanel.event table is large (which it probably is), you are effectively writing the data three times. Once at the source, once in this STG model, and once in mixpanel__event.

Thanks, @jasongroob this is a good point. Another user mentioned wanting to avoid full table scans, but at the end of the day it doesn't make much sense to have a large table 3 times. I have tried to balance the storage vs compute a little more by putting the stg model back to ephemeral since the tmp view has been removed.

makes sense. i think the current implementation of having the staging be ephemeral is a nice balance between compute and storage concerns.

updates

fivetran-joemarkiewicz

@fivetran-catfritz great job on this PR! I know there has been a lot of back and forth here. This is nearly across the line, I just have a few final requests to update before approving. Please don't hesitate to reach out if you have any questions regarding my comments. Thanks!

CHANGELOG.md

fivetran-joemarkiewicz · 2024-02-21T15:15:01Z

CHANGELOG.md

+    - `mixpanel__monthly_events`
+    - `mixpanel__sessions`
+  - Removed `stg_mixpanel__event_tmp` in favor of ephemeral model `stg_mixpanel__event`. This is to reduce redundancy of models created and reduce the number of full scans.
+  - Updated the materialization of `stg_mixpanel__user_first_event` to a view. 


I really like the reasoning you gave to the previous bullet. Can we also add a blurb about why converting the stg_mixpanel__user_first_event to a view is more performant.

CHANGELOG.md

fivetran-joemarkiewicz · 2024-02-21T15:17:39Z

CHANGELOG.md

+
+## Feature Updates
+- Added a default 7-day look-back to incremental models to accommodate late arriving events. The numbers of days can be changed by setting the var `lookback_window` in your dbt_project.yml. See the [Lookback Window section of the README](https://github.com/fivetran/dbt_mixpanel/blob/main/README.md#lookback-window) for more details. 
+  - Note: this replaces the variable `sessionization_trailing_window`, which was previously used in the `mixpanel__sessions` model. This variable was replaced due to the change in the incremental and lookback strategy. 


The removal of this variable should be treated as a breaking change in case users are leveraging this in their current workflow. Would you be able to move this to the breaking change section.

models/mixpanel__monthly_events.sql

models/staging/stg_mixpanel__user_event_date_spine.sql

models/mixpanel__sessions.sql

models/mixpanel__daily_events.sql

CHANGELOG.md

Co-authored-by: Joe Markiewicz <[email protected]>

models/mixpanel__daily_events.sql

Co-authored-by: Joe Markiewicz <[email protected]>

fivetran-catfritz · 2024-02-21T18:44:10Z

@fivetran-joemarkiewicz Thanks for all the review comments! I have updated, and this is ready for re-review.

fivetran-joemarkiewicz

@fivetran-catfritz Great job!! This is looks good to go to me!

fivetran-dejantucakov and others added 6 commits December 5, 2023 13:18

Update README.md

29c24b7

feature/performance-enhancement

3497935

update to incremental

a9119ce

update to incremental

0de2a07

feature/performance-enhancement

1129752

feature/performance-enhancement

198081e

fivetran-catfritz self-assigned this Jan 25, 2024

fivetran-catfritz added 4 commits January 25, 2024 14:52

feature/performance-enhancement

46fd5c4

feature/performance-enhancement

18ac897

update clustering

c6cd6a8

update clustering

0fd82bf

fivetran-joemarkiewicz mentioned this pull request Jan 26, 2024

Update README.md #40

Merged

fivetran-catfritz added 8 commits January 26, 2024 09:58

Merge pull request #40 from fivetran/fivetran-dejantucakov-patch-1

334010b

Update README.md

update changelog & readme

f049d87

update ymls

bcc310d

update readme

19901af

updates

d1ae3f1

update changelog, ymls, regen docs

ba53a3e

update changelog

00e2ccc

update changelog

5f374f7

fivetran-catfritz requested a review from fivetran-joemarkiewicz January 30, 2024 14:57

fivetran-joemarkiewicz requested changes Feb 1, 2024

View reviewed changes

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-catfritz added 5 commits February 2, 2024 17:48

update lookbacks

b17a9bd

update lookbacks

702694b

update lookbacks

dcb14e7

update readme

6345dc1

update

2cb15a3

fivetran-catfritz commented Feb 6, 2024

View reviewed changes

README.md Show resolved Hide resolved

update

823e2d0

fivetran-catfritz requested a review from fivetran-joemarkiewicz February 6, 2024 04:50

jasongroob reviewed Feb 13, 2024

View reviewed changes

fivetran-catfritz added 9 commits February 20, 2024 22:12

updates

1b66f95

delete extra macro

4b760c0

updates

6f4c906

updates

59e2434

Merge pull request #43 from fivetran/feature/test-materializations

2fac3fd

updates

Merge branch 'main' into feature/performance-enhancement

6574992

update var names

5e91f92

update macro

7b54bdb

remove extra comma

9bfcffa

fivetran-joemarkiewicz requested changes Feb 21, 2024

View reviewed changes

fivetran-catfritz commented Feb 21, 2024

View reviewed changes

models/mixpanel__daily_events.sql Outdated Show resolved Hide resolved

fivetran-catfritz commented Feb 21, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-catfritz and others added 2 commits February 21, 2024 11:16

Apply suggestions from code review

ee45a9a

Co-authored-by: Joe Markiewicz <[email protected]>

Update models/staging/stg_mixpanel__user_event_date_spine.sql

ea07fae

Co-authored-by: Joe Markiewicz <[email protected]>

fivetran-catfritz commented Feb 21, 2024

View reviewed changes

models/mixpanel__daily_events.sql Outdated Show resolved Hide resolved

fivetran-catfritz and others added 4 commits February 21, 2024 11:29

Apply suggestions from code review

befd1be

Co-authored-by: Joe Markiewicz <[email protected]>

update models, readme, changelog

c8fe97d

update changelog and regen docs

6e4fbc5

update yml

ed92bba

fivetran-catfritz requested a review from fivetran-joemarkiewicz February 21, 2024 18:41

update changelog

f9ae48d

fivetran-joemarkiewicz approved these changes Feb 21, 2024

View reviewed changes

add autoreleaser

d818a3a

fivetran-avinash self-requested a review February 21, 2024 20:15

update changelog

4f245f0

fivetran-avinash approved these changes Feb 21, 2024

View reviewed changes

fivetran-catfritz merged commit 19e968c into main Feb 21, 2024
8 checks passed

fivetran-joemarkiewicz mentioned this pull request Feb 22, 2024

Improvement: Ensure time filter is applied in temp table #39

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/performance enhancement #41

Feature/performance enhancement #41

fivetran-catfritz commented Jan 25, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-catfritz left a comment

jasongroob left a comment

jasongroob Feb 13, 2024

fivetran-catfritz Feb 21, 2024

jasongroob Feb 13, 2024

jasongroob Feb 13, 2024

fivetran-catfritz Feb 21, 2024

jasongroob Feb 13, 2024

jasongroob Feb 13, 2024

fivetran-catfritz Feb 21, 2024 •

edited

Loading

jasongroob Feb 21, 2024

jasongroob Feb 13, 2024

fivetran-catfritz Feb 21, 2024

jasongroob Feb 13, 2024

fivetran-catfritz Feb 21, 2024

jasongroob Feb 21, 2024

jasongroob Feb 13, 2024

fivetran-catfritz Feb 21, 2024

jasongroob Feb 21, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Feb 21, 2024

fivetran-joemarkiewicz Feb 21, 2024

fivetran-catfritz commented Feb 21, 2024

fivetran-joemarkiewicz left a comment


		We recognize there are some limitations with these strategies, particularly around updated records in the past which cause duplicates, and are assessing using a different strategy in the future.
		`insert_overwrite` is our preferred incremental strategy because it will be able to properly handle updates to records that exist outside the immediate incremental window. That is, because it leverages partitions, `insert_overwrite` will appropriately update existing rows that have been changed upstream instead of inserting duplicates of them--all without requiring a full table scan.


		> For either of these strategies, we highly recommend that users periodically run a `--full-refresh` to ensure a high level of data quality.
		`delete+insert` is our second-choice as it resembles `insert_overwrite` but lacks partitions. This strategy works most of the time and appropriately handles incremental loads that do not contain changes to past records. However, if a past record has been updated and is outside of the incremental window, `delete+insert` will insert a duplicate record. 😱

		@@ -0,0 +1,9 @@
		{% macro lookback(from_date, datepart='day', interval=7, default_start_date='2010-01-01') %}

Feature/performance enhancement #41

Feature/performance enhancement #41

Conversation

fivetran-catfritz commented Jan 25, 2024 • edited Loading

PR Overview

🚨 Breaking Changes 🚨

Feature Updates

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

fivetran-catfritz left a comment

Choose a reason for hiding this comment

jasongroob 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

fivetran-catfritz Feb 21, 2024 • edited Loading

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 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 commented Feb 21, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-catfritz commented Jan 25, 2024 •

edited

Loading

fivetran-catfritz Feb 21, 2024 •

edited

Loading