Consolidate metric constraints in DataflowPlanBuilder

[WilliamDee]

Context

#352 (comment)

Changes

Essentially in the DataflowPlanBuilder, given a MetricSpec

1. we get the Metric object corresponding to that MetricSpec and combine the where constraint defined in the Metric
2. combine on the where constraint defined in the MetricSpec as well

Currently, it doesn't seem like there's a use case of doing both of these other than to cause slight confusion. At runtime when building the query, we just pass a list of MetricSpec to the builder that was initialized by the metric names only. So to remove the redundancy here, in the query parser we should attach the constraint to the MetricSpec that gets passed to the builder so that we don't have to retrieve the metric constraint from the Metric object directly downstream.

[jzhu13]

Should this assertion have existed before or is this now needed due to how the constraints are being bundled?

[WilliamDee]

just a sanity check that shouldve existed before, something wouldve went wrong if len == 0.

[tlento]

Thanks, this looks less weird! I do think some added clarity around how we populate the spec constraint would be good to have before merge.

[tlento]

Hmmm. It was a little hard to wrap my head around this.

The reason we do base_metric_references is because we only evaluate constraints on metrics at this point. The input_metrics to a DERIVED metric have the constraints pushed into the spec inside MetricSemantics.

It might be worth it to consolidate this further - right now we do top level spec construction in the query parser and upstream dependent spec construction inside MetricSemantics. But I also think that's likely opening up a can of worms, since MetricSemantics is in need of some restructuring itself.

So, I've convinced myself we should leave this here. But let's add a comment somewhere in this vicinity for why the base_metric_references are the things we care about here.

Also, I suggest initializing the local variable right above this block, it's not as important as putting it on line 233 makes it seem. We might even want to factor this all out into a helper method, then use the docstring to explain everything that's going on. I think I like that best.