fix(arrow/scalar): align timestamp timezone parsing - #1109
fix(arrow/scalar): align timestamp timezone parsing#1109fallintoplace wants to merge 11 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.
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.