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.

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

Add country to ads insights country breakdown PK #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmosorast
Copy link
Contributor

Description of change

It seems that ads_insights_country is missing the country property from its Primary Key. This may cause targets that deduplicate based on the PK to have incorrect totals in situations where the values that were split by country get deduplicated back to the record as defined by the base insights PK.

Manual QA steps

  • None, this is to track what this will look like while I confirm that this is indeed an issue.

Risks

  • Medium, it's unclear what downstream effect changing the primary key will have, depending on target, but it may be safer since it's additive.

Rollback steps

  • revert this branch

@lserraga
Copy link

I've encountered this issue with a redshift target https://github.com/transferwise/pipelinewise-target-redshift. Resulting in ads_insights_country only having one row per day (1 country), regardless of how much data and countries tap-facebook extracted

@shyamodedraRK
Copy link

Same issue here, surely this report does not work as it stands so the breaking change this pull requests involves in changing the PK is needed ASAP...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants