Enable querying cumulative metrics with agg_time_dimension

[courtneyholcomb]

Resolves #1000

Description

Enable querying cumulative metrics with their agg_time_dimension. Should have the same behavior as when queried with metric_time.

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

[tlento]

Looks good, thanks for splitting this up into such bite-sized chunks for review!

There are a few boundary cases I'd like to test here and read through before approving.

1. The time range constraint SQL rendering - the query runs, but I worry this is because the optimizer collapses the metric_time references into direct column name references, and it's not clear to me what the raw rendering will look like. I see this is tested for result correctness in the integration test (which is great) but there is no SQL to look at right now.
2. Do we render correctly for metrics that take multiple measures with the same agg_time_dimension?
3. Do we throw the right error for metrics that take multiple measures with different agg_time_dimensions?

[tlento]

Careful.... the ConstrainTimeRangeNode is hard-coded against metric time here:

https://github.com/dbt-labs/metricflow/blob/main/metricflow/plan_conversion/dataflow_to_sql.py#L991

If you follow that through the underlying property accessor it's just a set of TimeDimensionInstances manufactured out of thin air with the value of the METRIC_TIME_NAME enum entry.

It's possible this will still work due to magic, but we may need to change things up a bit to allow an override, otherwise time range constraints might not work properly for cases where people use the underlying agg_time_dimension name.

[courtneyholcomb]

Yep I'm aware! But good point that I actually I should revert the change on this line so it only applies the time constraint with metric_time - forgot to do that.
I had actually put up a separate issue for this since it's not directly related to the feature: #1008
Since the time constraint param isn't exposed in any of our APIs, only in the MF CLI, it seems less important to fix that issue before releasing this feature.
Note: in the integration test you noted, I used a where filter instead of a time constraint.

[courtneyholcomb]

I just put up a commit to raise a more appropriate error if metric_time is not used here.

[tlento]

Since the time constraint param isn't exposed in any of our APIs, only in the MF CLI, it seems less important to fix that issue before releasing this feature.

Right, makes sense, although we still don't want result divergence for people who do local dev with the CLI and then issue where filters in prod cloud accounts.

If I'm honest, though, I'm mostly looking ahead to predicate pushdown implementation concerns. :)

I'll take a look at the update!

[tlento]

Nice.

[courtneyholcomb]

Do we render correctly for metrics that take multiple measures with the same agg_time_dimension?
Do we throw the right error for metrics that take multiple measures with different agg_time_dimensions?

@tlento I realized this isn't relevant to this PR since cumulative metrics can't have multiple input measures. So I'll move dealing with this feedback to the time offset PR!

[tlento]

LGTM, thanks!