add user passthrough column to users model and update dependencies #44

fivetran-reneeli · 2024-07-17T20:31:49Z

PR Overview

This PR will address the following Issue/Feature:
#43
and internal ticket

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

Non breaking as the default config for event_extension is still true so existing users won't see any changes. And the passthrough columns only add columns, instead of deleting or modifying existing ones.

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

Bug Fixes

Introduces variable iterable__using_event_extension to disable the event_extension table and exclude its fields from persisting downstream. This allows the downstream models to run even if the source event_extension table does not exist. For more information on how to configure the iterable__using_event_extension variable, refer to the README.
Persists user_history passthrough columns as stipulated via the iterable_user_history_pass_through_columns variable through to the iterable__users model. For more information on how to configure the iterable_user_history_pass_through_columns variable, refer to the README.

Under the Hood

Addition of integrity and consistency validation tests within integration tests pertaining to the iterable__user_unsubscriptions model.
Updated seed data to ensure proper testing of the latest v0.8.1 dbt_iterable_source release.
Updated pull request and issue templates.
Included auto-releaser GitHub Actions workflow to automate future releases.

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:

Variables tried:

    iterable_user_history_pass_through_columns:
      - name: phone_number_line_type
        alias: phone_digits
      - name: additional_properties
    iterable_event_extension_pass_through_columns:
      - name: browser_token
    iterable__using_event_extension: true
#    iterable__using_event_extension: false

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

💃

fivetran-reneeli · 2024-07-18T19:58:36Z

will regen docs upon approval

.buildkite/scripts/run_models.sh

fivetran-avinash

@fivetran-reneeli Awesome job! This is a pretty extensive update with a lot of custom logic you had to implement, so great work in getting it to this point.

Some comments to address before approval. Let me know if you have any questions.

Also, you will need to regenerate docs.

fivetran-avinash · 2024-07-22T18:18:19Z

CHANGELOG.md

@@ -1,4 +1,9 @@
-# dbt_iterable v0.RELEASE.RELEASE
+# dbt_iterable v0.11.1


There are changes to when experiment_id is or is not being brought in this model, plus the new aggregate metrics, which I believe classifies as a schema change. Should we consider making this a breaking change and bumping the version up?

good point-- bumped to breaking change

fivetran-avinash · 2024-07-22T18:18:56Z

CHANGELOG.md

+
+## Bug Fixes
+- Introduces variable `iterable__using_event_extension` to disable the `event_extension` table and exclude its fields from persisting downstream. This allows the downstream models to run even if the source `event_extension` table does not exist. For more information on how to configure the `iterable__using_event_extension` variable, refer to the [README](https://github.com/fivetran/dbt_iterable/blob/main/README.md#step-4-enablingdisabling-models).
+- Persists `user_history` passthrough columns, as stipulated via the `iterable_user_history_pass_through_columns` variable, through to the `iterable__users` model. For more information on how to configure the `iterable_user_history_pass_through_columns` variable, refer to the [README](https://github.com/fivetran/dbt_iterable/blob/main/README.md#passing-through-additional-fields).

 ## Under the Hood


You can add more details around the consistency tests you've created here too.

README.md

fivetran-avinash · 2024-07-22T18:20:54Z

integration_tests/dbt_project.yml

  ## For validation testing. To be commented out before release.
-models:
-  +schema: "iterable_{{ var('directed_schema','dev') }}"
+# models:


I think you can uncomment this, Buildkite is equipped to handle it.

fivetran-avinash · 2024-07-22T18:49:37Z

models/intermediate/int_iterable__user_event_metrics.sql

-                                        'experiment_id'] %}
-        , sum( {{ col.name }} ) as {{ col.name }}
+            {% if col.name|lower not in ['unique_user_key', 'user_id', '_fivetran_user_id', 'user_email', 'user_full_name', 'campaign_id', 'campaign_name', 'recurring_campaign_id', 'recurring_campaign_name', 'first_event_at', 'last_event_at', 'template_id', 'template_name'] %}
+                {% if col.name|lower == 'experiment_id' %}


Similar note to int_iterable__campaign_events.

Added experiment_id back to the exclusion

fivetran-avinash · 2024-07-22T18:49:42Z

models/intermediate/int_iterable__user_event_metrics.sql

+                {% if col.name|lower == 'experiment_id' %}
+                    {% if col.name in user_campaign_columns %}
+                        {% if var('iterable__using_event_extension', True) %}
+                            , sum({{ col.name }}) as {{ col.name }}


Similar note to int_iterable__campaign_events.

models/intermediate/int_iterable__campaign_event_metrics.sql

fivetran-avinash · 2024-07-22T18:57:12Z

integration_tests/tests/consistency/consistency_users.sql

