meltanolabs/dbt-facebook

dbt-facebook's Issues

Pat's General Review

This review is based on the development branch https://github.com/ryan-miranda-partners/dbt-facebook-meltano-sdk/tree/74a60e47f3e8a51afdb5875584830025dd0d88b1.

I think we'll want these to be standard dbt packages so they can be imported into someone's project easily. You should be able to follow dbt's package development guide https://docs.getdbt.com/guides/legacy/building-packages to update this to be consistent with that structure and recommended practices. Also our SQL style guide is https://handbook.meltano.com/data-team/sql-style-guide for your reference. I'll create a PR to add SQLFluff to this repo so you'll be setup with our configurations for aspects that can be automatically tested.

I also found https://github.com/fivetran/dbt_facebook_ads as an example of fivetran's dbt package if you we're aware of it and its helpful.

Repo Structure:

• Refresh repo to match dbt's package dev guide recommendations
• Add SqlFluff (Pat's PR)
• Fix SqlFluff errors
• README refresh

Fine Grain Code Review:

1. The dbt package dev guide suggests views instead of tables. What do you think about removing these table configs?

2. Can you explain a bit more about the jinja + lateral flattening youre doing(example 1, 2, 3, etc.)? Are those arrays with dynamic content where we want the table to expand over time? If not it would probably be better to explicitly define them.

3. I see hardcoded INDEX columns in a few place. Can you say more about what that column means and why it would be hard coded?

4. Theres also some hardcoded values that look like they might depend on how the tap is configured. For example, although I dont know what INTERVAL_DAYS is exactly, it smells like something thats user specific. It might be a case where you use dbt package variables to expose those to the user. Does that makes sense?

5. In label_history usually our style guide would recommend to using CTEs over subqueries in situations like this.

6. I still see some commented sections so I assume those will be cleaned up when this is finalized. Let me know when youre ready for a final review.

7. Consider adding tests

8. Is this supposed to be database instead? I don't think loader would be allowed in there.

@NeilGorman104 so far this looks like good progress 😄 ! I know it's a bit complicated to get everything to match. I left some general comments and some detailed comments on specific code references.

Fix sqlfluff CI linting

Improve README

Use https://github.com/MeltanoLabs/dbt-klaviyo as an example