fix(arrow): validate timestamp timezones#952
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
Validating timezones is a good idea, and empty tz + fixed offsets are handled correctly via GetZone. Two issues before merge: enforcing GetZone during IPC schema decode can false-reject valid IANA names on systems without tzdata, and the JSON builder only validates some value representations. Detail inline.
| tz := string(data.Timezone()) | ||
| return &arrow.TimestampType{Unit: unit, TimeZone: tz}, nil | ||
| typ := &arrow.TimestampType{Unit: unit, TimeZone: tz} | ||
| if _, err := typ.GetZone(); err != nil { |
There was a problem hiding this comment.
GetZone() resolves IANA names via time.LoadLocation, which needs the system/embedded tz database. Gating IPC schema decode on it means a valid schema with tz=America/New_York is rejected on a minimal/scratch/container runtime that lacks tzdata (and doesn't import the embedded time/tzdata) — a regression, since decode previously carried the tz string through untouched. Empty tz and fixed offsets like +09:00 are unaffected (they skip LoadLocation). Consider not failing decode on IANA resolution here, or documenting that IPC reads now require tzdata.
| break | ||
| } | ||
| loc, _ := b.dtype.GetZone() | ||
| loc, err := b.dtype.GetZone() |
There was a problem hiding this comment.
This validates the tz only on the non-integer string path. Numeric JSON still bypasses it — the case json.Number: and case float64: branches below just append without calling GetZone(). So a builder whose type has an invalid TimeZone still accepts numeric JSON like [0] without error. If this boundary is meant to reject invalid timezones, validate once up front regardless of the value representation.
f956a3a to
3da4011
Compare
3da4011 to
50e7c75
Compare
zeroshade
left a comment
There was a problem hiding this comment.
Re-review: IPC schema decode is now tzdata-independent (dropped the GetZone gate) and JSON timezone validation is applied upfront across all value types (string/number/float), with a 4-path test. LGTM — thanks!
Rationale for this change
TimestampBuilder JSON unmarshalling can silently treat invalid timezone metadata as UTC for timestamp strings, while numeric representations bypass timezone validation entirely.
Refs #951
What changes are included in this PR?
Are these changes tested?
Yes. Tests cover timestamp strings, integer strings,
json.Number, andfloat64values with invalid timezone metadata. The arrow/array, arrow/ipc, and arrow/compute package tests, including race tests, pass.Are there any user-facing changes?
TimestampBuilder JSON unmarshalling now returns an error for invalid timezone names regardless of the JSON value representation. IPC schema decoding remains unchanged.