fix: prevent get_removal_interval from returning invalid interval#3879
fix: prevent get_removal_interval from returning invalid interval#3879
Conversation
tobymao
force-pushed
the
toby/get_removal_interval
branch
4 times, most recently
from
eabd032 to
e9f37da
Compare
tobymao
force-pushed
the
toby/get_removal_interval
branch
from
9d98fb8 to
0f96c9e
Compare
tobymao
force-pushed
the
toby/get_removal_interval
branch
5 times, most recently
from
b37ff4d to
ba97893
Compare
|
|
||
| if not restating_parents and snapshot.name not in restate_models: | ||
| continue | ||
| if not removal_interval: |
There was a problem hiding this comment.
This doesn't look correct. What if removal_interval is empty, but restating_parents is not?
There was a problem hiding this comment.
Shouldn't this be
if not possible_intervals
instead?
There was a problem hiding this comment.
this used to be true before interval expansion, because we wanted to have daily parents restate, but not the monthly. i think this doesn't make sense anymore
| self._context_diff.snapshots[s] for s in snapshot.parents if s in restatements | ||
| ] | ||
|
|
||
| if not restating_parents and snapshot.name not in restate_models: |
There was a problem hiding this comment.
Should we do
removal_interval = snapshot.get_removal_interval(
after this check and not before?
There was a problem hiding this comment.
Btw, this check doesn't seem useful, since restating_parents doesn't filter out non-incremental models. So restating_models might be non-empty, but then possible_intervals will be empty.
There was a problem hiding this comment.
possible_intervals can be empty because parents are full, but get_removal_interval should always exist with expansion
tobymao
force-pushed
the
toby/get_removal_interval
branch
3 times, most recently
from
4ecff8d to
0aeb1a9
Compare
if your partial restate a daily model -> monthly, (2024-02-15, 2024-02-20), get_removal_interval returned an interval where start > end. this ensures get_removal_interval only returns valid intervals.
tobymao
force-pushed
the
toby/get_removal_interval
branch
from
0aeb1a9 to
84df3ce
Compare
2 participants
if your partial restate a daily model -> monthly, (2024-02-15, 2024-02-20), get_removal_interval returned an interval where start > end. this ensures get_removal_interval only returns valid intervals.