feat: add timestamp_to_datetime macro #860
Conversation
📝 WalkthroughWalkthroughA new macro function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/interpolation/macros.py (1)
74-89: Nice implementation! Consider usingdatetime.timezone.utcfor consistency?The function logic is solid and the error handling is appropriate. However, I noticed that
now_utc()(line 32) usesdatetime.timezone.utcwhile this new function usespytz.utc. For consistency and alignment with modern Python standards (wheredatetime.timezone.utcis the preferred built-in), would you consider switching todatetime.timezone.utc? The tests already expectdatetime.timezone.utcin their assertions, though the comparison should work either way.- return datetime.datetime.fromtimestamp(ts_value, tz=pytz.utc) + return datetime.datetime.fromtimestamp(ts_value, tz=datetime.timezone.utc)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/interpolation/macros.py(2 hunks)unit_tests/sources/declarative/interpolation/test_macros.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Check: source-intercom
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (4)
airbyte_cdk/sources/declarative/interpolation/macros.py (1)
243-243: LGTM!The function is correctly registered in the macros list and will be exported properly.
unit_tests/sources/declarative/interpolation/test_macros.py (3)
18-18: LGTM!The new macro is correctly registered in the export test.
180-203: Excellent test coverage!The parametrized tests cover the key input types (int, float, string) and correctly validate the microsecond handling for float inputs. The test structure follows the existing patterns in the file nicely.
206-209: LGTM!Good error handling test that validates the ValueError is raised for invalid inputs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/interpolation/macros.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/interpolation/macros.py (1)
239-254: Macro registration in_macros_listlooks consistentRegistering
timestamp_to_datetimeright aftertimestampin_macros_listkeeps the publicmacrosnamespace coherent and should make this immediately usable from Jinja, which looks good to me. I don’t see any further changes needed here.
| def timestamp_to_datetime(ts: Union[int, float, str]) -> datetime.datetime: | ||
| """ | ||
| Converts a Unix timestamp to a datetime object with UTC timezone. | ||
| Usage: | ||
| "{{ timestamp_to_datetime(1658505815) }}" | ||
| :param ts: Unix timestamp (in seconds) to convert to datetime | ||
| :return: datetime object in UTC timezone | ||
| """ | ||
| try: | ||
| ts_value = float(ts) | ||
| except (TypeError, ValueError) as exc: | ||
| raise ValueError(f"Invalid timestamp value: {ts}") from exc | ||
|
|
||
| return datetime.datetime.fromtimestamp(ts_value, tz=datetime.timezone.utc) | ||
|
|
There was a problem hiding this comment.
Document timestamp_to_datetime behavior for Xero millisecond timestamps; consider exception wrapping
The function explicitly documents "Unix timestamp (in seconds)" and passes float(ts) to datetime.fromtimestamp. However, the Xero C#.NET format /Date(1764104832903+0000)/ contains milliseconds. If a connector author extracts 1764104832903 from Xero data and passes it directly here, they'll get an OverflowError instead of a clear, actionable error message.
Consider one of these approaches:
- Explicit seconds-only: Enhance the docstring and add a clear example showing how to convert Xero milliseconds to seconds before calling this macro (e.g., divide by 1000), or
- Auto-detect milliseconds: Detect and handle millisecond inputs (e.g., by checking if the value exceeds a reasonable seconds threshold and dividing by 1000), though this introduces ambiguity for legitimate large timestamps.
As a follow-up polish (optional), consider wrapping datetime.fromtimestamp's potential OverflowError and OSError in ValueError to provide a consistent exception contract, similar to your float(ts) handling.
🤖 Prompt for AI Agents
airbyte_cdk/sources/declarative/interpolation/macros.py around lines 74-90: The
timestamp_to_datetime docstring and implementation assume seconds but callers
(e.g., Xero) may pass millisecond values causing OverflowError; update the
docstring to clearly state the function expects seconds and give an example
showing dividing milliseconds by 1000, and modify the implementation to either
(preferred) detect millisecond inputs (if ts numeric and > 10**11 treat as ms
and divide by 1000) or (alternative) leave behavior unchanged but add an
explicit check that raises a ValueError with a clear message if the numeric
value is unreasonably large; also wrap datetime.fromtimestamp calls in
try/except to catch OverflowError and OSError and re-raise as ValueError with a
descriptive message including the original value.
1 participant
Adds a new interpolation macro called
timestamp_to_datetimewhich is essentially the reverse of the existingtimestampmacro, taking an integers timestamp and converting it to a datetime object.The motivation for this is working with the Xero API, which uses the C#.NET format for dates (e.g.
/Date(1764104832903+0000)/). Currently we have no way of transforming this to an ISO date in the Connector Builder, which is something we need to do to enable incremental syncing.Would greatly appreciate this feature!
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.