PR feedback

Author: erindru

sqlmesh/core/context.py

@@ -120,7 +120,14 @@
 from sqlmesh.utils import UniqueKeyDict, Verbosity
 from sqlmesh.utils.concurrency import concurrent_apply_to_values
 from sqlmesh.utils.dag import DAG
-from sqlmesh.utils.date import TimeLike, now_ds, to_timestamp, format_tz_datetime, now_timestamp
+from sqlmesh.utils.date import (
+    TimeLike,
+    now_ds,
+    to_timestamp,
+    format_tz_datetime,
+    now_timestamp,
+    now,
+)
 from sqlmesh.utils.errors import (
     CircuitBreakerError,
     ConfigError,
@@ -1517,7 +1524,7 @@ def plan_builder(
                 max_interval_end_per_model,
                 backfill_models,
                 modified_model_names,
-                execution_time,
+                execution_time or now(),
             )
 
             # Refresh snapshot intervals to ensure that they are up to date with values reflected in the max_interval_end_per_model.

sqlmesh/core/plan/builder.py

@@ -42,6 +42,7 @@
     yesterday_ds,
     to_timestamp,
     time_like_to_str,
+    is_relative,
 )
 from sqlmesh.utils.errors import NoChangesPlanError, PlanError
 
@@ -139,7 +140,14 @@ def __init__(
         self._include_unmodified = include_unmodified
         self._restate_models = set(restate_models) if restate_models is not None else None
         self._effective_from = effective_from
+
+        # note: this deliberately doesnt default to now() here.
+        # There may be an significant delay between the PlanBuilder producing a Plan and the Plan actually being run
+        # so if execution_time=None is passed to the PlanBuilder, then the resulting Plan should also have execution_time=None
+        # in order to prevent the Plan that was intended to run "as at now" from having "now" fixed to some time in the past
+        # ref: https://github.com/TobikoData/sqlmesh/pull/4702#discussion_r2140696156
         self._execution_time = execution_time
+
         self._backfill_models = backfill_models
         self._end = end or default_end
         self._apply = apply
@@ -176,18 +184,22 @@ def is_start_and_end_allowed(self) -> bool:
 
     @property
     def start(self) -> t.Optional[TimeLike]:
-        if self._start and self._execution_time:
-            return to_datetime(self._start, relative_base=to_datetime(self._execution_time))
+        if self._start and is_relative(self._start):
+            # only do this for relative expressions otherwise inclusive date strings like '2020-01-01' can be turned into exclusive timestamps eg '2020-01-01 00:00:00'
+            return to_datetime(self._start, relative_base=to_datetime(self.execution_time))
         return self._start
 
     @property
     def end(self) -> t.Optional[TimeLike]:
-        if self._end and self._execution_time:
-            return to_datetime(self._end, relative_base=to_datetime(self._execution_time))
+        if self._end and is_relative(self._end):
+            # only do this for relative expressions otherwise inclusive date strings like '2020-01-01' can be turned into exclusive timestamps eg '2020-01-01 00:00:00'
+            return to_datetime(self._end, relative_base=to_datetime(self.execution_time))
         return self._end
 
-    @property
+    @cached_property
     def execution_time(self) -> TimeLike:
+        # this is cached to return a stable value from now() in the places where the execution time matters for resolving relative date strings
+        # during the plan building process
         return self._execution_time or now()
 
     def set_start(self, new_start: TimeLike) -> PlanBuilder:
@@ -274,7 +286,8 @@ def build(self) -> Plan:
         )
 
         restatements = self._build_restatements(
-            dag, earliest_interval_start(self._context_diff.snapshots.values(), self.execution_time)
+            dag,
+            earliest_interval_start(self._context_diff.snapshots.values(), self.execution_time),
         )
         models_to_backfill = self._build_models_to_backfill(dag, restatements)
 
@@ -284,6 +297,12 @@ def build(self) -> Plan:
             # model should be ignored.
             interval_end_per_model = None
 
