Skip to content

Conversation

@lixiliu
Copy link
Collaborator

@lixiliu lixiliu commented Sep 17, 2025

Addresses dsgrid Issues:

[1] Added TimeZoneConverter classes

  • TimeZoneConverter converts a tz-aware time column to the new time zone by outputting the new time as a tz-naive time column and recording the time_zone info as a new column*
  • TimeZoneConverterByColumn is used to convert from a single time zone to one or more time zones defined by a column in the input data table.
    • Accepts wrap_time_allowed (but not interval_adjustment or time_based_data_adjustment). If true, the converted timestamps aligns with the original timestamps in clock time.

[2] User can invoke two generic functions: convert_time_zone() and convert_time_zone_by_column() without having to call the class functions. Note:

  • TimeZoneConverter is different from mapper in that:
    • It accepts fewer processing arguments (e.g., not accepted: time_based_data_adjustment)
    • it does not accept to_schema arg. Instead other info is passed in (e.g., time_zone_column name) and to_schema is generated based on from_schema and returned to the users.

Future work:

  • Construct a DatetimeRange to DatetimeRangeWithTZColumn mapper by refactoring / leveraging IndexTimeRange to DatetimeRange mapping.
  • Consolidate converter with mapper class now that all converted timestamps can be described by a time config in full.

@lixiliu lixiliu requested a review from daniel-thom September 17, 2025 05:44
@lixiliu lixiliu self-assigned this Sep 17, 2025
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2025

Codecov Report

❌ Patch coverage is 91.63059% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.59%. Comparing base (eaa59e6) to head (15bb2a4).

Files with missing lines Patch % Lines
src/chronify/time_configs.py 69.64% 17 Missing ⚠️
tests/test_store.py 86.84% 10 Missing ⚠️
src/chronify/time_series_checker.py 88.00% 6 Missing ⚠️
src/chronify/models.py 42.85% 4 Missing ⚠️
src/chronify/sqlalchemy/functions.py 20.00% 4 Missing ⚠️
src/chronify/store.py 83.33% 4 Missing ⚠️
src/chronify/time_zone_converter.py 97.20% 4 Missing ⚠️
src/chronify/datetime_range_generator.py 94.82% 3 Missing ⚠️
tests/test_time_zone_converter.py 97.00% 3 Missing ⚠️
src/chronify/annual_time_range_generator.py 66.66% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
+ Coverage   91.43%   91.59%   +0.16%     
==========================================
  Files          44       47       +3     
  Lines        3466     4032     +566     
==========================================
+ Hits         3169     3693     +524     
- Misses        297      339      +42     

☔ 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.

@lixiliu lixiliu mentioned this pull request Oct 18, 2025
4 tasks
@daniel-thom daniel-thom requested a review from Copilot October 30, 2025 23:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds time zone conversion functionality to chronify, enabling conversion of time series data from one time zone to another. The implementation includes support for single time zone conversions and per-column time zone conversions.

Key changes:

  • Introduces TimeZoneConverter and TimeZoneConverterByColumn classes for time zone conversion operations
  • Adds DatetimeRangeWithTZColumn configuration to support external time zone columns
  • Renames IndexTimeRangeLocalTime to IndexTimeRangeWithTZColumn and INDEX_LOCAL to INDEX_TZ_COL for consistency
  • Refactors time range generator classes to separate public and private timestamp iteration methods

Reviewed Changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/chronify/time_zone_converter.py New module implementing time zone conversion with two converter classes
src/chronify/time_configs.py Adds DatetimeRangeWithTZColumn config and refactors time config hierarchy
src/chronify/datetime_range_generator.py Refactors to support external time zone columns with new generator class
src/chronify/time_utils.py Adds helper functions for time zone checking and name extraction
src/chronify/time_series_checker.py Enhances timestamp validation to support external time zone columns
src/chronify/store.py Adds convert_time_zone and convert_time_zone_by_column public API methods
tests/test_time_zone_converter.py New comprehensive test suite for time zone conversion functionality
Multiple test files Updates test functions to include return type annotations and use tzinfo type

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

raise InvalidValue(msg)
return time_zones

# Lixi TODO: ensure table schema has time_zone col?
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO comment should either be addressed or converted into a proper issue tracker item with more context about what needs to be done.

Suggested change
# Lixi TODO: ensure table schema has time_zone col?
# TODO(issue #123): Ensure table schema has the column specified by time_zone_column.

Copilot uses AI. Check for mistakes.
return self.time_zone_column

def get_time_zones(self) -> list[tzinfo | None]:
return [] # LIXI TODO
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO comment should either be addressed or include more context about what needs to be implemented for get_time_zones() in RepresentativePeriodTimeTZ.

Suggested change
return [] # LIXI TODO
# TODO: Implement logic to return the list of time zones represented by this dimension.
# This may require accessing the unique values from the `time_zone_column` in the underlying data,
# or otherwise determining which time zones are present. If this cannot be implemented here,
# document why and how it should be handled by the caller or in a subclass.
return []

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI commented Oct 31, 2025

@daniel-thom I've opened a new pull request, #58, to work on those changes. Once the pull request is ready, I'll request review from you.

lixiliu and others added 3 commits October 30, 2025 22:00
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mypy

mypy attempt 3

fix mypy error again
@lixiliu lixiliu requested a review from Copilot November 7, 2025 16:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 12 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lixiliu and others added 2 commits November 7, 2025 10:17
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@daniel-thom
Copy link
Collaborator

@lixiliu Reminder to squash and merge when ready.

@lixiliu lixiliu merged commit d7afd47 into main Nov 24, 2025
4 checks passed
@lixiliu lixiliu deleted the ll/to_local_time branch November 24, 2025 21:26
github-actions bot pushed a commit that referenced this pull request Nov 24, 2025
* initial commit

* clean up

* Add time_wrap feature for tz col conversion

* Rename IndexTimeRangeLocalTime to IndexTimeRangeWithTZColumn

* Rename INDEX_LOCAL to INDEX_TZ_COL

* Rework time_zone_converter to output tz naive col

* Common API

* Revise DatetimeRangeWithTZColumn class and downstream funcs

* keep time_zone in output for convert by col

* some cleanup

* mostly changing from ZoneInfo to tzinfo as typehint

* fix mypy errors

* more mypy stuff

* Add pytest for time_utils

* refactor time util funcs

* Fix pytest issue

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* fix mypy!

mypy

mypy attempt 3

fix mypy error again

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Apply code review suggestions 2

* review suggestions

* address comments v3

* potential parquet fix for spark hive

* update sql functions

* Show error

* temp

* final

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants