feat(cubesql): Support parsing extra timezone formats#10913
Conversation
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
|
Claude finished @MazterQyou's task in 1m 15s —— View job Code Review: PR #10913
Overall: This PR looks good. It's a clean, minimal change — a dependency bump plus targeted tests for the new functionality. A few observations below. Dependency ChangesThe PR bumps the No new dependencies are introduced; the only change is the revision pins. This is low-risk from a supply-chain perspective since the forks are under Tests (
|
| Format | Example | Snapshot |
|---|---|---|
±HH |
+00 |
test_cast_to_timestamp_with_tz_offset_hh |
±HH:MM |
+00:00 |
test_cast_to_timestamp_with_tz_offset_hh_mm |
±HH:MM:SS |
+00:00:00 |
test_cast_to_timestamp_with_tz_offset_hh_mm_ss |
The test follows the established pattern (using execute_query + insta::assert_snapshot!) and is placed logically next to the existing test_cast_to_timestamp_timezone_utc test.
Suggestions (minor, non-blocking)
-
Test only uses
+00offsets. All three test cases use a zero UTC offset (+00,+00:00,+00:00:00), which means they all produce the identical output2026-01-26T00:00:00.000. This doesn't fully exercise that the offset is correctly applied to the timestamp — a positive or negative offset (e.g.,+05:30or-08:00) would make the tests more meaningful by verifying the actual time conversion, not just the parsing. Consider adding at least one case with a non-zero offset to confirm the arithmetic is correct. -
Negative offset not tested. The PR description mentions
±HHformat support, but only+is tested. A-05or similar case would cover the negative path.
These are suggestions for better test coverage, not blockers — the parsing itself is proven to work by the fact that the tests pass at all (they would error pre-bump).
Snapshots
All three snapshot files are correctly structured and show the expected Timestamp(Nanosecond, None) output type. The results are consistent.
Security / Performance
No concerns. This is a dependency pin bump — no new code paths in the main crate, no user-facing API changes, no new unsafe blocks. The parsing improvement lives in the upstream fork.
Summary: Clean PR. The dependency bump is consistent across both lockfiles, the tests follow repo conventions and verify the new parsing capability. The one substantive suggestion is to add a non-zero offset test case to verify offset arithmetic, not just successful parsing.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10913 +/- ##
=======================================
Coverage 78.93% 78.94%
=======================================
Files 470 470
Lines 92837 92862 +25
Branches 3449 3449
=======================================
+ Hits 73283 73308 +25
Misses 19050 19050
Partials 504 504
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Check List
Description of Changes Made
This PR bumps cube-js/arrow-datafusion@9f49caa, allowing to parse extra timezone formats (such as
±HHand±HH:MM:SS) for timestamps. Related test is included.Fixes #10405