Fix: Make plan dates relative to --execution-time

[erindru]

This addresses some issues with setting --execution-time on a plan, which is intended to run a plan "as at" a certain time.

Currently, if you specify a relative start date like --start '2 days ago' and then you specify a fixed --execution-time, eg --execution-time '2020-01-05', SQLMesh will make the start date relative to the current time and not the execution time.

This gets confusing because it makes the plan start / end and any relative dates show no relation to the execution time.

This is also causes problems when --execution-time is set to a value older than the latest prod interval for a model, eg:

$ sqlmesh plan dev --execution-time 2020-01-05 --start '2 days ago'
...SNIP
Error: Start date / time (2 days ago) can't be greater than end date / time (1749168000000)

This PR tightens up the behavior, adds new checks at the plan building stage and adds tests to verify the relative date behaviour.

After this PR, the above command works as intended by making the plan end time not able to exceed the execution time

[izeigerman]

Should we fall back to now() in constructor? So that we have it return the same value throughout the plan building process?

[erindru]

Yep, that seems to work. I've updated it

[erindru]

I've reverted defaulting to now() in the constructor and left a comment for next time, based on the conversation below

[izeigerman]

I wonder whether we should keep None if the execution_time wasn't specified explicitly. This way the execution_time will be determined at runtime, which is something I believe we want.

[erindru]

It's been defaulted to now() in the PlanBuilder constructor, right?

In general, allowing it to be None is a bit annoying because every place that needs to use the value has to implement fallback behaviour if the value is not specified.

If the fallback is always "default to now()" then isn't it better to make this explicit up front?

[izeigerman]

It's been defaulted to now() in the PlanBuilder constructor, right?

sorry, the 2 suggestions were made independently. As far as I can tell we don't fall back to now() in constructor in main.

In general, allowing it to be None is a bit annoying because every place that needs to use the value has to implement fallback behaviour if the value is not specified.

I think it's fine to have it as @cached_property in the builder and elsewhere. But I'd prefer the value to be correctly set at runtime since there can be a non-insignificant delay between plan construction and application.

[erindru]

Ok, so if I understand correctly, if it's sent to the PlanBuilder as None then that should still be None in the resulting Plan as well?

i.e as part of building the plan, don't default it to now()?

[izeigerman]

That's exactly what I mean, yes

[erindru]

Understood. I've removed the default in the constructor and use it like self._execution_time or now() when it is referenced in the PlanBuilder in places that might be dealing with relative time strings.

I stopped short of adding @cached_property for it because I thought it might be confusing having self.execution_time used within the PlanBuilder but self._execution_time being used when constructing the Plan

[izeigerman]

I stopped short of adding @cached_property for it because I thought it might be confusing having self.execution_time used within the PlanBuilder but self._execution_time being used when constructing the Plan

Isn't this problematic too? Now we get inconsistent execution_time throughout the plan building process

[erindru]

Ok, i've reverted this back to my original design except with @cached_property instead of @property and added a comment explaining what's going on