+dev as (
+    select *
+    from {{ target.schema }}_iterable_dev.iterable__users
+    where date(created_at) < date({{ dbt.current_timestamp() }})


Suggested change

where date(created_at) < date({{ dbt.current_timestamp() }})

where date(updated_at) < date({{ dbt.current_timestamp() }})

fivetran-avinash · 2024-07-22T18:57:34Z

models/intermediate/int_iterable__campaign_event_metrics.sql

-
-{%- set user_campaign_columns = adapter.get_columns_in_relation(ref('iterable__user_campaign')) %}
-
+    {%- set user_campaign_columns = adapter.get_columns_in_relation(ref('iterable__user_campaign')) %}


Suggested change

{%- set user_campaign_columns = adapter.get_columns_in_relation(ref('iterable__user_campaign')) %}

{%- set user_campaign_columns = adapter.get_columns_in_relation(ref('iterable__user_campaign')) %}

Co-authored-by: Avinash Kunnath <[email protected]>

fivetran-avinash

@fivetran-reneeli thanks for applying all these changes! I verified that the validation tests worked as expected.

A few comments left to address (plus another reminder to regenerate the docs) and then you should be good for approval.

.buildkite/scripts/run_models.sh

README.md

.buildkite/scripts/run_models.sh

fivetran-avinash · 2024-07-23T21:04:35Z

README.md

Reminder to bump the README dbt_iterable version in lines 76-80, now that it's a breaking change.

ah thank you, updated

Co-authored-by: Avinash Kunnath <[email protected]>

fivetran-reneeli · 2024-07-24T14:15:04Z

.buildkite/scripts/run_models.sh

@@ -21,7 +21,8 @@ dbt compile --target "$db"
 dbt run --target "$db" --full-refresh
 dbt run --target "$db"
 dbt test --target "$db"
-dbt run --vars '{iterable__using_campaign_label_history: false, iterable__using_user_unsubscribed_message_type_history: false, iterable__using_campaign_suppression_list_history: false}' --target "$db" --full-refresh
+dbt run --vars '{iterable_user_history_pass_through_columns: [{name: "phone_number_updated_at"}], iterable_event_extension_pass_through_columns: [{name: "web_push_message"}]}' --target "$db" --full-refresh
+dbt run --vars '{iterable__using_campaign_label_history: false, iterable__using_user_unsubscribed_message_type_history: false, iterable__using_campaign_suppression_list_history: false, iterable__using_event_extension: false}' --target "$db" 


@fivetran-avinash this is now failing I think due to needing a full refresh after the variables change. Gonna update the script

@fivetran-reneeli Apologies. See PR approval comment.

fivetran-avinash

Hi @fivetran-reneeli ! Thanks for addressing all the additional comments! This looks good to go.

Regarding your script issue and your commit history, it looks like I sent a final incorrect comment. My initial recommendation was to add the additional incremental run script after the full refresh. But my last suggestion in Github did indeed remove it. Sorry, that line should remain unchanegd.

Can you test this configuration in Buildkite that runs a full-refresh, then an incremental run, and see if this works:

dbt run --vars '{iterable__using_campaign_label_history: false, iterable__using_user_unsubscribed_message_type_history: false, iterable__using_campaign_suppression_list_history: false, iterable__using_event_extension: false}' --target "$db" —full-refresh

dbt run --vars '{iterable__using_campaign_label_history: false, iterable__using_user_unsubscribed_message_type_history: false, iterable__using_campaign_suppression_list_history: false, iterable__using_event_extension: false}' --target "$db"

fivetran-reneeli · 2024-07-24T16:28:28Z

Hi @fivetran-reneeli ! Thanks for addressing all the additional comments! This looks good to go.

Regarding your script issue and your commit history, it looks like I sent a final incorrect comment. My initial recommendation was to add the additional incremental run script after the full refresh. But my last suggestion in Github did indeed remove it. Sorry, that line should remain unchanegd.

Can you test this configuration in Buildkite that runs a full-refresh, then an incremental run, and see if this works:
dbt run --vars '{iterable__using_campaign_label_history: false, iterable__using_user_unsubscribed_message_type_history: false, iterable__using_campaign_suppression_list_history: false, iterable__using_event_extension: false}' --target "$db" —full-refresh

dbt run --vars '{iterable__using_campaign_label_history: false, iterable__using_user_unsubscribed_message_type_history: false, iterable__using_campaign_suppression_list_history: false, iterable__using_event_extension: false}' --target "$db"

Thanks @fivetran-avinash yeah I had figured this was what you meant and I tried that in this commit, while it failed I think it's because I didn't add the vars the second time around. Just pushed this and should be good. Appreciate your review!!

fivetran-joemarkiewicz

@fivetran-reneeli thanks for working through these changes. Please see my comments below to address before this will be ready for release approval.

Additionally, I noticed that dbt test fails if you have set iterable__using_event_extension to false. This is because in the yml where we define the tests we are still including experiment_id and not catching if the variable is turned on/off.

I recommend it would probably be best to create a surrogate key in the model that defines the uniqueness of the end model (like here in our netsuite model) and make that surrogate key dynamic so we can then test that field in the yml (like we have done here for the same netsuite model).

Lastly, I know this is out of scope for this task, but would you be able to create an issue for us to explore the iterable__events incremental logic. I did an integrity test on our internal data (albeit our internal data may be incomplete and not emulate a customer environment) and found that the row count for the iterable__events model does not match the row count of the stg_iterable__event model following an incremental run (this is present in both dev and prod). It would be great for us to proactively create a ticket for us to explore this in a future update.

packages.yml

CHANGELOG.md

…ily move back packages deps to test

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

fivetran-reneeli · 2024-07-25T15:33:21Z

@fivetran-reneeli thanks for working through these changes. Please see my comments below to address before this will be ready for release approval.

Additionally, I noticed that dbt test fails if you have set iterable__using_event_extension to false. This is because in the yml where we define the tests we are still including experiment_id and not catching if the variable is turned on/off.

I recommend it would probably be best to create a surrogate key in the model that defines the uniqueness of the end model (like here in our netsuite model) and make that surrogate key dynamic so we can then test that field in the yml (like we have done here for the same netsuite model).

Lastly, I know this is out of scope for this task, but would you be able to create an issue for us to explore the iterable__events incremental logic. I did an integrity test on our internal data (albeit our internal data may be incomplete and not emulate a customer environment) and found that the row count for the iterable__events model does not match the row count of the stg_iterable__event model following an incremental run (this is present in both dev and prod). It would be great for us to proactively create a ticket for us to explore this in a future update.

Thanks @fivetran-joemarkiewicz for catching-- I removed experiment_id from anywhere it was included in a unique test and instead created a surrogate key in those models (iterable__user_campaign and iterable__campaigns).

Hmm strange about the row counts-- will make issue for that

fivetran-joemarkiewicz

@fivetran-reneeli thanks for working through this PR. I approved, but have a request to add another CHANGELOG entry for the new surrogate key fields and their purpose for the dynamic uniqueness tests. No need to block release approval, but please make sure that comment is addressed and the packages.yml is updated before being merged and released. Thanks!

fivetran-joemarkiewicz · 2024-07-25T19:16:14Z

packages.yml

+  # - package: fivetran/iterable_source
+  #   version: [">=0.8.0", "<0.9.0"]
+  # - local: ../dbt_iterable_source
+  - git: https://github.com/fivetran/dbt_iterable_source.git


Reminder to update the source range and switch before merge and release

fivetran-joemarkiewicz · 2024-07-25T19:20:37Z

CHANGELOG.md

+[PR #44](https://github.com/fivetran/dbt_iterable/pull/44) includes the following updates:
+
+## 🚨 Breaking Changes 🚨
+- Introduces variable `iterable__using_event_extension` to allow the `event_extension` table to be disabled and exclude its field, `experiment_id`, from persisting downstream. This permits the downstream models to run even if the source `event_extension` table does not exist. By default the variable is set to True. If you don't have this table, you will need to set `iterable__using_event_extension` to False. For more information on how to configure the `iterable__using_event_extension` variable, refer to the [README](https://github.com/fivetran/dbt_iterable/blob/main/README.md#step-4-enablingdisabling-models). 


You'll also want to add in here that the uniqueness test has been updated to account for instances wheniterable__using_event_extension is enabled/disabled. It would be good to add what the new surrogate fields are and which models they can be found in.

fivetran-reneeli added 7 commits July 17, 2024 16:30

add user passthrough column to users model

3754e57

include event_extension var

dfe785d

change temp packages deps

0f093c6

add the var to run script

0fb90e7

temporarily update deps

0876ec0

versioning

4883d86

changelog

ae866fd

fivetran-reneeli mentioned this pull request Jul 18, 2024

add event_extension var fivetran/dbt_iterable_source#31

Merged

7 tasks

fivetran-reneeli self-assigned this Jul 18, 2024

updates to docs

7f45498

fivetran-reneeli requested a review from fivetran-avinash July 18, 2024 19:58

fivetran-reneeli added 3 commits July 19, 2024 15:58

add validation testing

260cc6a

fix

e7a53a4

schema

79b3551

fivetran-avinash reviewed Jul 22, 2024

View reviewed changes

.buildkite/scripts/run_models.sh Show resolved Hide resolved

fivetran-avinash requested changes Jul 22, 2024

View reviewed changes

fivetran-reneeli and others added 9 commits July 22, 2024 15:20

Update README.md

aafbb89

Co-authored-by: Avinash Kunnath <[email protected]>

up the version, update changelog, move testing schema

0c63d7c

Update models/iterable__events.sql

fda5788

Co-authored-by: Avinash Kunnath <[email protected]>

Update models/intermediate/int_iterable__campaign_event_metrics.sql

e5cd253

Co-authored-by: Avinash Kunnath <[email protected]>

update using_event_extension logic

e2c2724

updates

9107b7f

fix validation tests

9124cb3

update docs and fix indent

e68d5c7

fix indentation

43e2e19

fivetran-reneeli requested a review from fivetran-avinash July 23, 2024 20:09

fivetran-avinash reviewed Jul 23, 2024

View reviewed changes

fivetran-reneeli and others added 2 commits July 23, 2024 18:02

change dependency versions

a7360a3

Update .buildkite/scripts/run_models.sh

a48cb0e

Co-authored-by: Avinash Kunnath <[email protected]>

fivetran-reneeli added 2 commits July 23, 2024 18:14

docs

627b705

try

ce3b634

fivetran-reneeli commented Jul 24, 2024

View reviewed changes

try 2

16268d7

fivetran-avinash approved these changes Jul 24, 2024

View reviewed changes

try 3

d30829b

fivetran-reneeli added 3 commits July 24, 2024 12:46

typo

50d8ee4

wrong dash?

70615c9

switch package deps

069aef1

fivetran-joemarkiewicz self-requested a review July 24, 2024 18:30

fivetran-joemarkiewicz requested changes Jul 24, 2024

View reviewed changes

packages.yml Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-reneeli and others added 3 commits July 24, 2024 18:31

remove experiment id from unique tests and make dynamic, and temporar…

dea51ce

…ily move back packages deps to test

Update CHANGELOG.md

1119d79

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

new schema

aaba9f8

fivetran-reneeli added 2 commits July 25, 2024 13:10

update to prefix unique

0a9b49e

docs

ebc02f7

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

[Bug] Include User History Passthrough Columns in iterable__users model #43

Closed

4 tasks

fivetran-reneeli mentioned this pull request Jul 25, 2024

[Bug] Review the incremental logic in iterable__events #45

Closed

4 tasks

fivetran-joemarkiewicz approved these changes Jul 25, 2024

View reviewed changes

change deps and update changelog

4d156b9

fivetran-reneeli merged commit 0c3b449 into main Jul 25, 2024
8 checks passed

fivetran-reneeli deleted the bugfix/table_dependencies_and_passthrough_columns branch July 25, 2024 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add user passthrough column to users model and update dependencies #44

add user passthrough column to users model and update dependencies #44

fivetran-reneeli commented Jul 17, 2024 •

edited

Loading

fivetran-reneeli commented Jul 18, 2024

fivetran-avinash left a comment •

edited

Loading

fivetran-avinash Jul 22, 2024

fivetran-reneeli Jul 23, 2024

fivetran-avinash Jul 22, 2024

fivetran-reneeli Jul 23, 2024

fivetran-avinash Jul 22, 2024

fivetran-reneeli Jul 22, 2024

fivetran-avinash Jul 22, 2024

fivetran-reneeli Jul 23, 2024

fivetran-avinash Jul 22, 2024

fivetran-reneeli Jul 22, 2024

fivetran-avinash Jul 22, 2024

fivetran-avinash Jul 22, 2024

fivetran-avinash left a comment

fivetran-avinash Jul 23, 2024

fivetran-reneeli Jul 23, 2024

fivetran-reneeli Jul 24, 2024

fivetran-avinash Jul 24, 2024 •

edited

Loading

fivetran-avinash left a comment

fivetran-reneeli commented Jul 24, 2024

fivetran-joemarkiewicz left a comment

fivetran-reneeli commented Jul 25, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 25, 2024 •

edited

Loading

fivetran-joemarkiewicz Jul 25, 2024

fivetran-reneeli Jul 25, 2024

		@@ -1,4 +1,9 @@
		# dbt_iterable v0.RELEASE.RELEASE
		# dbt_iterable v0.11.1

	where date(created_at) < date({{ dbt.current_timestamp() }})
	where date(updated_at) < date({{ dbt.current_timestamp() }})


		{%- set user_campaign_columns = adapter.get_columns_in_relation(ref('iterable__user_campaign')) %}

		{%- set user_campaign_columns = adapter.get_columns_in_relation(ref('iterable__user_campaign')) %}

add user passthrough column to users model and update dependencies #44

add user passthrough column to users model and update dependencies #44

Conversation

fivetran-reneeli commented Jul 17, 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-reneeli commented Jul 18, 2024

fivetran-avinash left a comment • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-reneeli commented Jul 24, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-reneeli commented Jul 25, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-reneeli commented Jul 17, 2024 •

edited

Loading

fivetran-avinash left a comment •

edited

Loading

fivetran-avinash Jul 24, 2024 •

edited

Loading

fivetran-joemarkiewicz Jul 25, 2024 •

edited

Loading