Add `TimestampWithOffset` canonical extension type by serramatutu · Pull Request #558 · apache/arrow-go

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu · 2025-10-30T11:16:25Z

Which issue does this PR close?

Implements the new arrow.timestamp_with_offset canonical extension type for arrow-go.

Rationale for this change

Be compatible with the Arrow columnar spec's arrow.timestamp_with_offset canonical extension type.

What changes are included in this PR?

Adds a TimestampWithOffset extension type. This type represents a timestamp column that stores a potentially different timezone offset per value: the timestamp is stored in UTC alongside the original timezone offset in minutes. The offset-in-minutes field can be primitive-, dictionary-, or run-end-encoded.

This PR has been decoupled from the inner-nullability parity work it was previously based on. Those comparison / JSON-marshaling changes now live in #918 and can be reviewed and merged independently. This PR is now scoped to just the extension type and touches only:

arrow/extensions/timestamp_with_offset.go (new)
arrow/extensions/timestamp_with_offset_test.go (new)
arrow/extensions/extensions.go (register the canonical type)

To keep this PR self-contained, the JSON round-trip test asserts the values the type guarantees (per-row instant, timezone offset, and validity) rather than array.RecordEqual, which would compare inner non-nullable struct child validity bitmaps — the parity concern tracked separately in #918.

Are these changes tested?

Yes — primitive/dictionary/run-end offset encodings, JSON marshal + round-trip, and IPC round-trip.

Are there any user-facing changes?

Yes, this is a new canonical extension type.

…48002) ### Rationale for this change Closes#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: #44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

zeroshade · 2025-12-22T18:05:19Z

+ timestamps.UnsafeAppendBoolToBitmap(false)
+ offsets.UnsafeAppendBoolToBitmap(false)


same as above, these are non-nullable according to the spec.

I did change this but now the IPC roundtrip tests are failing because the internal count of nulls is different between the thing before IPC and after IPC.
To make them pass we'll need to relax the arrow.RecordEqual() to ignore NullN() when the field is not nullable. Alternatively we can change it so that an array builder sets nulls = 0 if the array is not nullable, regardless of what has been set before in calls to UnsafeAppendBoolToBitmap() etc
Related: https://lists.apache.org/thread/7gbqjwykh1ob3xbvwph3ljsdl5c7kxpd

zeroshade · 2026-02-05T23:25:44Z

+	*arrow.Int8Type | *arrow.Int16Type | *arrow.Int32Type | *arrow.Int64Type |
+ *arrow.Uint8Type | *arrow.Uint16Type | *arrow.Uint32Type | *arrow.Uint64Type


this and the DictIndexType are both identical, instead of duplicating this we should either create a single type constraint or embed one in the other:
typeTimestampWithOffsetRunEndsTypeinterface { DictIndexType }
My personal preference would be to have a single type though, but I'm not averse to having the two separate ones if necessary.

Fixed: 231cb1e

Actually I had to revert. Spec says those types are different:
Dict index is either signed or unsigned int of 8-64 bits
The index type of a Dictionary type can only be an integer type, preferably signed, with width 8 to 64 bits.
Run ends is a signed int of 16-64 bits
The run end type of a Run-End Encoded type can only be a signed integer type with width 16 to 64 bits.
https://arrow.apache.org/docs/format/Columnar.html#data-types

