Skip to content

fix(arrow/scalar): align timestamp timezone parsing - #1109

Open
fallintoplace wants to merge 11 commits into
apache:mainfrom
fallintoplace:fix/scalar-parse-timestamp-timezone
Open

fix(arrow/scalar): align timestamp timezone parsing#1109
fallintoplace wants to merge 11 commits into
apache:mainfrom
fallintoplace:fix/scalar-parse-timestamp-timezone

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

MakeScalarParam, ParseScalar, timestamp builders, JSON decoding, and typed CSV parsing should use the same timestamp timezone rules. This also adds support for Arrow fixed-offset timezone strings.

What changes are included in this PR?

  • Validate timestamp timezone metadata through TimestampType.GetZone.
  • Accept UTC metadata case-insensitively, including forms such as Utc.
  • Require the input to contain an offset exactly when the timestamp type has a non-empty timezone.
  • Keep timezone-naive values offset-free.
  • Preserve the instant represented by an explicit input offset.
  • Keep numeric timestamp values as raw epoch values after validating the type timezone.
  • Validate CSV timestamp metadata before reading rows, including null-only columns.
  • Keep timezone offsets in CSV output for timezone-aware timestamps so CSV writer and reader round trips work.
  • Preserve an explicitly supplied empty custom timestamp layout.

Are these changes tested?

  • go test ./arrow ./arrow/scalar ./arrow/array ./arrow/compute -count=1
  • go test ./arrow/csv -run Timestamp -count=1
  • go test -race ./arrow ./arrow/scalar ./arrow/array ./arrow/compute ./arrow/csv -run Timestamp -count=1

Are there any user-facing changes?

Timestamp string parsing now rejects mismatched timezone presence:

  • timezone-less type plus no offset: accepted
  • timezone-less type plus an offset: rejected
  • timezone-aware type plus an offset: accepted
  • timezone-aware type without an offset: rejected

Explicit offsets still determine the represented instant, and mixed-case UTC metadata remains accepted. CSV output now includes the offset for timezone-aware timestamps.

@fallintoplace fallintoplace changed the title fix(arrow/scalar): honor timestamp timezones in ParseScalar fix(arrow/scalar): honor timezones in ParseScalar Aug 5, 2026
@fallintoplace fallintoplace changed the title fix(arrow/scalar): honor timezones in ParseScalar fix(arrow/scalar): parse zoned timestamps consistently Aug 6, 2026
@fallintoplace
fallintoplace marked this pull request as draft August 7, 2026 10:16
@fallintoplace fallintoplace changed the title fix(arrow/scalar): parse zoned timestamps consistently fix(arrow/scalar): align timestamp timezone parsing Aug 7, 2026
@fallintoplace
fallintoplace marked this pull request as ready for review August 12, 2026 17:57
@fallintoplace
fallintoplace marked this pull request as draft August 12, 2026 17:58
@fallintoplace
fallintoplace marked this pull request as ready for review August 12, 2026 18:33
@zeroshade

Copy link
Copy Markdown
Member

@fallintoplace need to fix the failing tests

@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.

Thanks for aligning timestamp parsing across the scalar, array, JSON, and CSV paths. The core timezone-presence rule and fixed-offset handling look consistent, and CI is green.

I found three regressions that need addressing before this lands:

  1. Invalid timestamp metadata can make CSV converters append no value, causing panics in fixed-size lists and multi-column inferred readers.
  2. CSV inference classifies zoned ISO-8601 values as timezone-less timestamps and then rejects them during conversion.
  3. Zoned timestamp scalars no longer round-trip through their own String() output and ParseScalar.

I included minimal reproducers inline. The affected core packages and focused CSV timestamp tests otherwise pass locally.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the points above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.

Comment thread arrow/csv/reader.go
if r.err == nil {
r.err = err
}
return func(string) {}

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.

Blocking: returning a no-op converter after setting r.err violates the CSV reader's one-input/one-builder-value invariant.

I reproduced two new panics:

  • fixed_size_list<timestamp[s, tz=not/a_timezone]> panics when constructing the array because the parent appends one list while this converter appends zero child values.
  • NewInferringReader with an invalid timestamp supplied through WithColumnTypes can panic because this column has zero rows while another column has one.

A regular list similarly produces an empty list rather than a null. Also, because this validation is lazy, an invalid timestamp nested in an all-null list is never validated at all.

Please validate timestamp metadata recursively and ensure an error-path converter still appends a null value.

Comment thread arrow/csv/reader.go
}

v, err := arrow.TimestampFromString(str, unit)
err := field.(*array.TimestampBuilder).AppendValueFromString(str)

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.

Blocking: this stricter parser is inconsistent with CSV type inference. tryParse still uses arrow.TimestampFromString, so 2024-01-01T00:00:00Z is inferred as timezone-less timestamp[s]; this call then rejects the same value because it contains an offset.

Reproducer:

r := csv.NewInferringReader(strings.NewReader("2024-01-01T00:00:00Z\n"))
r.Next()
// r.Err(): timestamp value ... for type timestamp[s] must not include a zone offset

This succeeds on the merge base. Inference should apply the same zone-presence rule so zoned input advances to the existing timestamp[*, UTC] inference rung.

Comment thread arrow/scalar/parse.go
return 0, err
}

if zonePresent != (dt.TimeZone != "") {

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.

Requiring an offset for zoned timestamp types makes the package's own scalar string representation non-parseable. scalar.Timestamp.String() still emits no offset:

typ := &arrow.TimestampType{Unit: arrow.Second, TimeZone: "UTC"}
s := scalar.NewTimestampScalar(0, typ)
scalar.ParseScalar(typ, s.String())
// must include a zone offset

This round-trip succeeds on the merge base. Please update timestamp scalar formatting to include the appropriate offset for zoned types, analogous to the array and CSV formatting changes in this PR.

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