Nov 2024 schema #18
Nov 2024 schema #18
Conversation
| cast(platform_version as {{ dbt.type_string() }}) as platform_version, | ||
| cast(source_type as {{ dbt.type_string() }}) as source_type, | ||
| cast(page_type as {{ dbt.type_string() }}) as page_type, | ||
| cast(pre_order as {{ dbt.type_string() }}) as pre_order, |
There was a problem hiding this comment.
just fyi. would have thought this is a boolean but apple docs has it as a string.
There was a problem hiding this comment.
@fivetran-reneeli thanks for this PR. A few comments below following this review.
| {{ | ||
| fivetran_utils.fill_staging_columns( | ||
| source_columns=adapter.get_columns_in_relation(ref('stg_apple_store__app_session_tmp')), | ||
| staging_columns=get_app_session_detailed_daily_columns() |
There was a problem hiding this comment.
Is this staging_columns parameter pointing at the correct macro?
There was a problem hiding this comment.
Just double checked, yes I believe so. Are you asking because the name is different? I cut off some of the characters because some warehouses had issue with long file names.
There was a problem hiding this comment.
Thanks! Yeah it was tripping me up that the names were divergent. But this makes sense and I can see the same approach used for other models in this update. No change required here.
| cast(original_start_date as date) as original_start_date, | ||
| cast(device as {{ dbt.type_string() }}) as device, | ||
| cast(client as {{ dbt.type_string() }}) as client, | ||
| cast(state as {{ dbt.type_string() }}) as state, |
There was a problem hiding this comment.
Can you confirm the state and quantity case statements are no longer necessary in this schema?
There was a problem hiding this comment.
I assume you meant the device case statement rather than quantity; for that one, I didn't see any values needing cleanup.
For state, actually now I noticed ' ' present in the data so thanks for calling attention to that-- I will add that case logic back for state.
| from fields | ||
| {{ dbt_utils.group_by(9) }} |
There was a problem hiding this comment.
Did the grain of this model change? Is there a reason we no longer need this group by?
There was a problem hiding this comment.
Yup I changed the grain-- I thought it was non-standard to have aggregations in the staging model. I understand why it was done (to only filter to relevant columns and aggregating quantity to ensure all values are captured), but I figured it made more sense to have everything in staging, then keep the aggregations in the transforms like how we have it elsewhere.
| cast(proceeds_reason as {{ dbt.type_string() }}) as proceeds_reason, | ||
| cast(subscription_offer_name as {{ dbt.type_string() }}) as subscription_offer_name, | ||
| cast(promotional_offer_id as {{ dbt.type_string() }}) as promotional_offer_id, | ||
| cast(state as {{ dbt.type_string() }}) as state, |
There was a problem hiding this comment.
Same question regarding if you confirm the state and quantity case statements are no longer necessary in this schema?
There was a problem hiding this comment.
Same comment as above
| from fields | ||
| {{ dbt_utils.group_by(8) }} |
There was a problem hiding this comment.
Same question regarding the grain of this model and why we grouped by before, but not now?
There was a problem hiding this comment.
Same comment as above
PR Overview
This PR will address the following Issue/Feature: #28
This PR will result in the following new package version: v0.5.0
Schema changes from Nov 2024
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
to be completed
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
If you had to summarize this PR in an emoji, which would it be?
💃