NYCPlanning / db-developments

🏠 🏘️ 🏗️ Developments Database
https://nycplanning.github.io/db-developments
8 stars 2 forks source link

Add in units mismatch check #521

Closed abrieff closed 2 years ago

abrieff commented 2 years ago

Adds in check for classa_net mismatch. Two things I wasn't sure about:

  1. Should the new file for adding new app specific qaqc checks be less specific, or should we plan on being more liberal with file creation?
  2. This follows the existing pattern of checks being binary fields across the entire table - pretty space intensive for an intermediate step. Should I move to instead only store the records with flags?
SashaWeinstein commented 2 years ago

Code makes sense to me at first glance. Not sure what you mean by question 1. For question 2 I think it does make sense to only store records with flags in the intermediate table, it means qaqc_app will have nulls instead of zeros for jobs that pass checks right?

abrieff commented 2 years ago
  1. is an architectural question of just: If we want to add more qaqc checks for the app in the future, should we plan for them to go in this same file/table? If so I'd probably name it something other than qaqc_app_units
  2. I could probably find some smart sql way to make them still zeroes on the fly?
SashaWeinstein commented 2 years ago
  1. Got it. I think it's more extendable to have a less specific name to give permission to the next engineer to add more columns via this new file. Think all those columns will be similar in purpose to classa_net_mismatch so makes sense for them to come from the same file
  2. If you find a way that's cool, if not we can deal with it downstream
abrieff commented 2 years ago

I think making the intermediate table only have records with flags requires an entirely different implementation to be extendable (otherwise how will manage that for multiple different columns with different checks) and would probably recommend leaving it as is for consistency/simplicity, if you disagree maybe we can chat about it

SashaWeinstein commented 2 years ago

Ok word this makes me think I don't understand yet. Shouldn't FINAL_devDB and FINAL_qaqc have the same list of job numbers?