Bug fix: apply offset for nested derived metrics #886
Conversation
|
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. |
| 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 |
There was a problem hiding this comment.
@tlento: 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.
@tlento I think you're right - I'll add a test case for that.
There was a problem hiding this comment.
@tlento - updated to handle that case!
|
Moving this PR over to a new PR based off of Paul's related branch. |
plypaul
force-pushed
the
plypaul--58.5--consolidate-into-measure-input-spec
branch
3 times, most recently
from
3995915
to
274d987
Compare
| expr: 2 * bookings_offset_once | ||
| metrics: | ||
| - name: bookings_offset_once | ||
| offset_window: 5 days |
There was a problem hiding this comment.
Can we make this offset value something different, maybe 2 days or 8 days? It's a little hard to pick up which offset is which with the INTERVAL values being the same.
| parent_node = parent_nodes[0] if len(parent_nodes) == 1 else CombineMetricsNode(parent_nodes=parent_nodes) | ||
| output_node: BaseOutput = ComputeMetricsNode(parent_node=parent_node, metric_specs=[metric_spec]) | ||
|
|
||
| # For nested ratio / derived metrics with time offset, apply offset & where constraint after metric computation. |
There was a problem hiding this comment.
This method got bigger in a hurry but I don't have any better suggestions since we wire so many parameters around.
courtneyholcomb
force-pushed
the
court/nested-offset-bug
branch
from
9485b77
to
e884632
Compare
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.