-
Notifications
You must be signed in to change notification settings - Fork 2
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
add configurable stu-program-agg columns #73
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.
we talked through this idea but noting here that I think it's a common enough use-case to account for any additional complexity, especially considering the added logic to not require the new var
models/build/edfi_3/students/bld_ef3__student_program__homeless.sql
Outdated
Show resolved
Hide resolved
-- custom homeless program agg indicators | ||
{% if custom_program_agg_indicators is not none and custom_program_agg_indicators | length -%} | ||
{%- for indicator in custom_program_agg_indicators -%} | ||
{{ custom_program_agg_indicators[indicator]['agg_sql'] }} as {{ indicator }}, |
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.
What is the reason for nesting the SQL query under the agg_sql
key? Are there other keys that we will want to add in the future for flexibility?
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.
it is possible in future we want more attributes, e.g. include_in_dim_student
I've left two comments regarding general structure of the changes. Overall, this addition looks good and adds much-needed flexibility when it comes to custom programs. |
Description and Motivation
By default in EDU, programs have
is_active
andis_annual
as indicators that are aggregated byk_student
. Those are written in build models like here. This feature branch adds the ability to add arbitrarily many more of those types of student aggregate program indicators, with custom rules.For example, if you want to add a new indicator for whether a student was in a 504 Plan Program during the school year, you can configure these variable in your dbt_project.yml, assuming your implementation has
program_name
values equal to '504 Plan':And the compiled code in the build model will look like this:
And any student with a program enrollment with name '504 Plan' will now have
is_504_plan_annual
= TRUE indim_student
. Plus, those values will be automatically added todim_subgroup
andfct_student_subgroup
.Changes to existing models:
bld_ef3__student_program__special_education
, this code optionally adds more agg columns by grabbing sql code from the project config:edu_wh/models/build/edfi_3/students/bld_ef3__student_program__special_education.sql
Lines 36 to 41 in 543a8b6
bld_ef3__student_program__homeless
,bld_ef3__student_program__language_instruction
, andbld_ef3__student_program__title_i
)dim_student
, this code adds new custom indicators as columns:edu_wh/models/core_warehouse/dim_student.sql
Lines 93 to 96 in 543a8b6
bld_ef3__student_program__homeless
,bld_ef3__student_program__language_instruction
, andbld_ef3__student_program__title_i
)New models added:
None
Tests and QC Done:
Tested on Boston with configuration of 504 plan annual & active indicators.
Also tested on SCDE with configuration of homeless unaccompanied youth indicators.
Tested in both Boston and SCDE with an empty & missing config
If adding configuration, did you test with empty, broken, and nonexistent configuration?
Yes, I tested with no configuration added, and the new code compiles to empty.
PR Merge Priority:
(Promised this feature by end of Sept for BPS)
Questions:
This code assumes your configuration will include SQL to aggregate over
k_student
. What if you want to add ak_student_xyear
aggregation? Should that be included in this branch?