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

pl.Datetime time_zone parameter has no type or value check #16038

Open
2 tasks done
mcrumiller opened this issue May 3, 2024 · 3 comments · May be fixed by #16165
Open
2 tasks done

pl.Datetime time_zone parameter has no type or value check #16038

mcrumiller opened this issue May 3, 2024 · 3 comments · May be fixed by #16165
Labels
A-timeseries Area: date/time functionality bug Something isn't working P-low Priority: low python Related to Python Polars

Comments

@mcrumiller
Copy link
Contributor

mcrumiller commented May 3, 2024 •
edited

Checks

  • I have checked that this issue has not already been reported.
  • I have confirmed this bug exists on the latest version of Polars.

Issue Description

The time zone parameter in pl.Datetime can be an invalid string, and can even be any type of Python object. We should probably ensure that the time zone is a valid time zone when constructing a pl.Datetime datatype.

import polars as pl

pl.Datetime(time_zone="invalid_time_zone")
# Datetime(time_unit='us', time_zone='invalid_time_zone')

pl.Datetime(time_zone=object())
# Datetime(time_unit='us', time_zone=<object object at 0x7f245331d0d0>)

Installed versions

main
@mcrumiller mcrumiller added bug Something isn't working needs triage Awaiting prioritization by a maintainer python Related to Python Polars labels May 3, 2024
@mcrumiller mcrumiller changed the title pl.Datetime accepts invalid time zone strings pl.Datetime time_zone parameter has no type or value check May 3, 2024
@MarcoGorelli
Copy link
Collaborator

thanks for reporting - the validation currently happens later:

In [9]: pl.Series([datetime(2020, 1, 1)], dtype=pl.Datetime(time_zone='cabbage'))
---------------------------------------------------------------------------
ComputeError                              Traceback (most recent call last)
Cell In[9], line 1
----> 1 pl.Series([datetime(2020, 1, 1)], dtype=pl.Datetime(time_zone='cabbage'))

File ~/scratch/.venv/lib/python3.11/site-packages/polars/series/series.py:312, in Series.__init__(self, name, values, dtype, strict, nan_to_null, dtype_if_empty)
    309         raise TypeError(msg)
    311 if isinstance(values, Sequence):
--> 312     self._s = sequence_to_pyseries(
    313         name,
    314         values,
    315         dtype=dtype,
    316         strict=strict,
    317         nan_to_null=nan_to_null,
    318     )
    320 elif values is None:
    321     self._s = sequence_to_pyseries(name, [], dtype=dtype)

File ~/scratch/.venv/lib/python3.11/site-packages/polars/_utils/construction/series.py:235, in sequence_to_pyseries(name, values, dtype, strict, nan_to_null)
    225         if values_tz != "UTC" and dtype_tz is None:
    226             warnings.warn(
    227                 "Constructing a Series with time-zone-aware "
    228                 "datetimes results in a Series with UTC time zone. "
   (...)
    233                 stacklevel=find_stacklevel(),
    234             )
--> 235         return s.dt.replace_time_zone(dtype_tz or "UTC")._s
    236     return s._s
    238 elif (
    239     _check_for_numpy(value)
    240     and isinstance(value, np.ndarray)
    241     and len(value.shape) == 1
    242 ):

File ~/scratch/.venv/lib/python3.11/site-packages/polars/series/utils.py:107, in call_expr.<locals>.wrapper(self, *args, **kwargs)
    105     expr = getattr(expr, namespace)
    106 f = getattr(expr, func.__name__)
--> 107 return s.to_frame().select_seq(f(*args, **kwargs)).to_series()

File ~/scratch/.venv/lib/python3.11/site-packages/polars/dataframe/frame.py:7906, in DataFrame.select_seq(self, *exprs, **named_exprs)
   7883 def select_seq(
   7884     self, *exprs: IntoExpr | Iterable[IntoExpr], **named_exprs: IntoExpr
   7885 ) -> DataFrame:
   7886     """
   7887     Select columns from this DataFrame.
   7888
   (...)
   7904     select
   7905     """
-> 7906     return self.lazy().select_seq(*exprs, **named_exprs).collect(_eager=True)

File ~/scratch/.venv/lib/python3.11/site-packages/polars/lazyframe/frame.py:1810, in LazyFrame.collect(self, type_coercion, predicate_pushdown, projection_pushdown, simplify_expression, slice_pushdown, comm_subplan_elim, comm_subexpr_elim, no_optimization, streaming, background, _eager)
   1807 if background:
   1808     return InProcessQuery(ldf.collect_concurrently())
-> 1810 return wrap_df(ldf.collect())

ComputeError: unable to parse time zone: 'cabbage'. Please check the Time Zone Database for a list of available time zones

@MarcoGorelli MarcoGorelli added P-low Priority: low A-timeseries Area: date/time functionality and removed needs triage Awaiting prioritization by a maintainer labels May 3, 2024
@luke396
Copy link
Contributor

luke396 commented May 7, 2024

Hi @MarcoGorelli, should we add some checks here

polars/py-polars/polars/datatypes/classes.py

Lines 461 to 463 in 2970c57

if isinstance(time_zone, timezone):
time_zone = str(time_zone)

Similar to what's done here

polars/py-polars/polars/_utils/convert.py

Lines 163 to 194 in 2970c57

if time_zone is None:
return EPOCH + td
elif _ZONEINFO_AVAILABLE:
dt = EPOCH_UTC + td
return _localize_datetime(dt, time_zone)
else:
msg = "install polars[timezone] to handle datetimes with time zone information"
raise ImportError(msg)
def _localize_datetime(dt: datetime, time_zone: str) -> datetime:
# zone info installation should already be checked
try:
tz = string_to_zoneinfo(time_zone)
except zoneinfo.ZoneInfoNotFoundError:
# try fixed offset, which is not supported by ZoneInfo
tz = _parse_fixed_tz_offset(time_zone)
return dt.astimezone(tz)
@no_type_check
@lru_cache(None)
def string_to_zoneinfo(key: str) -> Any:
"""
Convert a time zone string to a Python ZoneInfo object.
This is a simple wrapper for the zoneinfo.ZoneInfo constructor.
The wrapper is useful because zoneinfo is not available on Python 3.8
and the backports module may not be installed.
"""
return zoneinfo.ZoneInfo(key)

Or should we leave the error as it is? Do you have any thoughts on this? If possible, I'd like to improve it.

@luke396 luke396 linked a pull request May 11, 2024 that will close this issue
Draft
@max-muoto
Copy link
Contributor

max-muoto commented May 13, 2024 •
edited

Using Pyright/MyPy, you'll get a static type-checking warning if you try and pass in object() to time_zone here. The only way this could perhaps be built on (from a type-checking perspective), is maintaining a literal of all possible timezones, and using that instead of str.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-timeseries Area: date/time functionality bug Something isn't working P-low Priority: low python Related to Python Polars
Projects
Backlog
Status: Ready
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(python): Validate time_zone in pl.Datetime luke396/polars
4 participants
@mcrumiller @MarcoGorelli @max-muoto @luke396