SNOW-3746497 Snowpark Python SDK: add interval type support for UDFs & Sprocs - #4291
SNOW-3746497 Snowpark Python SDK: add interval type support for UDFs & Sprocs#4291sfc-gh-jzhan wants to merge 3 commits into
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4291 +/- ##
===========================================
- Coverage 95.46% 82.53% -12.93%
===========================================
Files 171 171
Lines 44720 44742 +22
Branches 7676 7679 +3
===========================================
- Hits 42694 36930 -5764
- Misses 1253 5842 +4589
- Partials 773 1970 +1197 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if isinstance(value, str) and isinstance(datatype, DayTimeIntervalType): | ||
| return f"{str_to_sql_for_day_time_interval(value, datatype)} :: {convert_sp_to_sf_type(datatype)}" | ||
|
|
||
| if isinstance(value, timedelta) and isinstance(datatype, DayTimeIntervalType): |
There was a problem hiding this comment.
Can we unit test this conversion to SQL string? The math is pretty complicated so some tests would be ideal
| assert result[0][0] == datetime.timedelta(days=-3) | ||
|
|
||
|
|
||
| def test_udf_yearmonth_interval(session): |
There was a problem hiding this comment.
We can also add a test for negative yearmonth
| def test_sproc_yearmonth_interval(session): | ||
| """YearMonthIntervalType sproc: int (total months) arg/return round-trips correctly.""" | ||
| if not _interval_udf_flag_enabled(session): | ||
| pytest.skip("ENABLE_INTERVAL_TYPES_IN_UDF not enabled on this account") |
There was a problem hiding this comment.
Because of the skip, the test passes if the feature isn't enabled so we need to be careful about syncing the parameter rollout and client release. Otherwise, we might merge the change with tests skipped, then turn on the parameter and have untested code in the release.
Imo we should keep this PR open until the parameter is turned on, or turn on the parameter in this client's test account, remove the skip, and merge after testing
| total_us %= 60_000_000 | ||
| s = total_us // 1_000_000 | ||
| us = total_us % 1_000_000 | ||
| interval_str = f"{sign}{d} {h:02d}:{m:02d}:{s:02d}.{us:06d}" |
There was a problem hiding this comment.
can you check if we can reuse format_day_time_interval() or format_day_time_interval_for_display() in type_utils.py?
another question I have is do we need :: {convert_sp_to_sf_type(datatype)} like we used when value is a string?
| abs_months = abs(value) | ||
| years = abs_months // 12 | ||
| months = abs_months % 12 | ||
| interval_str = f"{sign}{years}-{months:02d}" |
There was a problem hiding this comment.
similar here, is it possible we reuse format_year_month_interval_for_display() ?
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-3746497
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Enables
DayTimeIntervalTypeandYearMonthIntervalTypeas parameter and return types for Python UDFs and stored procedures, gated behind theENABLE_INTERVAL_TYPES_IN_UDFaccount parameter.New user-facing behavior:
DayTimeIntervalType—datetime.timedeltais now a recognized annotation:SDK changes: