Time zone localization #61
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces time zone localization functionality to handle conversion of timezone-naive (TIMESTAMP_NTZ) timestamps to timezone-aware (TIMESTAMP_TZ) timestamps. The changes include new localizer classes, refactoring of time configuration models, updates to test utilities, and bug fixes.
Changes:
- Added
TimeZoneLocalizerandTimeZoneLocalizerByColumnclasses for localizing tz-naive timestamps to standard time zones - Refactored time configuration models to include
dtypefield and consolidatedIndexTimeRangeclasses - Updated test utilities and fixed API inconsistencies (
np.concat→np.concatenate)
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/chronify/time_zone_localizer.py | New module implementing time zone localization classes and functions |
| src/chronify/time_configs.py | Added dtype field to DatetimeRange classes and consolidated IndexTimeRange classes |
| src/chronify/time.py | Replaced DatetimeFormat with TimeDataType enum |
| src/chronify/store.py | Added localize_time_zone and localize_time_zone_by_column methods |
| src/chronify/time_zone_converter.py | Updated error messages and added dtype field usage |
| src/chronify/datetime_range_generator.py | Refactored timestamp generation to use pd.date_range and improved handling of time zones |
| src/chronify/sqlalchemy/functions.py | Updated to use dtype field instead of start_time_is_tz_naive() |
| src/chronify/time_series_mapper_index_time.py | Added dtype field assignment in mapping creation |
| src/chronify/time_series_checker.py | Fixed typo in docstring |
| tests/test_time_zone_localizer.py | Comprehensive test coverage for new localization functionality |
| tests/test_store.py | Integration tests for localize_time_zone methods |
| tests/test_time_zone_converter.py | Updated error message test and refactored test utilities |
| tests/test_mapper_*.py | Refactored to use list_timestamps() instead of _iter_timestamps() |
| tests/test_mapper_column_representative_to_datetime.py | Fixed np.concat → np.concatenate |
| src/chronify/init.py | Updated exports to remove deprecated IndexTimeRange classes |
| docs/how_tos/map_time_config.md | Fixed np.concat → np.concatenate in documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from_schema = get_datetime_schema(2018, None, TimeIntervalType.PERIOD_BEGINNING, "base_table") | ||
| df = generate_datetime_dataframe(from_schema) | ||
| error = ( | ||
| Exception, |
There was a problem hiding this comment.
The error type should be MissingValue instead of generic Exception. The actual implementation in localize_time_zone_by_column raises MissingValue at line 120 of src/chronify/time_zone_localizer.py, but this test expects a generic Exception.
| and time_zone_column is not None | ||
| ): | ||
| msg = f"Input {time_zone_column=} will be ignored. time_zone_column is already defined in the time_config." | ||
| raise Warning(msg) |
There was a problem hiding this comment.
Raising Warning is incorrect. In Python, Warning is not an exception type meant to be raised. This should either use warnings.warn() to emit a warning, or raise a proper exception like InvalidParameter if this is an error condition.
| dtype = TimeDataType.TIMESTAMP_NTZ | ||
| case (False, None): | ||
| dtype = TimeDataType.TIMESTAMP_TZ | ||
| # validate dype if provided |
There was a problem hiding this comment.
Corrected spelling of 'dype' to 'dtype'.
| # validate dype if provided | |
| # validate dtype if provided |
| # if timestamps[0].tzinfo: | ||
| # timestamps = [x.astimezone(tz_name).replace(tzinfo=None) for x in timestamps] |
There was a problem hiding this comment.
This comment appears to contain commented-out code.
| # if timestamps[0].tzinfo: | |
| # timestamps = [x.astimezone(tz_name).replace(tzinfo=None) for x in timestamps] |
| # if time_zone_column not in from_schema.time_array_id_columns: | ||
| # msg = f"{time_zone_column=} must be in source schema time_array_id_columns." |
There was a problem hiding this comment.
This comment appears to contain commented-out code.
| # if time_zone_column not in from_schema.time_array_id_columns: | |
| # msg = f"{time_zone_column=} must be in source schema time_array_id_columns." | |
| # Note: time_zone_column is expected to be in from_schema.time_array_id_columns. |
No description provided.