zeroshade commented on code in PR #1158:
URL: https://github.com/apache/arrow-go/pull/1158#discussion_r3855820607
##########
arrow/compare.go:
##########
@@ -150,7 +150,8 @@ func TypeEqual(left, right DataType, opts
...TypeEqualOption) bool {
case *RunEndEncodedType:
r := right.(*RunEndEncodedType)
return TypeEqual(l.Encoded(), r.Encoded(), opts...) &&
- TypeEqual(l.runEnds, r.runEnds, opts...)
+ TypeEqual(l.runEnds, r.runEnds, opts...) &&
+ l.ValueNullable == r.ValueNullable
Review Comment:
**Blocking:** Making equality strict exposes a public API gap:
`array.NewRunEndEncodedArray` always calls `RunEndEncodedOf`, which resets
`ValueNullable` to true. Existing code that defines a non-nullable REE schema
and constructs its column with this public helper now gets unequal types and a
`NewRecordBatch` type-mismatch panic—even though both types print identically.
This PR works around the same problem with a private typed helper in `arrdata`.
Please add a public constructor that accepts/preserves the `*RunEndEncodedType`
and cover record construction and concatenation with `ValueNullable=false`.
##########
arrow/extensions/timestamp_with_offset.go:
##########
@@ -46,10 +46,6 @@ func isOffsetTypeOk(offsetType arrow.DataType) bool {
case *arrow.RunEndEncodedType:
return offsetType.ValidRunEndsType(offsetType.RunEnds()) &&
arrow.TypeEqual(offsetType.Encoded(),
arrow.PrimitiveTypes.Int16)
Review Comment:
**Blocking:** Now that IPC preserves `ValueNullable`, this compatibility
check still ignores it. `RunEndEncodedOf(Int16, Int16)` defaults to
`ValueNullable=true` and is accepted by
`NewTimestampWithOffsetTypeCustomOffset`, while the canonical
`NewTimestampWithOffsetTypeRunEndEncoded` constructor explicitly sets it to
false. These accepted extension types now compare unequal and have different
fingerprints; a nullable REE can deserialize successfully but fail
`ExtensionEquals` against the canonical type. The removed FIXME said this check
was disabled only because IPC lost the flag. Please either require
`!offsetType.ValueNullable` or normalize legacy input explicitly, with
rejection/round-trip coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
