-
Notifications
You must be signed in to change notification settings - Fork 3
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
Dbt cert josh #26
base: main
Are you sure you want to change the base?
Dbt cert josh #26
Conversation
06d42b0
to
2602696
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments :)
Also small not on commits - they should start with a capitalised imperative verb. That means 'added' -> 'Add' ; 'updated' -> 'Update' |
WITH returned_orders AS ( | ||
SELECT | ||
orders.customer_id AS customer_id | ||
orders.amount as total_amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this SQL would run - do you know why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missing comma, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it won't run because you're referencing an alias (orders) without assigning it first. Also, FYI we use leading rather than trailing commas:
SELECT
customer_id
, SUM(COALESCE(total_amount, 0)) AS total_value
FROM returned_orders
GROUP BY customer_id
@@ -1,6 +1,6 @@ | |||
with customers as ( | |||
|
|||
select * from {{ ref('stg_customers') }} | |||
select * from {{ ref('stg_customers_pii') }} | |||
|
|||
), | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've missed this in other PRs - SELECT * from cte's should generally not be used (only select columns that will be used ) + because this model uses PII, it should be renamed wh_customers_pii
and put in the sensitive schema. No need to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll revert this change then to just select from stg_customers
598933f
to
57c7e97
Compare
f1358e5
to
f1c5198
Compare
f1c5198
to
2036dac
Compare
No description provided.