Skip to content
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

Jump to bottom

SNOW-984699: Add support for to_timestamp_* to local testing #1244

Merged
merged 23 commits into from
Mar 7, 2024
Merged

SNOW-984699: Add support for to_timestamp_* to local testing #1244

merged 23 commits into from
Mar 7, 2024
+623 −138

Conversation

sfc-gh-jrose
Copy link
Contributor

@sfc-gh-jrose sfc-gh-jrose commented Feb 7, 2024 •
edited
Loading

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-984699

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency

  3. Please describe how your code solves the related issue.

    This PR does two main things:

    1. Adds support for to_timestamp_* functions which convert any compatible type into a relevant timestamp type.
    2. Refactors the related tests so that the dataframes are created without sql and parameterizes them so that they are more granular when they fail. I've also added additional examples so that there's better type coverage in the tests.

Open Questions:

  • This PR assumes that we want to hold datetime objects in the column emulators. I've chosen that because that is what we get back when collecting normal dataframes, but we could also store the datetimes as numpy.datetime64 objects which is what pandas would do natively.

@sfc-gh-jrose sfc-gh-jrose marked this pull request as ready for review February 8, 2024 19:37
@sfc-gh-jrose sfc-gh-jrose requested a review from a team as a code owner February 8, 2024 19:37
@sfc-gh-jrose sfc-gh-jrose force-pushed the jrose_snow_984699_local_testings_to_timestamp_support branch from 64d5788 to de0f3c3 Compare February 13, 2024 17:36
sfc-gh-aling
sfc-gh-aling reviewed Feb 22, 2024
View reviewed changes
src/snowflake/snowpark/functions.py Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
src/snowflake/snowpark/mock/_functions.py Show resolved Hide resolved
src/snowflake/snowpark/mock/_functions.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/mock/_functions.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/mock/_functions.py Outdated Show resolved Hide resolved
src/snowflake/snowpark/mock/_functions.py Show resolved Hide resolved
src/snowflake/snowpark/mock/_functions.py Show resolved Hide resolved
src/snowflake/snowpark/mock/_functions.py Outdated Show resolved Hide resolved
tests/utils.py Show resolved Hide resolved
@sfc-gh-jrose sfc-gh-jrose requested a review from a team as a code owner March 1, 2024 17:32
@sfc-gh-jrose sfc-gh-jrose force-pushed the jrose_snow_984699_local_testings_to_timestamp_support branch from cd8bad4 to 4b48612 Compare March 1, 2024 17:46
@sfc-gh-jrose sfc-gh-jrose changed the title SNOW-984699: Add support for as_timestamp_* and to_timestamp_* to local testing SNOW-984699: Add support for to_timestamp_* to local testing Mar 1, 2024
sfc-gh-aalam
sfc-gh-aalam approved these changes Mar 6, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-aalam sfc-gh-aalam left a comment

Choose a reason for hiding this comment

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

looks good to me

src/snowflake/snowpark/functions.py
Comment on lines +3160 to +3162
Per default fmt is set to auto, which makes Snowflake detect the format automatically. With `to_timestamp` strings
can be converted to timestamps. The format has to be specified according to the rules set forth in
<https://docs.snowflake.com/en/sql-reference/functions-conversion#date-and-time-formats-in-conversion-functions>
Copy link
Contributor

Choose a reason for hiding this comment

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

a simple example here would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if I can add a ntz example. Unfortunately I can't control the host tz for ltz an tz functions so examples there might fail doctests depending on the host tz.

sfc-gh-aling
sfc-gh-aling reviewed Mar 7, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-aling sfc-gh-aling left a comment

Choose a reason for hiding this comment

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

only minor comments, overall changes LGTM!

@sfc-gh-jrose sfc-gh-jrose enabled auto-merge (squash) March 7, 2024 20:20
@sfc-gh-jrose sfc-gh-jrose merged commit 624cffb into main Mar 7, 2024
56 of 57 checks passed
@sfc-gh-jrose sfc-gh-jrose deleted the jrose_snow_984699_local_testings_to_timestamp_support branch March 7, 2024 22:26
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sfc-gh-aalam sfc-gh-aalam sfc-gh-aalam approved these changes

@sfc-gh-aling sfc-gh-aling sfc-gh-aling approved these changes

@sfc-gh-stan sfc-gh-stan Awaiting requested review from sfc-gh-stan

@sfc-gh-yuwang sfc-gh-yuwang Awaiting requested review from sfc-gh-yuwang sfc-gh-yuwang is a code owner automatically assigned from snowflakedb/snowpark-python-api-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfc-gh-jrose @sfc-gh-aling @sfc-gh-aalam