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 fct_order_items model #3

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add fct_order_items model #3

wants to merge 5 commits into from

Conversation

JDW818
Copy link
Owner

@JDW818 JDW818 commented Jul 17, 2019

Add fct_order_items model to join orders and customers.

@JDW818 JDW818 requested a review from eogilvy12 July 17, 2019 20:48
Copy link
Collaborator

@eogilvy12 eogilvy12 left a comment

Choose a reason for hiding this comment

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

this was totally on my wording but I left a better phrased question that will require an alteration to your code

models/example/fct_order_items.sql Show resolved Hide resolved
Copy link
Collaborator

@eogilvy12 eogilvy12 left a comment

Choose a reason for hiding this comment

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

a few comments in line! happy to discuss in person tomorrow

models/example/order_calculations.sql Outdated Show resolved Hide resolved
models/example/order_calculations.sql Outdated Show resolved Hide resolved

select
source1.id as customer_id,
lag(source2.created_at, 1) [ignore nulls]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you want to pull from stg_orders and stg_order_items

your query can look like:

select
    items.*,
    orders.email,
    orders.customer_id,
    orders.order_date
from items
inner join order using (order_id)

and then do any of the additional modeling that you want for the window functions

models/example/order_calculations.sql Show resolved Hide resolved
@JDW818 JDW818 requested a review from eogilvy12 July 22, 2019 20:30
Copy link
Collaborator

@eogilvy12 eogilvy12 left a comment

Choose a reason for hiding this comment

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

comments on some changes! I think there is a high level topic for us to discuss tomorrow which is DAG related - if you are building a model like order_calculations, you want to select * from orders and then add the calculations. that way, when you do to visualize the data in mode/looker, you are only having to select from this new model to get all info for orders

#}
with orderitems as (

select * from {{ ref('order_items_upload') }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be ref the staging models that you built! stg_orders and stg_order_items

select
orderitems.id as order_item_id,
orderitems.order_id,
orderitems.price,
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is not typical to list out explicit order_item fields since you have already done that in the stg model. typically, it will be select orderitems.*, orders.whichever fields. let me know if this makes sense!


),

orderitems as (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you use the order_items CTE anywhere in this model so you can remove

calc as (

select
orders.customer_id,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is another situation where you want to select *, add_new_fields so that your downstream model has all of the fields and more for you to select from

we can talk about this in more detail during our check in tomorrow!

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.

2 participants