Split covid_hosp into daily & timeseries tables

[rzats]

Prerequisites:

• Unless it is a documentation hotfix it should be merged against the dev branch
• Branch is up-to-date with the branch to be merged with, i.e. dev
• Build is successful
• Code is cleaned up and formatted

Summary

Currently, the covid_hosp_state_timeseries table stores data from two datasets at once - daily snapshots and timeseries data; the nature of this data is distinguished by the record_type column. This PR by:

• Updating the state_daily DDL with two tables instead of one
• Creating a migration SQL from one table into two
• Modifying the timeseries endpoint to use two tables instead of one

[krivard]

Sonarcloud is overzealous

[melange396]

nicely done! i think the suggestions i made in the .sql files might make sonarcloud chillax

[melange396]

this line is getting a little gnarly, but we will fix it with the other yaml-schema changes so you can leave it for now

[krivard]

I've swapped the quality gate profile to one with a 20% duplication threshold. It should take effect on the next push to this branch (I attempted a manual re-run but it doesn't want to listen to me)

(the duplicated lines are from database.py constructors so wouldn't be affected by .sql file changes)

[melange396]

oof, looks like that force-push lost some changes 😢

[rzats]

@melange396 that one's intended - I reverted a couple of changes because of #1126 (comment)

[melange396]

hmmm, this makes it look like you lost a change to integrations/server/test_covid_hosp.py and re-introduced a comment removed from src/ddl/covid_hosp.sql ¯\_(ツ)_/¯

[rzats]

@melange396 good point, re-added those two changes! (The tests actually pass just fine without the truncate table call, since this is the only test that uses these tables, but it's probably nice to have just in case.)

[melange396]

a few more points...

[rzats]

@melange396 applied the changes from this round as well!

[melange396]

this looks pretty good, though i havent given it a very deep inspection yet. in the meantime, you should include explain plans and timing for some "mocked" sample queries against real data (where we can use covid_hosp_state_timeseries twice, in place of the new table).

[rzats]

Requesting re-review after many iterations on the covid_hosp_state_timeseries query :)

The testing results of this query compared to the current one in dev can be found in this spreadsheet (tl;dr this outperforms dev significantly, in a certain branch by >300%). The optimizations made were:

1. switching the "get greatest value per group" logic to a more well-performing approach (PARTITION -> INNER JOIN)
2. incidentally fixing an error which may have caused the query to ignore the index intended for it (that index has the columns ordered as state -> date -> issue, but the query was previously grouping by date -> state -> issue)

[melange396]

his is awesome. its much easier to understand (IMNSHO), even though we moved away from the one overloaded table and into two (getting rid of the WITH expressions is a big help). plus the performance increases speak for themselves....

all comments marked 'nit:' are elective, you can decide whether you want to do them or not.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
2.3% Duplication

[rzats]

@melange396 applied the fixes!

[melange396]

awesome!

[melange396]

we probbly want to hold off on merging just yet, and first figure out whether to do this as a migration or as a re-load of all the data from the external sources