Added multiple export types #314
Conversation
| ("export_list", "cube"), | ||
| ("count_list", "cube"), | ||
| ("flat_list", "flat"), | ||
| ("meta_list", "meta"), |
There was a problem hiding this comment.
nit: I like the move away from the generic export_list, but count and meta are semantic, while flat is syntactic. I know that we maybe don't know the semantic range of flat tables (and/or think the range will be so large, we don't want a field for each), so flat is safe. But maybe count should be cube? And meta can continue being a snowflake. Not a strong opinion over here, just bumped on it.
I guess one question is - can we imagine a second kind of cube we'd add in the future? Would we want a new field for it or would we want to put it into the same field?
There was a problem hiding this comment.
good old naming things problems!
I kind of like semantic for these, in case someone who is not super steeped in this stuff tries to do it - it's a lot clearer to say 'put a count table here from the count generator' than 'make a powerset cube output and put it here'. If I had a semantic name for flat, i would've used it - obviously i don't want to call it 'metric', but maybe there's a better name?
my vibe on cubes is 'maybe at one point we would want a cube that wouldn't be aggregated for some reason'? I'm not sure if I can think of another use case off the dome.
There was a problem hiding this comment.
Yeah OK - leave it as it is 👍
| count_list = [ | ||
| "study_valid__table2" | ||
| ] | ||
| flat_list = [ | ||
| "study_valid__table2" | ||
| ] |
There was a problem hiding this comment.
This is an interesting set up - do we want to error out if the same table is in two lists? When would you intentionally want that?
There was a problem hiding this comment.
i don't think you would, no.
I can make this more realistic and add an error - note that this will generate a lot of test churn, sorry in advance.
There was a problem hiding this comment.
Hah, jokes on you, I just won't read the diff
There was a problem hiding this comment.
But it does seem nice to catch the duplicate-table-name thing. Seems like a classic copy and paste in a hurry error that folks might actually do.
There was a problem hiding this comment.
OK, we'll now throw an error in the case.
dogversioning
force-pushed
the
mg/export_types
branch
from
fe57115 to
6041233
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
This PR breaks out the existing
export_listsection of the manifest into several different typed sections, and appends suffixes based on which list section things are in.Checklist
docs/needs to be updatedgenerate-mdcorestudy fields that not in US Core, update our list of those incore-study-details.mdmanifest.toml