Skip to content

fix(arrow): validate timestamp timezones#952

Merged
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/timestamp-timezone-validation
Jul 16, 2026
Merged

fix(arrow): validate timestamp timezones#952
zeroshade merged 1 commit into
apache:mainfrom
fallintoplace:fix/timestamp-timezone-validation

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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?

  • Resolve the timestamp timezone before dispatching on the JSON value representation.
  • Propagate timezone lookup failures without appending a value.
  • Keep IPC schema decoding independent of the system timezone database.
  • Preserve compute cast coverage by constructing deliberately invalid timestamp arrays without going through JSON unmarshalling.

Are these changes tested?

Yes. Tests cover timestamp strings, integer strings, json.Number, and float64 values 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.

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 15, 2026 14:21

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread arrow/ipc/metadata.go Outdated
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 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread arrow/array/timestamp.go Outdated
break
}
loc, _ := b.dtype.GetZone()
loc, err := b.dtype.GetZone()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@fallintoplace
fallintoplace force-pushed the fix/timestamp-timezone-validation branch from f956a3a to 3da4011 Compare July 16, 2026 02:58
@fallintoplace
fallintoplace force-pushed the fix/timestamp-timezone-validation branch from 3da4011 to 50e7c75 Compare July 16, 2026 04:16

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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!

@zeroshade
zeroshade merged commit 936ca0b into apache:main Jul 16, 2026
23 checks passed
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.

2 participants