Bug fix: incorrect time constraint for time offset metrics #932
Conversation
courtneyholcomb
requested review from
tlento and
plypaul
and removed request for
tlento
There was a problem hiding this comment.
Thanks for the fix!
I'm a little wary of these changes because the inlined special casing is hard to reason about. Looking at the snapshots it seems ok, but I think future readers will struggle.
That said, it seems like a lot of this is going to conflict with the approach @plypaul is using in his recent PRs, so some coordination seems in order there. In theory it should be straightforward to control when these time constraint filters get applied using the resolvers, but I don't know if all of the relevant handlers are built out for time constraints specifically.
| ON | ||
| ( | ||
| DATE_TRUNC(bookings_source_src_10001.ds, day) <= subq_15.ds | ||
| ) AND ( | ||
| DATE_TRUNC(bookings_source_src_10001.ds, day) > DATE_SUB(CAST(subq_15.ds AS DATETIME), INTERVAL 2 day) | ||
| ) | ||
| ) subq_16 | ||
| ON | ||
| DATE_SUB(CAST(subq_17.metric_time__day AS DATETIME), INTERVAL 2 day) = subq_16.metric_time__day |
There was a problem hiding this comment.
This all makes my head hurt, but I think it is correct - we query everything with a 2 day expansion at the start window, then we offset everything by 2 days and use the INNER JOIN to constrain the output times. And also have a where clause for that, but that's fine.
Resolves SL-1357
Resolves #925
Description
Fixes time constraint issues with time offset metrics. The time constraint was being applied in several places:
This PR removes 2 and 4.
2 is incorrect because the input metric subquery has not had the time offset applied yet, so the constrained values will be incorrect. Plus, one the input metric subquery is joined to the time spine subquery, the time constraint will effectively be applied since the queries use
INNER JOINon the time constraint column.3 is not incorrect, it's just redundant.
4 may appear redundant for cases when all input metrics have offsets, but in the case when one input metric is offset and another is not, it is necessary.