-
Notifications
You must be signed in to change notification settings - Fork 15
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
Reports vendor info #2374
Reports vendor info #2374
Conversation
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.
Just a few comments, mostly style / kind of hypothetical (the least helpful kinds of comments? 😅 )
One specific thing to do once ready is to add YAML documenting the existence of this table. If you haven't already tested joining with idx_monthly_reports_site
I would maybe just double check that to make sure your grain is ok.
I think it's fine to have this in a separate table, I saw you asked in Slack about having to turn this into JSON if it were joined into idx_monthly_reports_site
. If you want to be able to use this to filter on the site, I suspect you may need to go that route (join and turn it into JSON on that table), but I could be wrong -- would defer to @ryon and @charlie-costanzo about that. (And just to be clear, that is a specific limitation of a specific adapter that we use in the reports site generation that couldn't handle arrays of structs; I still think that was the elegant way to go on that in the warehouse.)
Even if you do want to join it in, given how much logic there is in this, I think it could make sense to keep this as a separate table (intermediate in that case, as you note in the PR description) and then join it into the index table.
warehouse/models/mart/gtfs_quality/fct_monthly_reports_site_organization_gtfs_vendors.sql
Show resolved
Hide resolved
SELECT | ||
source_record_id, name, organization_type | ||
FROM {{ ref('dim_organizations') }} | ||
WHERE _is_current |
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.
Just noting that by adding this filter, you might risk having a failed join if an organization record for a vendor has been deleted by was active in the past. Don't think it's a huge risk because IIRC the service components tables aren't fully historical yet (which we may want to just double check on..... If there is anyone we know has changed vendors, I think we may need to look into making those fully historical so we aren't assessing a current vendor on a prior vendor's data?)
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.
Put another way, I think we might eventually need to make a vendor/components bridge table that uses the versioned key
s that you could use here 🤔
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.
So, I think this is probably ok for now, but maybe we need a ticket to make it more robust in future?
warehouse/models/mart/gtfs_quality/fct_monthly_reports_site_organization_gtfs_vendors.sql
Outdated
Show resolved
Hide resolved
warehouse/models/mart/gtfs_quality/fct_monthly_reports_site_organization_gtfs_vendors.sql
Outdated
Show resolved
Hide resolved
@lauriemerrell is there a preferred ordering for entries in |
It's pretty YOLO, I'd just add it in the end 😅 |
tested join with
|
I think this could be ready! Would appreciate a docs build/review since I can't do that using the Hub yet |
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.
LGTM 🚀 , I do think we might want a follow-up ticket to figure out the organization versioning here (probably low priority, I don't think that organization records get deleted often, but if a vendor changes names I think this logic would show the updated name.... That could maybe be desirable? But basically it just might be worth exploring further to confirm)
Description
A new table (or intermediate table to be joined to idx_monthly_reports_site) providing a high-level summary of vendors each organization uses for GTFS-schedule related tasks and GTFS-RT related tasks.
Supports
Type of change
How has this been tested?
Successful build of
fct_monthly_reports_site_organization_gtfs_vendors
on Eric's staging.Screenshots (optional)