fix(arrow/extensions): reject Null Variant typed_value - #1243
Conversation
A Null typed_value column is not a valid shredded type. Variant nulls belong in the value column. Fixes apache#1205 Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The direct Null typed_value rejection is incomplete in two ways.
First, NewShreddedVariantType(arrow.Null) now silently returns nil because its call to NewVariantType discards the new validation error. Second, nested typed_value fields can still wrap Null storage in an extension and bypass the new checks. Both cases were reproduced; details are inline.
Extension, race, vet, and focused pqarrow Variant tests otherwise pass. This head currently has no CI results.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the points above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
| dt = dt.(arrow.ExtensionType).StorageType() | ||
| } | ||
|
|
||
| if dt.ID() == arrow.NULL { |
There was a problem hiding this comment.
This new rejection makes NewShreddedVariantType(arrow.Null) silently return nil, because that constructor discards the error returned by NewVariantType. Normal use of the returned type then panics. Please handle arrow.Null with a valid non-nil representation or expose an error-returning construction path instead of discarding the validation failure.
| if f.Name == "value" { | ||
| return isBinary(f.Type) | ||
| } | ||
| return f.Name == "typed_value" && f.Type.ID() != arrow.NULL |
There was a problem hiding this comment.
Checking only f.Type.ID() lets an extension whose storage type is arrow.Null bypass this validation because its ID is EXTENSION. The same bypass exists in the two-field path below. Please unwrap extension storage before checking for Null in both nested typed_value forms and add one- and two-field regression tests.
2 participants
Rationale for this change
A Null
typed_valuecolumn is not a valid shredded Variant type. Nulls are encoded in thevaluecolumn as Variant null (00). A dedicated Nulltyped_valuewould also make “field present and null” indistinguishable from “field missing” for shredded objects.See apache/arrow#50622 and apache/arrow#50810.
Fixes #1205
What changes are included in this PR?
NewVariantTyperejects a Nulltyped_valuefieldtyped_valueare also rejectedAre these changes tested?
go test ./arrow/extensionsAre there any user-facing changes?
Yes — constructing a Variant extension type with a Null
typed_valuenow returnsarrow.ErrInvalid.