feat: Add `time_zone` to `Datetime` `result`s #33

dfrankland · 2025-05-07T17:09:47Z

Motivation

Closes #32

Changes

allows setting a time zone for datetimes and checking if the values in the column matches it

Pertinent polars docs:
https://docs.pola.rs/user-guide/transformations/time-series/timezones/

allows setting a time zone for datetimes and checking if the values in the column matches it

dfrankland · 2025-05-07T17:10:47Z

tests/column_types/test_datetime.py

+        (
+            dy.Date(),
+            [dt.date(2020, 1, 1), dt.date(2021, 1, 15), dt.date(2022, 12, 1)],
+            {},
+        ),
        (
            dy.Date(resolution="1mo"),
            [dt.date(2020, 1, 1), dt.date(2021, 1, 15), dt.date(2022, 12, 1)],
            {"resolution": [True, False, True]},
        ),
+        (
+            dy.Time(),
+            [dt.time(12, 0), dt.time(13, 15), dt.time(14, 0, 5)],
+            {},
+        ),
        (
            dy.Time(resolution="1h"),
            [dt.time(12, 0), dt.time(13, 15), dt.time(14, 0, 5)],
            {"resolution": [True, False, False]},
        ),
+        (
+            dy.Datetime(),
+            [
+                dt.datetime(2020, 4, 5),
+                dt.datetime(2021, 1, 1, 12),
+                dt.datetime(2022, 7, 10, 0, 0, 1),
+            ],
+            {},
+        ),


I added these since I wanted to double-check the negative case when nothing is specified, but I'm happy to remove them if you believe these are too verbose

I find it a little confusing to have these cases here. The test is named test_validate_resolution after all. I'd propose to remove

codecov · 2025-05-09T09:29:25Z

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.89%. Comparing base (6c955c4) to head (30e3538).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
dataframely/columns/datetime.py	90.90%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main      #33      +/-   ##
===========================================
- Coverage   100.00%   99.89%   -0.11%     
===========================================
  Files           38       38              
  Lines         1845     1852       +7     
===========================================
+ Hits          1845     1850       +5     
- Misses           0        2       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

AndreasAlbertQC

Thanks for the nice PR @dfrankland! Could you add a test in test_sql_schema.py for the oracle part?

dataframely/columns/datetime.py

borchero

Thanks for the initiative @dfrankland, I think this is a very useful addition! :)

dataframely/columns/datetime.py

dataframely/random.py

tests/columns/test_sql_schema.py

dfrankland · 2025-05-09T23:56:23Z

Let me know if you'd like for me to rebase these changes

borchero

All right, nice, I think we're almost there! :)

borchero · 2025-05-10T09:56:35Z

dataframely/columns/datetime.py

-                return sa_mssql.DATETIME2(6)
+                return sa_mssql.DATETIME2(6, timezone=timezone_enabled)
+            case "postgresql":
+                return sa_postgresql.TIMESTAMP(timezone=timezone_enabled)


Do you need this extra condition for postgres or does sa.DateTime(timezone=timezone_enabled) map to TIMESTAMPTZ anyways? We're currently not maintaining 100% coverage (which we should) because we're not using the default case.

borchero · 2025-05-10T09:57:50Z

tests/column_types/test_datetime.py

+        (
+            dy.Date(),
+            [dt.date(2020, 1, 1), dt.date(2021, 1, 15), dt.date(2022, 12, 1)],
+            {},
+        ),
        (
            dy.Date(resolution="1mo"),
            [dt.date(2020, 1, 1), dt.date(2021, 1, 15), dt.date(2022, 12, 1)],
            {"resolution": [True, False, True]},
        ),
+        (
+            dy.Time(),
+            [dt.time(12, 0), dt.time(13, 15), dt.time(14, 0, 5)],
+            {},
+        ),
        (
            dy.Time(resolution="1h"),
            [dt.time(12, 0), dt.time(13, 15), dt.time(14, 0, 5)],
            {"resolution": [True, False, False]},
        ),
