fix(arrow): include REE value nullability in type equality - #1158
Conversation
cc1f953 to
cfda77b
Compare
cfda77b to
36f906a
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Found two blocking API consistency issues: timestamp-with-offset still accepts a nullable REE offset type that is unequal to its canonical type, and strict REE equality leaves no public array constructor that can preserve ValueNullable=false. The branch also currently conflicts with main and has a failing Verify check.
This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. After you've addressed the points above and pushed an update, an Apache Arrow Go maintainer — a real person — will take the next look at the PR. If you think one of the findings is misapplied, please reply on the PR and a maintainer will weigh in.
More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
| @@ -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) | |||
There was a problem hiding this 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.
| return TypeEqual(l.Encoded(), r.Encoded(), opts...) && | ||
| TypeEqual(l.runEnds, r.runEnds, opts...) | ||
| TypeEqual(l.runEnds, r.runEnds, opts...) && | ||
| l.ValueNullable == r.ValueNullable |
There was a problem hiding this 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.
36f906a to
120bf15
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The previous blockers are resolved. REE value nullability is now consistently represented in equality and fingerprints, preserved through array construction and serialization paths, and enforced for timestamp-with-offset storage types. The affected package, race, roundtrip, and tagged C Data tests passed locally.
2 participants
Rationale for this change
TypeEqual compares run-end and value data types for run-end encoded types, but it does not compare whether the value field is nullable. Structurally different types can therefore compare equal and produce the same type fingerprint.
What changes are included in this PR?
Include ValueNullable in run-end encoded type equality and fingerprints.
Are these changes tested?
Yes. The regression test verifies both comparison directions and distinct fingerprints for nullable and non-nullable value fields. The full arrow package suite passes.
Are there any user-facing changes?
Run-end encoded types with different value nullability now compare unequal and have different fingerprints.