zeroshade commented on code in PR #1109:
URL: https://github.com/apache/arrow-go/pull/1109#discussion_r3798680337
##########
arrow/csv/reader.go:
##########
@@ -500,8 +500,14 @@ func (r *Reader) initFieldConverter(bldr array.Builder)
func(string) {
}
}
case *arrow.TimestampType:
+ if _, err := dt.GetZone(); err != nil {
+ if r.err == nil {
+ r.err = err
+ }
+ return func(string) {}
Review 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.
- `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.
##########
arrow/csv/reader.go:
##########
@@ -741,21 +747,16 @@ func (r *Reader) parseFloat64(field array.Builder, str
string) {
field.(*array.Float64Builder).Append(v)
}
-// parses timestamps using millisecond precision
-func (r *Reader) parseTimestamp(field array.Builder, str string, unit
arrow.TimeUnit) {
+func (r *Reader) parseTimestamp(field array.Builder, str string) {
if r.isNull(str) {
field.AppendNull()
return
}
- v, err := arrow.TimestampFromString(str, unit)
+ err := field.(*array.TimestampBuilder).AppendValueFromString(str)
Review 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:
```go
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.
##########
arrow/scalar/parse.go:
##########
@@ -41,6 +40,27 @@ type TypeFromScalar interface {
FromStructScalar(*Struct) error
}
+func parseTimestamp(val string, dt *arrow.TimestampType) (arrow.Timestamp,
error) {
+ loc, err := dt.GetZone()
+ if err != nil {
+ return 0, err
+ }
+
+ ts, zonePresent, err := arrow.TimestampFromStringInLocation(val,
dt.Unit, loc)
+ if err != nil {
+ return 0, err
+ }
+
+ if zonePresent != (dt.TimeZone != "") {
Review 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:
```go
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
