Conversation
This reverts commit e15b0d4.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
==========================================
+ Coverage 76.90% 77.55% +0.64%
==========================================
Files 137 137
Lines 14784 14982 +198
==========================================
+ Hits 11370 11619 +249
+ Misses 3414 3363 -51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds local time support for tz-naive timestamps by introducing Chronify-based localization during dataset registration, along with new/updated time-zone configuration semantics and tests.
Changes:
- Adds Chronify-powered timestamp localization helpers and wires them into dataset registration.
- Updates time zone format modeling (rename to
aligned_in_local_std_time, allowtime_zone=Nonefor aligned absolute time). - Adds tests for localization routing and UTC-offset parsing in time-in-parts conversion.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_localize_timestamps_if_necessary.py | New tests validating localization routing and no-op behavior. |
| tests/test_create_time_dimensions.py | Minor test fix for periods argument type. |
| tests/test_convert_time_format_if_necessary.py | New tests for UTC offset parsing and timestamp transformation behavior. |
| tests/data/dimension_models/minimal/dimension_test_time.json5 | Updates time zone format string to new enum value. |
| pyproject.toml | Switches Chronify dependency to a Git URL/branch for local-time work. |
| missing_associations/geography__subsector.csv | Adds missing associations CSV fixture. |
| dsgrid/utils/dataset.py | Adds Chronify localization helpers and localize_timestamps_if_necessary. |
| dsgrid/registry/dataset_registry_manager.py | Integrates time-in-parts conversion and timestamp localization into registration. |
| dsgrid/query/query_submitter.py | Uses TIME_ZONE_COLUMN consistently; refactors Chronify conversion routing. |
| dsgrid/dimension/time.py | Renames time zone enum value to aligned_in_local_std_time. |
| dsgrid/dataset/dataset_schema_handler_base.py | Adds Chronify-based timestamp localization pathway at schema-handler level. |
| dsgrid/config/index_time_dimension_config.py | Updates Chronify return type hints for index time config. |
| dsgrid/config/dimensions.py | Updates time config models (offset column, time zone formats, removes legacy model). |
| dsgrid/config/date_time_dimension_config.py | Adds Chronify dtype support + localization plan logic. |
| dsgrid/common.py | Adds TIME_COLUMN constant. |
| dsgrid-test-data | Updates submodule pointer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # update time_dim | ||
| time_column = time_dim.model.column_format.time_column | ||
| time_dim.model.column_format = TimeFormatDateTimeTZModel(time_column=time_column) | ||
|
|
There was a problem hiding this comment.
After localization, the code updates the file path and the time dimension dtype, but it does not update config.model.data_layout.data_file.columns (when present) to reflect TIMESTAMP_TZ. This can leave the dataset config’s declared schema inconsistent with the written parquet. Consider mirroring _update_config_for_timestamp behavior here: replace the time column entry with data_type='TIMESTAMP_TZ' when columns is not None.
| # update data_file columns schema, if present, to reflect TIMESTAMP_TZ | |
| data_file = config.model.data_layout.data_file | |
| if data_file.columns is not None: | |
| updated_columns: list[Column] = [] | |
| for col in data_file.columns: | |
| if col.name == time_column: | |
| updated_columns.append(Column(name=time_column, data_type="TIMESTAMP_TZ")) | |
| else: | |
| updated_columns.append(col) | |
| data_file.columns = updated_columns |
| # "boto3", | ||
| # "s3path", | ||
| "chronify ~= 0.6.0", | ||
| "chronify@git+https://github.com/NatLabRockies/chronify.git@ll/local_time2", # "chronify ~= 0.6.0", |
There was a problem hiding this comment.
Using a Git URL pinned to a branch name makes builds non-reproducible (branch heads can move) and can break dependency resolution for consumers. Prefer pinning to an immutable commit SHA or (ideally) a released Chronify version, and keep VCS overrides behind a dev-only extra if needed.
| "chronify@git+https://github.com/NatLabRockies/chronify.git@ll/local_time2", # "chronify ~= 0.6.0", | |
| "chronify ~= 0.6.0", |
Closes GitHub Issue #
Description
Local time support.
Allows config to describe tz-naive timestamps that are:
aligned_in_absolute_time(with or without time zone) oraligned_in_local_std_time(where time zones are given by a TIME_ZONE_COLUMN in the data table)Time zone localization is triggered during dataset registration when:
aligned_in_local_std_time, localization is based on the TIME_ZONE_COLUMN from the geography dimension.No time zone localization available when dataset is submitted to project
Chronify: NatLabRockies/chronify#61
Checklist