Skip to content

Fix: convert pandas timestamp to py datetime for unit test generation#2149

Merged
treysp merged 2 commits intomainfrom
trey/unit-test-pd-timestamp
Feb 21, 2024
Merged

Fix: convert pandas timestamp to py datetime for unit test generation#2149
treysp merged 2 commits intomainfrom
trey/unit-test-pd-timestamp

Conversation

@treysp
Copy link
Contributor

@treysp treysp commented Feb 20, 2024

Problem/background

  • generate_test() creates a SQLMesh unit test YAML file based on a user-specified model name and input query
  • When we fetchdf the query results to Pandas, all datetime columns are converted to pd.Timestamp type (at least when queries run on DuckDB)
  • We use ruamel.yaml to serialize the data values returned by the query to YAML, and it does not support pd.Timestamp objects

This PR

  • Converts pd.Timestamp to datetime objects before writing query result data values to YAML
  • We want to maintain fidelity to the source column type. If it is DATE, we extract the date component from the datetime object before writing so a time component of 00:00:00 isn't included in the YAML.

@treysp treysp force-pushed the trey/unit-test-pd-timestamp branch 2 times, most recently from ad2090c to 8dd66f2 Compare February 20, 2024 17:44
@treysp treysp marked this pull request as ready for review February 20, 2024 17:45
@treysp treysp requested a review from georgesittas February 20, 2024 17:45
Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Is this currently tested in the integration suite? Should we also add a unit test for this?

Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Discussed in Slack about changing the for loop order for perf. reasons, but other than that LGTM 👍

@treysp treysp force-pushed the trey/unit-test-pd-timestamp branch 2 times, most recently from 6c8cbea to 93c7884 Compare February 20, 2024 23:21
@treysp treysp force-pushed the trey/unit-test-pd-timestamp branch from 93c7884 to acd3ee5 Compare February 20, 2024 23:22
georgesittas
georgesittas approved these changes Feb 21, 2024
@treysp treysp merged commit a0e2b4d into main Feb 21, 2024
@treysp treysp deleted the trey/unit-test-pd-timestamp branch February 21, 2024 16:54
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.

2 participants