Skip to content

Fix: normalize datetime values to strings in unit tests #4696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
georgesittas merged 1 commit into from
Jun 10, 2025
+41 −1
Merged

Fix: normalize datetime values to strings in unit tests #4696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion sqlmesh/core/test/definition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,12 +258,21 @@ def assert_equal(
actual = actual.replace({np.nan: None})
expected = expected.replace({np.nan: None})

# We define this here to avoid a top-level import of numpy and pandas
DATETIME_TYPES = (
datetime.datetime,
datetime.date,
datetime.time,
np.datetime64,
pd.Timestamp,
)

def _to_hashable(x: t.Any) -> t.Any:
if isinstance(x, (list, np.ndarray)):
return tuple(_to_hashable(v) for v in x)
if isinstance(x, dict):
return tuple((k, _to_hashable(v)) for k, v in x.items())
return str(x) if not isinstance(x, t.Hashable) else x
return str(x) if isinstance(x, DATETIME_TYPES) or not isinstance(x, t.Hashable) else x
Comment thread
georgesittas marked this conversation as resolved.

actual = actual.apply(lambda col: col.map(_to_hashable))
expected = expected.apply(lambda col: col.map(_to_hashable))
Expand Down
31 changes: 31 additions & 0 deletions tests/core/test_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2609,3 +2609,34 @@ def test_model_test_text_result_reporting_no_traceback(
prefix = "ERROR" if is_error else "FAIL"
assert f"{prefix}: test_foo (None)" in output
assert "Exception: failure" in output


def test_timestamp_normalization() -> None:
model = _create_model(
"SELECT id, array_agg(timestamp_col::timestamp) as agg_timestamp_col FROM temp_model_with_timestamp GROUP BY id",
meta="MODEL (name foo, kind FULL)",
)

_check_successful_or_raise(
_create_test(
body=load_yaml(
"""
test_foo:
model: temp_agg_model_with_timestamp
inputs:
temp_model_with_timestamp:
rows:
- id: "id1"
timestamp_col: "2024-01-02T15:00:00"
outputs:
query:
rows:
- id: id1
agg_timestamp_col: ["2024-01-02T15:00:00.000000"]
Copy link
Copy Markdown
Collaborator

@VaggelisD VaggelisD Jun 9, 2025

Choose a reason for hiding this comment

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

Would the same output comparison work properly if we compared an equivalent datetime value from a different string? E.g `2024-01-02T15:00:00.00" or one based on a different timezone that results in the same time?

Ideally we'd normalize all datetime types into a common representation at a common timezone (?)

Copy link
Copy Markdown
Collaborator Author

@georgesittas georgesittas Jun 9, 2025

Choose a reason for hiding this comment

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

I think so, there's a test for this: https://github.com/TobikoData/sqlmesh/blob/f87d3f0a6dad639584a4b70540ac19010c0ca92c/tests/core/test_test.py#L1220

Copy link
Copy Markdown
Collaborator

@VaggelisD VaggelisD Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This looks like it's on the same timezone from a quick look, I was referring to a scenario such as :

inputs:
   ...
   time_col: "2020-01-01 05:00:00+05"

outputs:
   ...
   time_col: "2020-01-01 00:00:00+00"

this test should be correct, right? But if it's a string comparison (or at best, normalized to a certain precision with zeroes) then it'd fail

Copy link
Copy Markdown
Collaborator Author

@georgesittas georgesittas Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that we encourage specifying time zones in the timestamp values (in the sense that it's unsupported).

The +zone syntax is only mentioned in relation to the execution_time variable in the docs, and I think that was intentional at the time.

"""
),
test_name="test_foo",
model=model,
context=Context(config=Config(model_defaults=ModelDefaultsConfig(dialect="duckdb"))),
).run()
)