From 83aed27acae645be925e035760710e7cbaef8221 Mon Sep 17 00:00:00 2001 From: Digvijay Date: Fri, 28 Aug 2026 00:14:57 -0500 Subject: [PATCH 1/3] fix(arrow/extensions): reject Null Variant typed_value A Null typed_value column is not a valid shredded type. Variant nulls belong in the value column. Fixes #1205 Signed-off-by: Digvijay --- arrow/extensions/variant.go | 11 +++++++++-- arrow/extensions/variant_test.go | 10 ++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/arrow/extensions/variant.go b/arrow/extensions/variant.go index fee2e046a..555946961 100644 --- a/arrow/extensions/variant.go +++ b/arrow/extensions/variant.go @@ -225,6 +225,10 @@ func NewVariantType(storage arrow.DataType) (*VariantType, error) { dt = dt.(arrow.ExtensionType).StorageType() } + if dt.ID() == arrow.NULL { + return nil, fmt.Errorf("%w: typed_value field must not be null type", arrow.ErrInvalid) + } + if nt, ok := dt.(arrow.NestedType); ok { if !validNestedType(nt) { return nil, fmt.Errorf("%w: typed_value field must be a valid nested type, got %s", arrow.ErrInvalid, typedValueField.Type) @@ -295,7 +299,10 @@ func validStruct(s *arrow.StructType) bool { switch s.NumFields() { case 1: f := s.Field(0) - return (f.Name == "value" && isBinary(f.Type)) || f.Name == "typed_value" + if f.Name == "value" { + return isBinary(f.Type) + } + return f.Name == "typed_value" && f.Type.ID() != arrow.NULL case 2: valField, ok := s.FieldByName("value") if !ok || !valField.Nullable || !isBinary(valField.Type) { @@ -311,7 +318,7 @@ func validStruct(s *arrow.StructType) bool { return validNestedType(nt) } - return true + return typedField.Type.ID() != arrow.NULL default: return false } diff --git a/arrow/extensions/variant_test.go b/arrow/extensions/variant_test.go index a39fd5131..e49177465 100644 --- a/arrow/extensions/variant_test.go +++ b/arrow/extensions/variant_test.go @@ -83,6 +83,11 @@ func TestVariantExtensionType(t *testing.T) { arrow.Field{Name: "metadata", Type: arrow.BinaryTypes.String, Nullable: false}, arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: false}), "metadata field must be non-nullable binary type, got utf8"}, + {arrow.StructOf( + arrow.Field{Name: "metadata", Type: arrow.BinaryTypes.Binary, Nullable: false}, + arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, + arrow.Field{Name: "typed_value", Type: arrow.Null, Nullable: true}), + "typed_value field must not be null type"}, } for _, tt := range tests { @@ -113,6 +118,11 @@ func TestVariantExtensionBadNestedTypes(t *testing.T) { ), Nullable: false})}, {"empty struct elem", arrow.StructOf( arrow.Field{Name: "foobar", Type: arrow.StructOf(), Nullable: false})}, + {"null typed_value in shredded field", arrow.StructOf( + arrow.Field{Name: "foobar", Type: arrow.StructOf( + arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, + arrow.Field{Name: "typed_value", Type: arrow.Null, Nullable: true}, + ), Nullable: false})}, {"non-nullable two elem struct", arrow.StructOf( arrow.Field{Name: "foobar", Type: arrow.StructOf( arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, From d07d9d53c5a627676ab63cece677f49a659a3409 Mon Sep 17 00:00:00 2001 From: Digvijay Date: Mon, 31 Aug 2026 23:19:14 -0500 Subject: [PATCH 2/3] fix(arrow/extensions): handle Null shredded construction Treat Null like a missing shred type so NewShreddedVariantType never returns nil. Unwrap extension storage before nested Null checks. Signed-off-by: Digvijay --- arrow/extensions/variant.go | 27 +++++++++++++++++---------- arrow/extensions/variant_test.go | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/arrow/extensions/variant.go b/arrow/extensions/variant.go index 555946961..0a691452d 100644 --- a/arrow/extensions/variant.go +++ b/arrow/extensions/variant.go @@ -122,10 +122,10 @@ func createShreddedField(dt arrow.DataType) arrow.DataType { // ), Nullable: true}) // // This is intended to be a convenient way to create a shredded variant type from a definition -// of the fields to shred. If the provided data type is nil, it will create a default -// variant type. +// of the fields to shred. If the provided data type is nil or Null (including an +// extension whose storage is Null), it will create a default variant type. func NewShreddedVariantType(dt arrow.DataType) *VariantType { - if dt == nil { + if dt == nil || isNullType(dt) { return NewDefaultVariantType() } @@ -220,11 +220,7 @@ func NewVariantType(storage arrow.DataType) (*VariantType, error) { return nil, fmt.Errorf("%w: typed_value field must be nullable, got %s", arrow.ErrInvalid, typedValueField.Type) } - dt := typedValueField.Type - if dt.ID() == arrow.EXTENSION { - dt = dt.(arrow.ExtensionType).StorageType() - } - + dt := storageType(typedValueField.Type) if dt.ID() == arrow.NULL { return nil, fmt.Errorf("%w: typed_value field must not be null type", arrow.ErrInvalid) } @@ -295,6 +291,17 @@ func isBinary(dt arrow.DataType) bool { dt.ID() == arrow.BINARY_VIEW } +func storageType(dt arrow.DataType) arrow.DataType { + if ext, ok := dt.(arrow.ExtensionType); ok { + return ext.StorageType() + } + return dt +} + +func isNullType(dt arrow.DataType) bool { + return storageType(dt).ID() == arrow.NULL +} + func validStruct(s *arrow.StructType) bool { switch s.NumFields() { case 1: @@ -302,7 +309,7 @@ func validStruct(s *arrow.StructType) bool { if f.Name == "value" { return isBinary(f.Type) } - return f.Name == "typed_value" && f.Type.ID() != arrow.NULL + return f.Name == "typed_value" && !isNullType(f.Type) case 2: valField, ok := s.FieldByName("value") if !ok || !valField.Nullable || !isBinary(valField.Type) { @@ -318,7 +325,7 @@ func validStruct(s *arrow.StructType) bool { return validNestedType(nt) } - return typedField.Type.ID() != arrow.NULL + return !isNullType(typedField.Type) default: return false } diff --git a/arrow/extensions/variant_test.go b/arrow/extensions/variant_test.go index e49177465..d2cc01249 100644 --- a/arrow/extensions/variant_test.go +++ b/arrow/extensions/variant_test.go @@ -88,6 +88,11 @@ func TestVariantExtensionType(t *testing.T) { arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, arrow.Field{Name: "typed_value", Type: arrow.Null, Nullable: true}), "typed_value field must not be null type"}, + {arrow.StructOf( + arrow.Field{Name: "metadata", Type: arrow.BinaryTypes.Binary, Nullable: false}, + arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, + arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType(arrow.Null, "null", "test"), Nullable: true}), + "typed_value field must not be null type"}, } for _, tt := range tests { @@ -123,6 +128,15 @@ func TestVariantExtensionBadNestedTypes(t *testing.T) { arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, arrow.Field{Name: "typed_value", Type: arrow.Null, Nullable: true}, ), Nullable: false})}, + {"null typed_value extension in one-field shredded field", arrow.StructOf( + arrow.Field{Name: "foobar", Type: arrow.StructOf( + arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType(arrow.Null, "null", "test"), Nullable: true}, + ), Nullable: false})}, + {"null typed_value extension in two-field shredded field", arrow.StructOf( + arrow.Field{Name: "foobar", Type: arrow.StructOf( + arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, + arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType(arrow.Null, "null", "test"), Nullable: true}, + ), Nullable: false})}, {"non-nullable two elem struct", arrow.StructOf( arrow.Field{Name: "foobar", Type: arrow.StructOf( arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, @@ -1570,6 +1584,10 @@ func TestVariantBuilderUnmarshalJSON(t *testing.T) { func TestNewSimpleShreddedVariantType(t *testing.T) { assert.True(t, arrow.TypeEqual(extensions.NewDefaultVariantType(), extensions.NewShreddedVariantType(nil))) + assert.True(t, arrow.TypeEqual(extensions.NewDefaultVariantType(), + extensions.NewShreddedVariantType(arrow.Null))) + assert.True(t, arrow.TypeEqual(extensions.NewDefaultVariantType(), + extensions.NewShreddedVariantType(extensions.NewOpaqueType(arrow.Null, "null", "test")))) vt := extensions.NewShreddedVariantType(arrow.PrimitiveTypes.Float32) s := arrow.StructOf( From a5adf0e2342723f3c3e825670070a98216715cc9 Mon Sep 17 00:00:00 2001 From: Digvijay Date: Tue, 1 Sep 2026 23:25:41 -0500 Subject: [PATCH 3/3] fix(arrow/extensions): unwrap nested Variant typed_value extensions Reject double-wrapped Null and nil Opaque storage instead of panicking or silently constructing an invalid type. Signed-off-by: Digvijay --- arrow/extensions/variant.go | 21 +++++++++++++++++---- arrow/extensions/variant_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/arrow/extensions/variant.go b/arrow/extensions/variant.go index 0a691452d..4f27dac4c 100644 --- a/arrow/extensions/variant.go +++ b/arrow/extensions/variant.go @@ -221,6 +221,9 @@ func NewVariantType(storage arrow.DataType) (*VariantType, error) { } dt := storageType(typedValueField.Type) + if dt == nil { + return nil, fmt.Errorf("%w: typed_value field has invalid storage type", arrow.ErrInvalid) + } if dt.ID() == arrow.NULL { return nil, fmt.Errorf("%w: typed_value field must not be null type", arrow.ErrInvalid) } @@ -292,14 +295,24 @@ func isBinary(dt arrow.DataType) bool { } func storageType(dt arrow.DataType) arrow.DataType { - if ext, ok := dt.(arrow.ExtensionType); ok { - return ext.StorageType() + seen := make(map[arrow.DataType]struct{}) + for dt != nil { + ext, ok := dt.(arrow.ExtensionType) + if !ok { + return dt + } + if _, dup := seen[dt]; dup { + return nil + } + seen[dt] = struct{}{} + dt = ext.StorageType() } - return dt + return nil } func isNullType(dt arrow.DataType) bool { - return storageType(dt).ID() == arrow.NULL + st := storageType(dt) + return st == nil || st.ID() == arrow.NULL } func validStruct(s *arrow.StructType) bool { diff --git a/arrow/extensions/variant_test.go b/arrow/extensions/variant_test.go index d2cc01249..905fc5d9b 100644 --- a/arrow/extensions/variant_test.go +++ b/arrow/extensions/variant_test.go @@ -93,6 +93,17 @@ func TestVariantExtensionType(t *testing.T) { arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType(arrow.Null, "null", "test"), Nullable: true}), "typed_value field must not be null type"}, + {arrow.StructOf( + arrow.Field{Name: "metadata", Type: arrow.BinaryTypes.Binary, Nullable: false}, + arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, + arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType( + extensions.NewOpaqueType(arrow.Null, "null", "test"), "null", "test"), Nullable: true}), + "typed_value field must not be null type"}, + {arrow.StructOf( + arrow.Field{Name: "metadata", Type: arrow.BinaryTypes.Binary, Nullable: false}, + arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, + arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType(nil, "null", "test"), Nullable: true}), + "typed_value field has invalid storage type"}, } for _, tt := range tests { @@ -137,6 +148,17 @@ func TestVariantExtensionBadNestedTypes(t *testing.T) { arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType(arrow.Null, "null", "test"), Nullable: true}, ), Nullable: false})}, + {"double-wrapped null typed_value in one-field shredded field", arrow.StructOf( + arrow.Field{Name: "foobar", Type: arrow.StructOf( + arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType( + extensions.NewOpaqueType(arrow.Null, "null", "test"), "null", "test"), Nullable: true}, + ), Nullable: false})}, + {"double-wrapped null typed_value in two-field shredded field", arrow.StructOf( + arrow.Field{Name: "foobar", Type: arrow.StructOf( + arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, + arrow.Field{Name: "typed_value", Type: extensions.NewOpaqueType( + extensions.NewOpaqueType(arrow.Null, "null", "test"), "null", "test"), Nullable: true}, + ), Nullable: false})}, {"non-nullable two elem struct", arrow.StructOf( arrow.Field{Name: "foobar", Type: arrow.StructOf( arrow.Field{Name: "value", Type: arrow.BinaryTypes.Binary, Nullable: true}, @@ -1588,6 +1610,11 @@ func TestNewSimpleShreddedVariantType(t *testing.T) { extensions.NewShreddedVariantType(arrow.Null))) assert.True(t, arrow.TypeEqual(extensions.NewDefaultVariantType(), extensions.NewShreddedVariantType(extensions.NewOpaqueType(arrow.Null, "null", "test")))) + assert.True(t, arrow.TypeEqual(extensions.NewDefaultVariantType(), + extensions.NewShreddedVariantType(extensions.NewOpaqueType( + extensions.NewOpaqueType(arrow.Null, "null", "test"), "null", "test")))) + assert.True(t, arrow.TypeEqual(extensions.NewDefaultVariantType(), + extensions.NewShreddedVariantType(extensions.NewOpaqueType(nil, "null", "test")))) vt := extensions.NewShreddedVariantType(arrow.PrimitiveTypes.Float32) s := arrow.StructOf(