Add new experiments_column_type to support events_stream tables#507
Conversation
| """IF( | ||
| JSON_VALUE(ds.event_extra, '$.experiment') = '{experiment_slug}', | ||
| JSON_VALUE(ds.event_extra, '$.branch'), | ||
| NULL | ||
| ) != e.branch """ |
There was a problem hiding this comment.
Yea I think you're right that this matches the other checks a little more closely than just checking the branch alone, and either way it's no harm to check the experiment + branch.
|
|
||
| @pytest.mark.parametrize("experiments_column_type", [None, "simple", "native", "glean"]) | ||
| @pytest.mark.parametrize( | ||
| "experiments_column_type", [None, "simple", "native", "glean", "events_stream"] | ||
| ) | ||
| def test_datasource_constructor_succeeds(experiments_column_type): | ||
| DataSource( | ||
| name="foo", |
There was a problem hiding this comment.
It seems like there should be more tests than this (e.g., for the two functions you modified here). I know we have more comprehensive testing in jetstream so maybe not a big deal, but if you've got some time and inclination now want to tackle that here? Otherwise can you just file a ticket (in mozanalysis issues is good) to add unit tests for those two functions?
|
@relud Want to land this? Or are you working on the tests? |
i'll merge now and file a follow up pr for tests when i catch a break between other tasks |
2 participants
fixes mozilla/jetstream#2174
I'm unclear if there's more plumbing this needs.
I went for a condition that's going to match the other type's semantics of checking both experiment and branch, but I don't understand enough to tell if that's actually necessary or preferrable.