add school year to fct models where it was missing

[rlittle08]

Description & motivation

This adds school_year to several fct models. Previously, downstream queries needed to make an extra join (e.g. to dim_calendar_date or dim_student) to get school_year.

We are adding this for general convenience, as well as to support better output from the new automatic inventory output recently released in edu_data_quality_monitoring 0.1.4

Breaking changes introduced by this PR:

By default, this is non-breaking. However, certain queries/models may break downstream if they are following poor practice and failing to explicitly reference columns.

e.g., if there's a model or ad-hoc query like:

select
  ...,
  school_year
from fct_student_daily_attendance
join dim_calendar_date
...

--> WILL BREAK
but if referenced explicitly:

select
  ...,
  dim_calendar_date.school_year
from fct_student_daily_attendance
join dim_calendar_date
...

--> WILL NOT BREAK

PR Merge Priority:

• Low
• Medium
• High

Changes to existing files:

self-explanatory

New files created:

none

Tests and QC done:

This has been tested in our two implementations with the most dependent dbt packages (SC & TX), with no errors found. Row counts, metrics, etc. have NOT been checked, but I'm assuming those are safe, given the simple nature of these edits.

edu_wh PR Review Checklist:

Make sure the following have been completed before approving this PR:

• Description of changes has been added to Unreleased section of CHANGELOG.md. Add under ## New Features for features, etc.
• Code has been tested/checked for Databricks and Snowflake compatibility - EA engineers see Databricks checklist here
• Reviewer confirms the grain of all tables are unchanged, OR any changes are expected, communicated, and this PR is flagged as a breaking change (not for patch release)
• If a new configuration xwalk was added:
  • The code is written such that the xwalk is optional (preferred), and this behavior was tested, OR
  • The code is written such that the xwalk is required, and the required xwalk is added to edu_project_template, and this PR is flagged as breaking change (not for patch release)
  • A description for the new xwalk has been added to EDU documentation site here
• If a new configuration variable was added:
  • The code is written such that the variable is optional (preferred), and this behavior was tested, OR
  • The code is written such that the variable is required, and a default value was added to edu_project_template, and this PR is flagged as breaking change (not for patch release)
  • A description for the new variable has been added to EDU documentation site here

[tomreitz]

Hi @rlittle08 I'm approving this, it looks good to me. Thanks for your work here!

I do have two questions:

1. are there are other models that should also be included? like – from the query you slacked me – perhaps fct_student_program_service?
2. I noticed that some edu_ext_podium models have fields like display_school_year (here) or enrolled_school_year/assessed_school_year (here) instead of school_year – should we update the data_inventory() macro of edu_data_quality_monitoring to look for those columns when api_year and school_year are not found?

[tomreitz]

The changes in this file look great, but go beyond just adding school_year? Just want to make sure that's intentional.

[rlittle08]

yeah thanks, i realized while in here that this one was under documented, figured i'd knock it out

[rlittle08]

Hi @rlittle08 I'm approving this, it looks good to me. Thanks for your work here!

I do have two questions:

1. are there are other models that should also be included? like – from the query you slacked me – perhaps fct_student_program_service?
2. I noticed that some edu_ext_podium models have fields like display_school_year (here) or enrolled_school_year/assessed_school_year (here) instead of school_year – should we update the data_inventory() macro of edu_data_quality_monitoring to look for those columns when api_year and school_year are not found?

1. I tried to be as inclusive as I could of fcts that already have school_year in their ref'd tables. Program service is one where the stg tables don't keep it, and we'd have to add a dependent edu_edfi_source update. I think we just leave for now
2. Maybe, but I don't think necessary right now, as you can see here, for those ones I'm using the legacy code to alias them