Dataflow plan for metrics in filters

[courtneyholcomb]

Resolves #740

Description

Build source nodes for requested GroupByMetrics during dataflow plan building. This is the last step needed to enable these metrics/queries! 🎉

Most of these joins require a join from a primary key to a foreign key, which is normally not allowed because a fan-out join might make metric calculation inaccurate. When joining to a pre-aggregated metric, though, we can allow that type of join since the aggregation prevents duplicate rows. We can ensure that's true by allowing no more than one entity in the join columns for GroupByMetrics. For V0 of this feature we'll implement that restriction. Later, we will want to enable multiple join columns (both entities and dimensions), so we will need to clarify what types of joins are allowed here.

Other than that, this adds some tests and updates a couple of places with group by metric logic that was missed in previous PRs. I would recommend reviewing by commit.

Note the scope of this PR - it enables metric filters on metrics (in YAML) and in metric queries, joining by exactly one entity. It does not enable 1) metrics in the group by for queries, 2) joining to metrics via dimensions or multiple group bys, or 3) metrics in filters for distinct values queries (queries without metrics). Those will be handled in lower-priority follow ups.

[tlento]

🎉

I was expecting this particular PR to be a lot more complicated than this, very nice!

I left two minor things inline for consideration - the sequences and the check query simplification - but otherwise this looks great! Thanks for splitting things up for me, made review a lot more straightforward.

[tlento]

Oof, I guess we do have to thread this through everywhere, eh?

[tlento]

Ok, this will do for now but we'll need a more robust way of handling this than the inlined isinstance check once we have other cases we need to think about. I'll keep this in mind as I read more of these updates, because we want a way to ask if the join is ok due to aggregation state, and we may need one that works across node types.

I don't think we have access to AggregationState from dataflow plan nodes, but something like that might be what we should be checking against - that and the grain, because we'll need to match the grain eventually.

[courtneyholcomb]

Agreed! Definitely will need more robustness here in v2.

[tlento]

Can we add a TODO here to make this more robust against different grains and different node types that might satisfy this condition?

[tlento]

Oh ok but this is not one with a window so it resolves to the same as the active_listings example.

[courtneyholcomb]

Yes! I didn't add any with window or grain_to_date yet because those require querying with metric_time, which is not yet supported for metric group bys. So you'll get an error if you try to use them anyway.

[tlento]

Right, we need the LEFT OUTER here because of listing__bookings IS NULL OR listing__bookings > 2 or similar expressions.

[tlento]

These are all pretty clearly generated via --compile or whatever so they're kind of hard to read (and also that means they're asserting that the compile output doesn't change semantically, but do we know the results are correct here?).

It might be worth a cleanup pass to simplify this SQL down to its most basic by hand. Then we can look at the optimizers later to see if we can cut back on the repetition here.

Also, I think this feature makes CTEs more important, so maybe we should surface that in the feature roadmap. We don't need them right away but they've got to get some higher precedence, because this output will be quite difficult to read.

[courtneyholcomb]

For the really complex queries I do usually just let MF compile the SQL, then I read through it to verify and simplify it a little before adding it here. I see most of the value in these tests as making sure that the query runs end to end without error and that the SQL actually executes successfully (i.e., no columns are dropped unexpectedly, etc.)
So I'll do 2 things:

• go back through these to simplify them as much as I can
• add output tests

[courtneyholcomb]

Simplified the SQL more - will add output tests tomorrow.