…type (apache#48002) ### Rationale for this change Closesapache#44248 Arrow has no built-in canonical way of representing the `TIMESTAMP WITH TIME ZONE` SQL type, which is present across multiple different database systems. Not having a native way to represent this forces users to either convert to UTC and drop the time zone, which may have correctness implications, or use bespoke workarounds. A new `arrow.timestamp_with_offset` extension type would introduce a standard canonical way of representing that information. Rust implementation: apache/arrow-rs#8743 Go implementation: apache/arrow-go#558 [DISCUSS] [thread in the mailing list](https://lists.apache.org/thread/yhbr3rj9l59yoxv92o2s6dqlop16sfnk). ### What changes are included in this PR? Proposal and documentation for `arrow.timestamp_with_offset` canonical extension type. ### Are these changes tested? N/A ### Are there any user-facing changes? Yes, this is an extension to the arrow format. * GitHub Issue: apache#44248 --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

Copilot

Pull request overview

This PR introduces the canonical extension type arrow.timestamp_with_offset to match the Arrow spec, and updates JSON marshaling/unmarshaling plus array/record equality behavior to better respect schema nullability (especially for nested types and IPC-vs-JSON roundtrips).

Changes:

Add TimestampWithOffsetType, TimestampWithOffsetArray, and a convenience builder supporting primitive/dictionary/REE encodings for per-row timezone offsets.
Update JSON marshaling (GetOneForMarshal) to take schema nullability into account, and adjust struct/record JSON unmarshaling to handle explicit null for required fields.
Update array/record/table equality logic to optionally ignore validity bitmaps when fields are considered non-nullable, and refresh test fixtures accordingly.

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/json/json.go	Add `IsNullMessage` helper for null detection.
internal/json/json_stdlib.go	Stdlib variant of `IsNullMessage`.
arrow/ipc/cmd/arrow-ls/main_test.go	Update expected struct field nullability formatting in output.
arrow/internal/arrjson/arrjson_test.go	Update expected JSON schema nullability for struct children.
arrow/internal/arrdata/arrdata.go	Update struct test data to mark inner fields nullable and adjust validity masks.
arrow/extensions/variant.go	Update `GetOneForMarshal` signature to include nullability.
arrow/extensions/uuid.go	Update `GetOneForMarshal` signature + call sites.
arrow/extensions/uuid_test.go	Mark UUID field nullable in record-builder test.
arrow/extensions/timestamp_with_offset.go	New extension type implementation + builder/array logic.
arrow/extensions/timestamp_with_offset_test.go	New tests covering primitive/dict/REE encodings, JSON roundtrip, IPC roundtrip.
arrow/extensions/json.go	Make JSON extension marshaling honor caller-provided nullability.
arrow/extensions/extensions.go	Register `TimestampWithOffsetType` as a canonical extension type.
arrow/extensions/bool8.go	Update `GetOneForMarshal` signature to include nullability.
arrow/compute/vector_sort_test.go	Mark fields nullable in C++ parity tests (to align with new nullability-aware behavior).
arrow/array/util.go	Pass schema field nullability into `GetOneForMarshal` during Record-to-JSON.
arrow/array/util_test.go	Exercise record JSON roundtrip under nullable vs non-nullable schemas.
arrow/array/union.go	Make union JSON marshaling and approx-equality respect child-field nullability.
arrow/array/timestamp.go	Update `GetOneForMarshal` and equality to respect nullable option.
arrow/array/struct.go	Make struct JSON marshaling/unmarshaling nullability-aware; adjust struct equality accordingly.
arrow/array/struct_test.go	Add coverage for explicit `null` in required struct fields.
arrow/array/string.go	Update string array marshaling and equality to respect nullable option.
arrow/array/record.go	Make record JSON unmarshaling treat explicit `null` in required fields as empty value.
arrow/array/record_test.go	Extend record-builder tests for new null-handling + JSON roundtrip.
arrow/array/numeric_generic.go	Update numeric marshaling and equality to respect nullable option.
arrow/array/null.go	Update `GetOneForMarshal` signature.
arrow/array/map.go	Thread nullable option through map equality.
arrow/array/list.go	Update list marshaling and equality to respect element-field nullability.
arrow/array/json_reader_test.go	Update JSON reader test to pass nullable flag when marshaling one value.
arrow/array/interval.go	Update interval marshaling and equality to respect nullable option.
arrow/array/float16.go	Update `GetOneForMarshal` signature to include nullability.
arrow/array/fixedsize_binary.go	Update fixed-size-binary marshaling and equality to respect nullable option.
arrow/array/fixed_size_list.go	Update fixed-size-list marshaling and equality to respect element-field nullability.
arrow/array/extension.go	Thread nullable option through extension array equality and marshaling.
arrow/array/encoded.go	Update run-end-encoded marshaling and equality to thread nullable option.
arrow/array/dictionary.go	Update dictionary marshaling and equality to thread nullable option.
arrow/array/decimal256_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal128_test.go	Update test call sites for new `GetOneForMarshal` signature.
arrow/array/decimal.go	Update decimal marshaling and equality to respect nullable option.
arrow/array/compare.go	Add nullable-aware equality plumbing and new APIs; thread schema nullability into comparisons.
arrow/array/compare_test.go	Add tests for `WithNullable(false)` behavior.
arrow/array/boolean.go	Update boolean marshaling and equality to respect nullable option.
arrow/array/binary.go	Update binary marshaling and equality to respect nullable option.
arrow/array.go	Change `GetOneForMarshal` interface to accept nullability flag.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

serramatutu

@zeroshade some comments:

Can you think of a way to decouple these 2 things (timestamp with offset and the inner nullability fixes)? So we can merge the extension type without having to wait on all the fixes for that and do that more carefully?

I'm sorry I left the branch based off of an old version of the inner nullability parity fixes. There are some differences with what I was doing in the new version of the JSON PR:

I ended up dropping the equalOpts because this is not what C++ does. It looks like they just validate the schemas on Validate() instead of making Equal() more lenient. I had not fixed that in that PR because there are some issues with the CSV reader (it calls Validate()) and it was becoming somewhat of a sprawl to fix it all. There are lots of behavior in arrow-go that never checks for the schema of structs.
I did not do the GetOneForMarshalNullable() and instead I changed the struct/record builder to check for IsValid() before calling GetOneForMarshal().

This commit adds a new `TimestampWithOffset` extension type that can be used to represent timestamps with per-row timezone information. It stores information in a `struct` with 2 fields, `timestamp=[T, "UTC"]`, where `T` can be any `arrow.TimeUnit` and `offset_minutes=int16`, which represents the offset in minutes from the UTC timestamp.

Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Start a run when none exists yet (e.g. leading null rows) instead of continuing a nonexistent run, and only update lastOffset when a new run starts. This stops a leading null from corrupting the run-ends/values children and stops a null between equal offsets from splitting one contiguous run into two. Add a noLastOffset sentinel, handle nil valids, and add regression tests across all offset encodings.

Address roborev review findings on the run-end-encoded offset builder: - AppendValues now treats a nil valids slice as all-valid so the parent struct and its child builders stay the same length. Previously the struct received zero slots while the children received len(values), producing an inconsistent extension array. - Override NewArray/NewExtensionArray to reset lastOffset to noLastOffset on finalization, so a reused builder starts a fresh run instead of continuing a run that belonged to the array just finalized (which could emit run-ends without a matching value on a run-end-encoded offset). - Add regression tests for nil validity and builder reuse across all offset encodings.

Follow-up to the nil-validity fix: an empty (len 0) valids slice is also all-valid, matching the convention of the other Arrow builders. AppendValues now normalizes both nil and empty validity, and panics on a genuine length mismatch (a non-empty valids whose length differs from values). Extend the validity regression test to cover the empty slice and add a test asserting the length-mismatch panic.

Follow-up to the run-end-encoding fixes: AppendNull (and therefore the UnmarshalOne(null) and AppendValueFromString(null) paths that call it) appends a null offset run via the embedded struct builder but left lastOffset stale. A subsequent value repeating the pre-null offset would then continue the null run instead of starting its own, encoding that value with the null offset run. Override AppendNull/AppendNulls to reset lastOffset to noLastOffset, and add a regression test for Append(value), AppendNull(), Append(same-offset value) with run-end-encoded offsets.

Replace array.RecordEqual with a per-row check of the extension's guarantees (instant, timezone offset, validity). RecordEqual compares the inner non-nullable struct child validity bitmaps, which legitimately differ between builder output (children valid under a null parent) and JSON reader output (children null). That inner-nullability parity is tracked separately and is out of scope for this extension type.

zeroshade · 2026-07-09T19:37:06Z

@serramatutu updated this PR to be only the timestamp with offset extension type

…rays iterValues started the run-end tracker at physical index 0 and compared the logical row against absolute run ends, ignoring the array offset. A sliced array whose offset began inside a later run decoded offsets from the wrong run in Values() and MarshalJSON(). Initialize the tracker from GetPhysicalOffset() and advance using absolute positions (Offset()+i), and add a regression test slicing an REE-encoded array across run boundaries.

zeroshade · 2026-07-09T20:14:26Z

@serramatutu filed #918 to split out the nullability stuff

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

- timeFromFieldValues: derive the zone-name sign from the whole offset so a sub-hour negative offset keeps its sign (e.g. -30 minutes now formats as "UTC-00:30" instead of "UTC+00:30"; integer hours is 0 there and cannot carry the sign). Add a regression test. - MarshalJSON: store the timestamp by value instead of taking the address of the range variable. - Values: rename the loop variable so it no longer shadows the time package.

Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

…erialize Deserialize now rejects serialized metadata other than "" or "{}", matching the canonical extension spec (empty metadata) and the existing JSONType pattern. Adds a regression test. Also documents (NOTE apache#918) that AppendNull writes a null into the non-nullable offset_minutes storage field; inner-field comparison/JSON parity is tracked in apache#918.

…set Deserialize Adds TestTimestampWithOffsetTypeDeserializeInvalidStorage covering non-struct storage, wrong field count/names/order, non-UTC and empty timezone, nullable timestamp/offset fields, and invalid offset encodings (wrong primitive type, dictionary value not Int16, run-end-encoded value not Int16).

…fset roundtrip

lidavidm · 2026-07-18T10:06:58Z

+ // FIXME: Technically this should be non-nullable, but a Arrow IPC does not deserialize
+ // ValueNullable properly, so enforcing this here would always fail when reading from an IPC
+ // stream
+ // !offsetType.ValueNullable


This is pretty major right? Can we link this to a tracking issue? (Does integration catch this?)

This is part of what was pulled out into #918 and friends

serramatutu mentioned this pull request Oct 30, 2025
GH-44248: [Format] Add TimestampWithOffset canonical extension type apache/arrow#48002
Merged

felipecrv self-requested a review November 1, 2025 01:55

serramatutu force-pushed the serramatutu/TimestampWithOffset/go branch 3 times, most recently from 95230ad to b9b8bf2Compare December 22, 2025 14:33

serramatutu changed the title ~~[DRAFT] Add TimestampWithOffset extension type~~Add TimestampWithOffset extension typeDec 22, 2025

serramatutu marked this pull request as ready for review December 22, 2025 14:42

serramatutu requested a review from zeroshade as a code owner December 22, 2025 14:42

serramatutu changed the title ~~Add TimestampWithOffset extension type~~Add TimestampWithOffset canonical extension typeDec 22, 2025

serramatutu force-pushed the serramatutu/TimestampWithOffset/go branch from b9b8bf2 to ccdd288Compare December 22, 2025 15:17

zeroshade requested changes Dec 22, 2025
View reviewed changes

serramatutu force-pushed the serramatutu/TimestampWithOffset/go branch from ccdd288 to cee17a3Compare January 30, 2026 12:21

serramatutu requested a review from zeroshade February 2, 2026 13:29

zeroshade requested changes Feb 5, 2026
View reviewed changes

serramatutu force-pushed the serramatutu/TimestampWithOffset/go branch 3 times, most recently from bc577e9 to 8a7d6d7Compare April 15, 2026 11:05

serramatutu force-pushed the serramatutu/TimestampWithOffset/go branch 2 times, most recently from f060911 to 9f931fbCompare April 30, 2026 11:33

zeroshade force-pushed the serramatutu/TimestampWithOffset/go branch 2 times, most recently from 49da897 to b6f73a4Compare July 2, 2026 18:11

zeroshade requested review from Copilot, kou and lidavidm July 2, 2026 21:08

Copilotstarted reviewing on behalf of zeroshade July 2, 2026 21:59 View session

CopilotAI reviewed Jul 2, 2026
View reviewed changes

Comment threadarrow/array/compare.go Outdated
Comment threadarrow/array/compare.go Outdated
Comment threadarrow/array/compare.go Outdated
Comment threadarrow/array/compare.go Outdated
Comment threadarrow/array/record_test.go Outdated
Comment threadarrow/extensions/timestamp_with_offset.go Outdated

serramatutu commented Jul 7, 2026
View reviewed changes

serramatutuand others added 13 commits July 9, 2026 15:29

Uncomment test
d3a6336

Fix REE ends type
0f8e9dd

Support unix epoch
b3f52ca
Stop using `time=0` as a sentinel for null in `iterValues()`. Instead, return a tuple of `(time.Time, bool)` where the bool indicates whether the value is valid or not. Added a test for it.

Fix inconsistency with UnsafeAppend()
1dc6886

Stop appending nulls to child arrays
836357a

Fix lint
a9f68f5

Simplify fieldValuesFromTime() implementation
5d4e717

Add subtests
773ae24

zeroshade force-pushed the serramatutu/TimestampWithOffset/go branch from 03b049f to 2ef684cCompare July 9, 2026 19:30

zeroshade mentioned this pull request Jul 9, 2026
fix(arrow/array): honor field nullability in comparison and JSON marshaling #918
Closed

zeroshade requested a review from Copilot July 10, 2026 16:58

Copilotstarted reviewing on behalf of zeroshade July 10, 2026 16:58 View session

CopilotAI reviewed Jul 10, 2026
View reviewed changes

Comment threadarrow/extensions/timestamp_with_offset.go
Comment threadarrow/extensions/timestamp_with_offset.go
Comment threadarrow/extensions/timestamp_with_offset.go

zeroshade added 2 commits July 10, 2026 14:20

Avoid int16 overflow when formatting TimestampWithOffset zone name
3cd5be8
Widen offsetMinutes to int before negating so an out-of-range int16 offset (e.g. math.MinInt16) cannot overflow when computing the absolute value used for the zone name.

serramatutu commented Jul 10, 2026
View reviewed changes

Comment threadarrow/extensions/timestamp_with_offset_test.go Outdated

zeroshadeand others added 3 commits July 12, 2026 13:05

test(extensions): add FIXME to revisit RecordEqual on TimestampWithOf…
1850566
…fset roundtrip

lidavidm approved these changes Jul 19, 2026
View reviewed changes

zeroshade approved these changes Jul 19, 2026
View reviewed changes

zeroshade merged commit c9791ae into apache:mainJul 19, 2026
23 checks passed