Repository navigation
fix(query): render named-timezone timestamps - #283
Merged
Merged
Conversation
Results are fetched as Arrow IPC and formatted client-side, so a
timestamp arrives as a raw integer plus a timezone recorded in the
schema. Formatting one whose zone is a named IANA zone ("UTC",
"America/New_York") needs a timezone database; arrow was built with
default features off, so none was compiled in and its formatter
returned an error for those values. The cell then reported them as
null.
Only named zones were affected: a fixed offset ("+00:00") and a
zone-less timestamp both format without a timezone database, so a
neighbouring column in the same row looked correct and the loss was
easy to miss. Nothing was wrong with the data — the same query served
inline, formatted by the service, returned the values correctly.
Enable arrow's chrono-tz feature, and stop turning a formatting
failure into a null: the cell has already been checked for null above,
so reaching that arm means a present value could not be rendered, and
reporting it as null asserts the opposite. The output shape is kept
(callers rely on one value per column) and the failure is reported on
stderr instead, once per run so a large result cannot flood it.
Two tests cover it: named zones across several time units, and a
sweep over the temporal and decimal types a query can return
asserting none of them render a present value as null. Both fail
without the feature.
Regressed in 50e3439, which moved result fetching from JSON to Arrow.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Two review points. The warn-once flag was global, so a result with two unformattable column types reported only the first and the second went back to nulling silently — the exact hazard this removes. Key the set on the data type, which is what the failure is a property of. `try_new` only reports a type Arrow cannot format at all. A single out-of-range value inside a formattable type fails later, in the value formatter, and the plain `Display` impl under the default `safe: true` writes the text "ERROR: <msg>" into the cell as if it were data (and panics when the inner write fails). Render the value through `try_to_string` so one bad value degrades the same way a bad type does. Test covers it: an out-of-range Timestamp(Second) must not panic and must not put an error message in the data. Without the change it fails with "ERROR: Cast error: Failed to convert ... to datetime".
Contributor
There was a problem hiding this comment.
Both prior nits are addressed: the warning is now keyed per data type, and the per-value path uses try_to_string instead of Display. No further findings.
Note: CI / test and CI / fmt were still pending at review time, so the new try_to_string call is not confirmed to compile from this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A timestamp column whose type carries a named IANA timezone (
UTC,America/New_York) renders asnullwhen a result is fetched, instead of showing its value. Fixed-offset (+00:00) and zone-less timestamps were unaffected, so a neighbouring column in the same row looked fine.The data was never wrong — only the rendering.
Cause
Results are fetched as Arrow IPC and formatted client-side, so a timestamp arrives as a raw integer plus a timezone recorded in the Arrow schema. Turning that into text for a named zone requires a timezone database.
arrowwas declared withdefault-features = false, so none was compiled in andArrayFormatter::try_newreturned an error for those values:A fixed offset parses arithmetically and a zone-less timestamp needs no lookup, which is why only named zones broke.
Regressed in 50e3439, which moved result fetching from JSON to Arrow — before that the service formatted every value and sent strings, so the client never had to resolve a timezone.
Fix
chrono-tzfeature so named zones resolve.null. The cell is already checked for null above that arm, so reaching it means a present value could not be rendered — reportingnullasserts the opposite and is indistinguishable from a real null to anything reading the output. The shape is kept (callers rely on one value per column) and the failure now goes to stderr, deduplicated once per run so a large result cannot flood the terminal.Verification
Against the live API, fetching the same stored result with the released build and this one:
The client now matches what the service returns when it formats the same query inline.
Tests
named_timezone_timestamps_render_instead_of_nulling— named zones across several time units, plus the fixed-offset and zone-less forms kept as controls so a regression narrows to the named-zone case, and a genuine null still rendering as null.no_column_type_silently_renders_as_null— sweeps the temporal and decimal types a query can return (Date32/64,Time32/64,Duration,Decimal128, all fourTimestampunits in naive,UTC, fixed-offset and named-zone forms) and asserts none renders a present value asnull. This guards the wider hazard rather than just this instance: every type without an explicit arm falls through to the same formatter.Both fail without the feature (
Timestamp(s, UTC) has a value at row 0 but rendered as null) and pass with it.cargo fmt,cargo clippy --all-targets -- -D warningsand the full suite pass.