feature/api-updates #33

fivetran-catfritz · 2023-07-07T21:50:12Z

PR Overview

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

This PR will result in the following new package version:

v0.10.0

Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:

Changes stem from connector updates, so many columns have changed. See changelog for list.
Passthrough support added to any tables in the Fivetran ERD indicated they have custom fields.
Seeds, macros, docs updated
_fivetran_deleted filter added

PR Checklist

Basic Validation

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

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked and tagged
You are assigned to the corresponding issue and this PR
BuildKite integration tests are passing

Detailed Validation

Please acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":

You have validated these changes and assure this PR will address the respective Issue/Feature.
You are reasonably confident these changes will not impact any other components of this package or any dependent packages.
You have provided details below around the validation steps performed to gain confidence in these changes.

NOTE I could only find updated data for the below tables, so I had to run them individually. The below code can be used to run just these tables:

dbt run -s +stg_marketo__campaigns
&& dbt run -s +stg_marketo__program
&& dbt run -s +stg_marketo__lead

Updated seed data and used that for general checks.
Make sure to turn on campaigns and progams

  marketo__enable_campaigns:   true   
  marketo__enable_programs:    true

Passthroughs work:
New columns added:

Standard Updates

Please acknowledge that your PR contains the following standard updates:

Package versioning has been appropriately indexed in the following locations:
- indexed within dbt_project.yml
- indexed within integration_tests/dbt_project.yml
CHANGELOG has individual entries for each respective change in this PR
README updates have been applied (if applicable)
~~DECISIONLOG updates have been updated (if applicable)~~
Appropriate yml documentation has been added (if applicable)

dbt Docs

Please acknowledge that after the above were all completed the below were applied to your branch:

docs were regenerated (unless this PR does not include any code or yml updates)

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

💃

fivetran-joemarkiewicz

@fivetran-catfritz Thanks for working through these feature updates! Your updates looks great and appreciate the care and thought that went into updating the package to be in line with the connector updates.

I do have a few comments, suggestions, and questions before approving the PR. Let me know if you have any questions on my suggestions. Happy to chat in more detail if you would like!

CHANGELOG.md

fivetran-joemarkiewicz · 2023-07-11T14:25:32Z

models/stg_marketo__lead.sql

+        email,
+        first_name,
+        last_name
+
+        {{ fivetran_utils.fill_pass_through_columns('marketo__lead_passthrough_columns') }}


Is there a reason we opted for the passthrough columns here as opposed to just bringing all the fields in (like we had done previously)?

Thinking from the end users experience, it may be a great deal of work to go through these custom fields and manually add them to the passthrough variable. Especially if we automatically included all the fields in the previous version of the package.

I would actually suggest removing the passthrough variable for the lead models. Instead I would recommend we use the dbt-utils.star method as we had before, but we just exclude the new fields.

Let me know if you have any concerns with this approach or any questions on why we would remove the passthrough variable.

Now that I better understand the benefit of the .star method that makes total sense. Updated.

Thanks! However it seems we may have some new issues. I ran this locally with our production data and I am running into an error:

I think there are two issues coming from this update:

We have a missing comma if there are fields that fit the star criteria, but then we have a trailing comma if there are no fields 🤔

The cte is pulling from the macro results. Therefore, all of these fields that are included in the star macro are not included in the get_lead_columns macro. We will need to rework this staging model to account for these fields not included in the get_lead_columns macro. Possibly the workaround is similar to what I recently did in hubspot where the star macro is used in the same cte as the get_lead_columns macro. Then the renaming is done in the macro for the permanent fields.

Thanks, @fivetran-joemarkiewicz --I was able to adapt the hubspot strategy into here! My approach:

added aliases to the get_lead_columns macro

moved dbt_utils.star to the macro cte. I did have to adjust the approach to work with dbt_utils.star vs. the fivetran_utils from hubspot, but got it working in the end!

select * as the final select statement

macros/get_lead_columns.sql

macros/get_activity_send_email_columns.sql

models/stg_marketo__campaigns.sql

models/stg_marketo__program.sql

README.md

fivetran-joemarkiewicz

@fivetran-catfritz see my one unresolved comment for an issue I just came across. Let me know if you want to dig into it more together.

fivetran-joemarkiewicz

Great work on this @fivetran-catfritz very creative with your solution in the lead staging model!

I only have one required update before merging. Other than that, this is good to go!

models/stg_marketo__lead.sql

fivetran-joemarkiewicz · 2023-07-12T15:02:18Z

models/stg_marketo__lead.sql

+        -- This will check if there are non-default columns to bring in
+        {% set default_cols = ['id', 'created_at', 'updated_at', 'email', 'first_name', 'last_name'] %}
+        {% set new_cols = dbt_utils.star(from=ref('stg_marketo__lead_tmp'), except=default_cols) %}
+        {% if new_cols != '/* no columns returned from star() macro */' %}


This is really neat! We definitely will want to keep an eye on this macro in the dependent package to ensure this string doesn't change in the future.

Yeah... I didn't like hard coding the list, but when I tried to fetch the names dynamically like you did in Hubspot, the double quotes were surprisingly hard to deal with. I opted to do the hard code rather than lines and lines of code to replace the double quotes with single quotes needed for dbt_utils.star.

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

fivetran-catfritz added 9 commits July 7, 2023 16:38

feature/api-updates

7e1835b

feature/api-updates

141eb17

update seeds and regen docs

deafb27

update docs

e2b990f

update docs

1015a6d

update docs

33b7d1c

update changelog

410e388

update changelog

7ac31fd

update changelog

14a419b

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

[Feature] align campaign and program with connector updates #32

Closed

7 tasks

fivetran-catfritz self-assigned this Jul 10, 2023

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 11, 2023 01:06

fivetran-joemarkiewicz requested changes Jul 11, 2023

View reviewed changes

fivetran-catfritz added 2 commits July 11, 2023 10:43

model and doc updates

424e56b

model and doc updates

3ca1d04

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 11, 2023 19:14

fivetran-joemarkiewicz requested changes Jul 11, 2023

View reviewed changes

update lead column strategy

85aa60c

fivetran-catfritz requested a review from fivetran-joemarkiewicz July 11, 2023 22:26

fivetran-joemarkiewicz approved these changes Jul 12, 2023

View reviewed changes

fivetran-catfritz and others added 2 commits July 12, 2023 10:40

Update models/stg_marketo__lead.sql

3d0b65f

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

Update stg_marketo__lead.sql

3c3933b

fivetran-catfritz merged commit e348042 into main Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/api-updates #33

feature/api-updates #33

fivetran-catfritz commented Jul 7, 2023 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 11, 2023

fivetran-catfritz Jul 11, 2023

fivetran-joemarkiewicz Jul 11, 2023

fivetran-catfritz Jul 11, 2023

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jul 12, 2023

fivetran-catfritz Jul 12, 2023

feature/api-updates #33

feature/api-updates #33

Conversation

fivetran-catfritz commented Jul 7, 2023 • edited Loading

PR Overview

PR Checklist

Basic Validation

Detailed Validation

Standard Updates

dbt Docs

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-joemarkiewicz Jul 11, 2023

Choose a reason for hiding this comment

fivetran-catfritz Jul 11, 2023

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 11, 2023

Choose a reason for hiding this comment

fivetran-catfritz Jul 11, 2023

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jul 12, 2023

Choose a reason for hiding this comment

fivetran-catfritz Jul 12, 2023

Choose a reason for hiding this comment

fivetran-catfritz commented Jul 7, 2023 •

edited

Loading