Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature/remove-age #22

Merged
merged 7 commits into from
Dec 10, 2024
Merged

feature/remove-age #22

merged 7 commits into from
Dec 10, 2024

Conversation

fivetran-catfritz
Copy link
Contributor

@fivetran-catfritz fivetran-catfritz commented Dec 6, 2024 •
edited
Loading

PR Overview

This PR will address the following Issue/Feature:

This PR will result in the following new package version:

  • v0.7.0 since customers may need to resync

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

Breaking Changes

  • In the July 2023 update for the ADGROUP_HISTORY table, the age column was renamed to age_groups.
    • Previously in dbt_tiktok_source, we coalesced these two columns in the stg_tiktok_ads__ad_group_history model to account for connectors using the old naming convention. However, due to inconsistent data types, we can no longer use this approach.
    • As a result, the coalesced field has been removed in favor of the age_groups column.
    • If necessary, you can populate historical data in the age_groups column by performing a resync of the ADGROUP_HISTORY table, since TikTok provides all data regardless of the previous sync state.
    • For more details, see the DECISIONLOG entry.

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:

  • Minor update: coalesce was removed

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

💃

@fivetran-catfritz fivetran-catfritz self-assigned this Dec 6, 2024
fivetran-joemarkiewicz
fivetran-joemarkiewicz approved these changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with just a few small changes, but nothing blocking initial approval.

CHANGELOG.md Outdated Show resolved Hide resolved
DECISIONLOG.md Outdated Show resolved Hide resolved
packages.yml Outdated
Comment on lines 2 to 6
# version: [">=0.7.0", "<0.8.0"]
- git: https://github.com/fivetran/dbt_tiktok_ads_source.git
revision: feature/remove-age
warn-unpinned: false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder to swap before release.

CHANGELOG.md
# dbt_tiktok_ads v0.7.0
[PR #22](https://github.com/fivetran/dbt_tiktok_ads/pull/22) includes the following updates:

## Breaking Changes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should callout that these are upstream breaking changes. Since the age_groups column isn't used in any models within dbt_tiktok_ads it could be confusing to read this and wonder where the age_groups column was before. We should clearly callout that this change really only impacts the upstream staging model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

fivetran-catfritz and others added 2 commits December 9, 2024 12:51
@fivetran-catfritz @fivetran-joemarkiewicz
Apply suggestions from code review
5fcef85 
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
@fivetran-catfritz
update changelog
cbf42af
fivetran-catfritz
fivetran-catfritz commented Dec 9, 2024
View reviewed changes
Copy link
Contributor Author

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review. I have made the updates!

CHANGELOG.md
# dbt_tiktok_ads v0.7.0
[PR #22](https://github.com/fivetran/dbt_tiktok_ads/pull/22) includes the following updates:

## Breaking Changes
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

fivetran-jamie
fivetran-jamie reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of doc suggestions!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md
- If necessary, you can populate historical data in the `age_groups` column by performing a resync of the `ADGROUP_HISTORY` table, as TikTok provides all data regardless of the previous sync state.
- For more details, see the [DECISIONLOG entry](https://github.com/fivetran/dbt_tiktok_ads_source/blob/main/DECISIONLOG.md).


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we note the creation of the DECISIONLOG in a Documentation section here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with the stuff about metrics across different grains

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

DECISIONLOG.md Outdated

### Action for Customers Requiring Historical `age` Data
Customers who still need the historical `age` column data can:
1. Resync the `ADGROUP_HISTORY` table in their TikTok Ads connector. TikTok provides all historical data in the `age_groups` column, allowing the data to be fully populated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as source -- if there's no second option, we should remove the 1.

@fivetran-catfritz @fivetran-jamie
Apply suggestions from code review
1df1fbf 
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
This was linked to issues Dec 9, 2024
[Feature] Add documentation on differences among aggregations across different grains #10
Closed
[Documentation] Move badges below H1 heading #21
Closed
fivetran-catfritz
fivetran-catfritz commented Dec 9, 2024
View reviewed changes
Copy link
Contributor Author

@fivetran-catfritz fivetran-catfritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fivetran-jamie made the updates!

CHANGELOG.md
- If necessary, you can populate historical data in the `age_groups` column by performing a resync of the `ADGROUP_HISTORY` table, as TikTok provides all data regardless of the previous sync state.
- For more details, see the [DECISIONLOG entry](https://github.com/fivetran/dbt_tiktok_ads_source/blob/main/DECISIONLOG.md).


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

packages.yml Outdated Show resolved Hide resolved
fivetran-jamie
fivetran-jamie approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@fivetran-jamie fivetran-jamie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fivetran-catfritz fivetran-catfritz merged commit c7bb80a into main Dec 10, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fivetran-joemarkiewicz fivetran-joemarkiewicz fivetran-joemarkiewicz approved these changes

@fivetran-jamie fivetran-jamie fivetran-jamie approved these changes

Assignees

@fivetran-catfritz fivetran-catfritz

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@fivetran-catfritz @fivetran-jamie @fivetran-joemarkiewicz