Create dim dates#37
Conversation
| extract(dayofyear from date_day) as day_of_year, | ||
|
|
||
| ## Week information ## | ||
|
|
There was a problem hiding this comment.
This looks good overall. One thing I’d flag is that we are mixing ISO week logic with calendar year logic here.
week_of_year uses extract(isoweek ...), but year_number uses the calendar year. Around year boundaries that can produce confusing combinations, for example ISO week 1 paired with the previous calendar year, or vice versa.
I think we should either add an iso_year field to keep the week logic consistent, or switch week_of_year to a non-ISO definition if we want this dimension to stay purely calendar-based.
harjeetkalsi
left a comment
harjeetkalsi
left a comment
There was a problem hiding this comment.
Nice addition overall. The model is clean and useful, and the core calendar fields look good.
The main thing I’d like us to address before merging is the week logic. We currently use isoweek for week_of_year, but calendar year for year_number, which can give confusing results around year boundaries. I think we should either add iso_year or make the week definition fully non-ISO for consistency.
A couple of smaller follow-ups:
- worth making the start/end dates configurable via
var() - optional, but a
date_keycould be useful if this is going to be a commonly joined dimension
… test for the iso calculation
|
Changes since last PR
|
There was a problem hiding this comment.
This is a really strong PR, well done Rosie!
You’ve taken the feedback on board and implemented the configurable date range cleanly using var(), which makes the model much more flexible across environments (by that I meean: test, dev prod). The use of generate_date_array to build the date spine is also a great choice.
The overall structure of the dim_date model is clear, well organised, and easy to follow, with good coverage across day, ISO week, month, quarter, and year.
Quality of your tests is great too. You’ve gone beyond basic checks and are validating logic and edge cases. The is_weekend test enforces consistency nicely, and the ISO year/week tests are strong as they cover boundary cases too.
You've built something that is reliable, testable, and prod ready.
Happy to approve :)
4 participants
Linked to this ticket
Creating a dim_date table with one row per day. This table isn't macro-dependent (as it was previously.
Calendar attributes are calculated using SQL and a weekend boolean value is added.
The idea is that this dim_table can be joined where time analysis is needed.
Tests are also added and pass.