+        (
+            dy.Datetime(),
+            [
+                dt.datetime(2020, 4, 5),
+                dt.datetime(2021, 1, 1, 12),
+                dt.datetime(2022, 7, 10, 0, 0, 1),
+            ],
+            {},
+        ),


I find it a little confusing to have these cases here. The test is named test_validate_resolution after all. I'd propose to remove

borchero · 2025-05-10T09:58:43Z

tests/column_types/test_datetime.py

+        ),
+    ],
+)
+def test_dtype_validation(


[nit]

Suggested change

def test_dtype_validation(

def test_dtype_time_zone_validation(

add time_zone to Datetime results

b61a268

allows setting a time zone for datetimes and checking if the values in the column matches it

dfrankland requested review from borchero, AndreasAlbertQC and delsner as code owners May 7, 2025 17:09

dfrankland commented May 7, 2025

View reviewed changes

dfrankland changed the title ~~add time_zone to Datetime results~~ feat: add time_zone to Datetime results May 7, 2025

github-actions bot added the enhancement New feature or request label May 7, 2025

AndreasAlbertQC requested changes May 9, 2025

View reviewed changes

dataframely/columns/datetime.py Outdated Show resolved Hide resolved

dataframely/columns/datetime.py Outdated Show resolved Hide resolved

dfrankland added 2 commits May 9, 2025 12:32

add sql schema tests for datetime with time zone

58e510d

add better documentation of time_zone argument

0796225

dfrankland requested a review from AndreasAlbertQC May 9, 2025 21:46

borchero reviewed May 9, 2025

View reviewed changes

dataframely/columns/datetime.py Outdated Show resolved Hide resolved

dataframely/columns/datetime.py Outdated Show resolved Hide resolved

dataframely/random.py Outdated Show resolved Hide resolved

tests/columns/test_sql_schema.py Outdated Show resolved Hide resolved

use polars default for time_unit

830fff8

borchero changed the title ~~feat: add time_zone to Datetime results~~ feat: Add time_zone to Datetime results May 9, 2025

dfrankland requested a review from borchero May 9, 2025 23:56

dfrankland added 2 commits May 9, 2025 17:28

switch to using dtype validation for datetime timezones

fa62831

backout oracle updates

30e3538

dfrankland force-pushed the add-time_zone-to-datetime-results branch from 8de8c8f to 30e3538 Compare May 10, 2025 00:28

borchero reviewed May 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `time_zone` to `Datetime` `result`s #33

feat: Add `time_zone` to `Datetime` `result`s #33

dfrankland commented May 7, 2025 •

edited

Loading

dfrankland May 7, 2025

borchero May 10, 2025

codecov bot commented May 9, 2025 •

edited

Loading

AndreasAlbertQC left a comment

borchero left a comment

dfrankland commented May 9, 2025

borchero left a comment

borchero May 10, 2025

borchero May 10, 2025

borchero May 10, 2025

	def test_dtype_validation(
	def test_dtype_time_zone_validation(

feat: Add time_zone to Datetime results #33

Are you sure you want to change the base?

feat: Add time_zone to Datetime results #33

Conversation

dfrankland commented May 7, 2025 • edited Loading

Motivation

Changes

dfrankland May 7, 2025

Choose a reason for hiding this comment

borchero May 10, 2025

Choose a reason for hiding this comment

codecov bot commented May 9, 2025 • edited Loading

Codecov Report

AndreasAlbertQC left a comment

Choose a reason for hiding this comment

borchero left a comment

Choose a reason for hiding this comment

dfrankland commented May 9, 2025

borchero left a comment

Choose a reason for hiding this comment

borchero May 10, 2025

Choose a reason for hiding this comment

borchero May 10, 2025

Choose a reason for hiding this comment

borchero May 10, 2025

Choose a reason for hiding this comment

feat: Add `time_zone` to `Datetime` `result`s #33

feat: Add `time_zone` to `Datetime` `result`s #33

dfrankland commented May 7, 2025 •

edited

Loading

codecov bot commented May 9, 2025 •

edited

Loading