+        # this deliberately uses the passed in self._execution_time and not self.execution_time cached property
+        # the reason is because that there can be a delay between the Plan being built and the Plan being actually run,
+        # so this ensures that an _execution_time of None can be propagated to the Plan and thus be re-resolved to
+        # the current timestamp of when the Plan is eventually run
+        plan_execution_time = self._execution_time
+
         plan = Plan(
             context_diff=self._context_diff,
             plan_id=self._plan_id,
@@ -307,7 +326,7 @@ def build(self) -> Plan:
             selected_models_to_backfill=self._backfill_models,
             models_to_backfill=models_to_backfill,
             effective_from=self._effective_from,
-            execution_time=self.execution_time,
+            execution_time=plan_execution_time,
             end_bounded=self._end_bounded,
             ensure_finalized_snapshots=self._ensure_finalized_snapshots,
             user_provided_flags=self._user_provided_flags,

sqlmesh/core/plan/definition.py

@@ -83,8 +83,9 @@ def start(self) -> TimeLike:
     def end(self) -> TimeLike:
         return self.provided_end or self.execution_time
 
-    @property
+    @cached_property
     def execution_time(self) -> TimeLike:
+        # note: property is cached so that it returns a consistent timestamp for now()
         return self.execution_time_ or now()
 
     @property

sqlmesh/utils/date.py

@@ -373,6 +373,16 @@ def is_categorical_relative_expression(expression: str) -> bool:
     return not any(k in TIME_UNITS for k in grain_kwargs)
 
 
+def is_relative(value: TimeLike) -> bool:
+    """
+    Tests a TimeLike object to see if it is a relative expression, eg '1 week ago' as opposed to an absolute timestamp
+    """
+    if isinstance(value, str):
+        return is_categorical_relative_expression(value)
+
+    return False
+
+
 def to_time_column(
     time_column: t.Union[TimeLike, exp.Null],
     time_column_type: exp.DataType,

tests/integrations/github/cicd/test_github_controller.py

@@ -19,6 +19,7 @@
     GithubCheckStatus,
     MergeStateStatus,
 )
+from sqlmesh.utils.date import to_datetime, now
 from tests.integrations.github.cicd.conftest import MockIssueComment
 
 pytestmark = pytest.mark.github
@@ -236,6 +237,7 @@ def test_pr_plan(github_client, make_controller):
 def test_pr_plan_auto_categorization(github_client, make_controller):
     custom_categorizer_config = CategorizerConfig.all_semi()
     default_start = "1 week ago"
+    default_start_absolute = to_datetime(default_start, relative_base=now())
     controller = make_controller(
         "tests/fixtures/github/pull_request_synchronized.json",
         github_client,
@@ -249,7 +251,7 @@ def test_pr_plan_auto_categorization(github_client, make_controller):
     assert not controller._context.apply.called
     assert controller._context._run_plan_tests.call_args == call(skip_tests=True)
     assert controller._pr_plan_builder._categorizer_config == custom_categorizer_config
-    assert controller.pr_plan.start == default_start
+    assert controller.pr_plan.start == default_start_absolute
 
 
 def test_prod_plan(github_client, make_controller):

tests/utils/test_date.py

@@ -12,6 +12,7 @@
     date_dict,
     format_tz_datetime,
     is_categorical_relative_expression,
+    is_relative,
     make_inclusive,
     to_datetime,
     to_time_column,
@@ -324,3 +325,17 @@ def test_format_tz_datetime():
     test_datetime = to_datetime("2020-01-01 00:00:00")
     assert format_tz_datetime(test_datetime) == "2020-01-01 12:00AM UTC"
     assert format_tz_datetime(test_datetime, format_string=None) == "2020-01-01 00:00:00+00:00"
+
+
+def test_is_relative():
+    assert is_relative("1 week ago")
+    assert is_relative("1 week")
+    assert is_relative("1 day ago")
+    assert is_relative("yesterday")
+
+    assert not is_relative("2024-01-01")
+    assert not is_relative("2024-01-01 01:02:03")
+    assert not is_relative(to_datetime("2024-01-01 01:02:03"))
+    assert not is_relative(to_timestamp("2024-01-01 01:02:03"))
+    assert not is_relative(to_datetime("1 week ago"))
+    assert not is_relative(to_timestamp("1 day ago"))