fix(arrow/scalar): align timestamp timezone parsing - #1109
fix(arrow/scalar): align timestamp timezone parsing#1109fallintoplace wants to merge 15 commits into
Conversation
|
@fallintoplace need to fix the failing tests |
zeroshade
left a comment
There was a problem hiding this comment.
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:
- Invalid timestamp metadata can make CSV converters append no value, causing panics in fixed-size lists and multi-column inferred readers.
- CSV inference classifies zoned ISO-8601 values as timezone-less timestamps and then rejects them during conversion.
- Zoned timestamp scalars no longer round-trip through their own
String()output andParseScalar.
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.
| if r.err == nil { | ||
| r.err = err | ||
| } | ||
| return func(string) {} |
There was a problem hiding this comment.
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.NewInferringReaderwith an invalid timestamp supplied throughWithColumnTypescan 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.
| } | ||
|
|
||
| v, err := arrow.TimestampFromString(str, unit) | ||
| err := field.(*array.TimestampBuilder).AppendValueFromString(str) |
There was a problem hiding this comment.
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 offsetThis succeeds on the merge base. Inference should apply the same zone-presence rule so zoned input advances to the existing timestamp[*, UTC] inference rung.
| return 0, err | ||
| } | ||
|
|
||
| if zonePresent != (dt.TimeZone != "") { |
There was a problem hiding this comment.
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 offsetThis 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.
zeroshade
left a comment
There was a problem hiding this comment.
The three issues from my previous review are addressed: nested timestamp metadata is validated without violating builder lengths, CSV inference handles zoned timestamps consistently, and ordinary UTC/fixed-offset scalar strings round-trip.
Two follow-ups still block merging: historical named-zone offsets can lose seconds during String()/ParseScalar round-trips, and the timestamp expression expectation currently fails CI. Details are inline.
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.
| if err != nil { | ||
| return "..." | ||
| } | ||
| return toTime(s.Value).Format(timestampScalarLayout + "Z0700") |
There was a problem hiding this comment.
Z0700 loses seconds from historical IANA timezone offsets, so this still does not universally round-trip. For Europe/Berlin at year 0000, String() emits 0000-01-01 00:53:28+0053; parsing it changes the timestamp by 28 seconds. Please preserve second-granularity offsets and add a historical named-zone regression test.
|
|
||
| func TestExpressionToString(t *testing.T) { | ||
| ts, _ := scalar.MakeScalar("1990-10-23 10:23:33.123456").CastTo(arrow.FixedWidthTypes.Timestamp_ns) | ||
| ts, _ := scalar.MakeScalar("1990-10-23 10:23:33.123456Z").CastTo(arrow.FixedWidthTypes.Timestamp_ns) |
There was a problem hiding this comment.
This fixture now includes a timezone, so the scalar renders with Z, but the expected value below remains timezone-less. Current CI reports expected 1990-10-23 10:23:33.123456, actual 1990-10-23 10:23:33.123456Z. Please update the expectation so the test suite passes.
cfdf2bd to
4235a61
Compare
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?
TimestampType.GetZone.Utc.Are these changes tested?
go test ./arrow ./arrow/scalar ./arrow/array ./arrow/compute -count=1go test ./arrow/csv -run Timestamp -count=1go test -race ./arrow ./arrow/scalar ./arrow/array ./arrow/compute ./arrow/csv -run Timestamp -count=1Are there any user-facing changes?
Timestamp string parsing now rejects mismatched timezone presence:
Explicit offsets still determine the represented instant, and mixed-case UTC metadata remains accepted. CSV output now includes the offset for timezone-aware timestamps.