Fix: ad creative url parsing--child links #22
Conversation
There was a problem hiding this comment.
Left one comment in the code. In addition: why are we only implementing this on Snowflake? If this data exists in Snowflake, then it also exists in Redshift. It seems like we should implement this for both, right? I understand that the SQL on Redshift would be quite different, but I have to imagine that the same transformation is possible...
|
|
||
| {% if target.type == 'snowflake' %} | ||
|
|
||
| links_joined as ( |
There was a problem hiding this comment.
@axelazaid we can't do this! :) you're in the "default" version of this macro--if you need to do something specific to snowflake, you need to do that using a snowflake version of the macro, just like we do everywhere else. We can never write an if statement like this, it's very bad for maintainability of this code moving forwards.
I'm happy to talk this through with you, it's something i've also discussed with erin and chris recently as well.
There was a problem hiding this comment.
i think i understand where you're coming from, but i'm happy to go through this with you!
There was a problem hiding this comment.
@axelazaid this PR is getting a bit stale at this point. Is this still something that you'd like to get merged? at this point, the if/then logic is the blocker here. this would need to be rewritten in order to get the PR merged.
There was a problem hiding this comment.
@jthandy i made those changes last week in this branch. there's still some changes that need to be done on the url-parsing aspect though (related to the most recent pr of coalescing url_tags & url), before this could be looked at again
|
@jthandy this is ready for another review! |
There was a problem hiding this comment.
This is progress! Thanks for continuing to push forwards here. My feedback is pretty consistent with other feedback that I've given on other package-related PRs: our code needs to be very clean when we're working on packages that will be implemented in dozens or hundreds of different projects. At the moment we're not quite there.
|
|
||
| links_joined as ( | ||
|
|
||
| select | ||
|
|
||
| id as creative_id, |
There was a problem hiding this comment.
you've added a join to this CTE, which now makes it very challenging to tell which fields come from which of the tables that are being joined. please prefix all fields with the table that they're coming from in this situation so that others can understand your code when they read through it!
| @@ -8,39 +8,55 @@ | |||
| {% macro default__stitch_fb_ad_creatives() %} | |||
There was a problem hiding this comment.
almost all of the implementation of this model across redshift and snowflake is identical. we should refactor this to make the code more DRY.
| @@ -0,0 +1,101 @@ | |||
| -- There are cases where ads can have multiple links attached to them. | |||
There was a problem hiding this comment.
The core logic that you're trying to do across redshift and snowflake is identical here, it's just that Stitch loads the data in differently. My recommendation would be to create a base model that normalizes the schemas across both Redshift and snowflake, and then do the transformation in a single model that doesn't need two different adapters because the underlying data is identical. We can accomplish what you're doing here much more simply and minimize the maintenance burden for future analysts.
…nto fix/ad-creative-url-parsing
|
@axelazaid if you're confident that this works the various adapters, i'm good to go! I just want to make sure that you're triple-checked that the results are good on top of real data. |
|
yup! this works 100%. tested on redshift and snowflake :) merging now |
built new model
fb_ad_creatives__child_linksupdated existing
fb_ad_creativesmodel that joins tofb_ad_creatives__child_linksurlfieldvariable
ad_creatives__child_links_tablecreated indbt_project.ymlto be enabled/disabled depending on the warehousePasses on snowflake and redshift