Bug fix: apply offset for nested derived metrics #884
Conversation
courtneyholcomb
force-pushed
the
court/nested-derived-offset
branch
from
6a3078f to
a82ddea
Compare
courtneyholcomb
force-pushed
the
court/nested-derived-offset
branch
from
a82ddea to
ad5dec1
Compare
|
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. |
courtneyholcomb
changed the title
courtneyholcomb
changed the title
courtneyholcomb
force-pushed
the
court/nested-derived-offset
branch
from
dcff1cb to
ac56a58
Compare
There was a problem hiding this comment.
I have a concern about where this new time spine join is happening. I'll also leave this in other people's review queues so @plypaul can take a look. He's been spending a lot of time in this derived metric stack, so he may spot things I'm missing.
| @@ -234,14 +234,32 @@ def _build_derived_metric_output_node( | |||
| ), | |||
| metric_specs=[metric_spec], | |||
| ) | |||
| # For nested ratio / derived metrics with time offset, apply offset after metric computation. | |||
| join_to_time_spine_node: Optional[JoinToTimeSpineNode] = None | |||
| if metric_spec.offset_window or metric_spec.offset_to_grain: | |||
There was a problem hiding this comment.
What happens if there's a filter on metric_time?
Today we generally do:
- join
- filter
- aggregate
All of this is in ComputeMetricsNode, which comes post-aggregation, so I think we'll end up joining the time spine against the filtered input set here, effectively truncating the input data on the second offset.
If I'm right about this, we'll need to propagate the offset information so we can expand that window. For now it might be adequate to apply the filter expression after the final calculation if there's a nested derived offset, and then we can optimize the predicate handling later.
There was a problem hiding this comment.
Moving this discussion over to a new PR, based off of one of Paul's branches
|
Also, #879 looks relevant. |
Resolves #882
Description
Apply time offset for nested ratio or derived input metrics. Previously, we were only applying offset during measure aggregation. For these metrics, the offset needs to be applied post-metric aggregation.