lidavidm commented on code in PR #455:
URL: https://github.com/apache/arrow-go/pull/455#discussion_r2296506705
##########
parquet/variant/builder_test.go:
##########
@@ -59,7 +59,7 @@ func TestBuildPrimitive(t *testing.T) {
{"primitive_int32", func(b *variant.Builder) error { return
b.AppendInt(123456) }},
// FIXME: https://github.com/apache/parquet-testing/issues/82
// primitive_int64 is an int32 value, but the metadata is int64
- {"primitive_int64", func(b *variant.Builder) error { return
b.AppendInt(12345678) }},
+ {"primitive_int64", func(b *variant.Builder) error { return
b.AppendInt(1234567890123456789) }},
Review Comment:
Ditto here
##########
parquet/variant/variant_test.go:
##########
@@ -154,7 +154,7 @@ func TestPrimitiveVariants(t *testing.T) {
{"primitive_int32", int32(123456), variant.Int32, "123456"},
// FIXME: https://github.com/apache/parquet-testing/issues/82
// primitive_int64 is an int32 value, but the metadata is int64
- {"primitive_int64", int32(12345678), variant.Int32, "12345678"},
+ {"primitive_int64", int64(1234567890123456789), variant.Int64,
"1234567890123456789"},
Review Comment:
Seems the linked issue is resolved?
##########
arrow/extensions/variant.go:
##########
@@ -171,21 +172,27 @@ func NewVariantType(storage arrow.DataType)
(*VariantType, error) {
return nil, fmt.Errorf("%w: missing non-nullable field
'metadata' in variant storage type %s", arrow.ErrInvalid, storage)
}
- if valueFieldIdx, ok = s.FieldIdx("value"); !ok {
- return nil, fmt.Errorf("%w: missing non-nullable field 'value'
in variant storage type %s", arrow.ErrInvalid, storage)
+ var valueOk, typedValueOk bool
+ valueFieldIdx, valueOk = s.FieldIdx("value")
+ typedValueFieldIdx, typedValueOk = s.FieldIdx("typed_value")
+
+ if !valueOk && !typedValueOk {
+ return nil, fmt.Errorf("%w: there must be at least one of
'value' or 'typed_value' fields in variant storage type %s", arrow.ErrInvalid,
storage)
}
- if s.NumFields() > 3 {
- return nil, fmt.Errorf("%w: too many fields in variant storage
type %s, expected 2 or 3", arrow.ErrInvalid, storage)
+ // if valueFieldIdx, ok = s.FieldIdx("value"); !ok {
+ // return nil, fmt.Errorf("%w: missing non-nullable field 'value'
in variant storage type %s", arrow.ErrInvalid, storage)
+ // }
Review Comment:
Did you mean to leave this in?
##########
arrow/extensions/variant_test.go:
##########
@@ -126,11 +113,11 @@ func TestVariantExtensionBadNestedTypes(t *testing.T) {
), Nullable: false})},
{"empty struct elem", arrow.StructOf(
arrow.Field{Name: "foobar", Type: arrow.StructOf(),
Nullable: false})},
- {"nullable value struct elem",
- arrow.StructOf(
- arrow.Field{Name: "foobar", Type:
arrow.StructOf(
- arrow.Field{Name: "value", Type:
arrow.BinaryTypes.Binary, Nullable: true},
- ), Nullable: false})},
+ // {"nullable value struct elem",
+ // arrow.StructOf(
+ // arrow.Field{Name: "foobar", Type:
arrow.StructOf(
+ // arrow.Field{Name: "value", Type:
arrow.BinaryTypes.Binary, Nullable: true},
+ // ), Nullable: false})},
Review Comment:
Just remove the case?
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
