Metrics package #1
Conversation
…macro dispatch strategy
There was a problem hiding this comment.
Very cool stuff! I started pulling this apart just to understand how the pieces fit together, and in service of standing up some initial CI (#2). I didn't get to the breadth of review I'd hoped, hoping to get some more time later to keep going
This reverts commit a7e8940.
Add CI for four original adapters
There was a problem hiding this comment.
@joellabes - this is a masterpiece!! A masterpiece!!
I need to spend more time with this, and will ideally pull the branch down and take it for a spin locally. I left some high-level questions in here to start, but am definitely planning on taking another, deeper pass too :)
Really incredible work so far 💪 💪 💪
README.md
Outdated
| ``` | ||
|
|
||
| ### Time Grains | ||
| The package protects against nonsensical secondary calculations, such as a month-to-date aggregate of data which has been rolled up to the quarter. If you customise your calendar (for example by adding a [4-5-4 retail calendar](https://nrf.com/resources/4-5-4-calendar) month), you will need to override the `get_grain_order()` macro. In that case, you might remove `month` and replace it with `month_4_5_4`. All date columns must be prefixed with `date_` in the table, but this is not necessary in the model config. |
There was a problem hiding this comment.
just putting a placeholder to come back to this once i've scanned further down this PR :)
There was a problem hiding this comment.
@drewbanin if you had anything else to say on the topic, now's the time 🕐
macros/get_metric_relation.sql
Outdated
|
|
||
| {% macro get_metric_calendar(ref_name) %} | ||
| --TODO: this is HORRID. | ||
| {% do return(metrics.get_metric_relation([(ref_name.split("'")[1])])) %} |
There was a problem hiding this comment.
what does this macro do? I'm having a hard time following the logic
There was a problem hiding this comment.
I'm assuming you mean just the get_metric_calendar one - if you mean this whole file, refer to this thread
Basically, even though the metric yml file has its model as a full ref
- name: slack_joiners
model: ref('dim_slack_users_2')
the refs array from the graph contains just the string, inside a second array:
{
"fqn":["joel_sandbox","metrics","slack_joiners"],
"unique_id":"metric.joel_sandbox.slack_joiners",
"time_grains":["day", "week", "month"],
"dimensions":["has_messaged"],
"resource_type":"metric",
"refs":[
[
"dim_slack_users_2"
]
],
"created_at":1642578505.5324879
}
Whereas the calendar variable:
vars:
dbt_metrics_calendar_model: ref('all_days_extended_2')
comes through as the entire ref string (it hasn't been parsed or processed yet). This splits on the ' character, takes the second element, and wraps it inside an array, to have the same shape as get_metric_relation expects, which is a perfect fit for the metric's model style.
I'm 100% sure there's a better way to do it, but my graph-fu was failing me so I figured we'd just talk about it here instead.
There was a problem hiding this comment.
@jtcohen6 do you have any tricks up your sleeve that can make this less janky?
* first pass at setting up postgres integration tests * try number 2 * try number 3 * try number 4 * try number 5 * try number 6 * try number 7 * add snowflake integration tests * use snowflake target * add rest of 'em * try again please * fix target arg * fix syntax * use json for bigquery keyfile * fix env var * rename workflow * fix bigquery * more fix bigquery * more fix bigquery * re run * test * asdf * aha!
GitHub doesn't make it easy to do code review on a whole repository, so I'm tricking it by following these steps.
The big questions: