Skip to content

Chore: Validate start and end dates during plan build time#5100

Merged
themisvaltinos merged 3 commits intomainfrom
themis/metadatastart
Aug 13, 2025
Merged

Chore: Validate start and end dates during plan build time#5100
themisvaltinos merged 3 commits intomainfrom
themis/metadatastart

Conversation

@themisvaltinos
Copy link
Contributor

Update to validate start is less than end date/time early during plan build time in the event that the user hasn't provided a start date in the config or in all models explicitly, fixes: #4916

@themisvaltinos themisvaltinos requested a review from a team August 6, 2025 16:20
# Validate that the start is not greater than the end date / time
effective_start = start or default_start
effective_end = end or default_end
if effective_start is not None and effective_end is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a separate function for this block

snapshot.node.start is None
or to_timestamp(snapshot.node.start) > end_ts
):
raise SQLMeshError(
Copy link
Collaborator

@izeigerman izeigerman Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right place to perform validation. With PlanBuilder users can set the start and end dynamically which means that currently they can pass this initial check and then set an invalid value later.

This should be moved into the PlanBuilder itself and the check needs to be performed every time build is called just like with other _ensure_* methods in the builder. Also, this should be PlanError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored and moved it to the _ensure_valid_date_range method which had similar logic and gets called in build

@themisvaltinos themisvaltinos force-pushed the themis/metadatastart branch 2 times, most recently from d93ca2b to 6e25a17 Compare August 8, 2025 16:44
@themisvaltinos themisvaltinos merged commit 9760ae0 into main Aug 13, 2025
25 of 28 checks passed
@themisvaltinos themisvaltinos deleted the themis/metadatastart branch August 13, 2025 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modifying model metadata often causes plan to fail if they haven't been run recently

3 participants