laskoviymishka commented on code in PR #1581:
URL: https://github.com/apache/iceberg-go/pull/1581#discussion_r3681096087
##########
table/partitioned_fanout_writer.go:
##########
@@ -500,6 +547,8 @@ func getArrowValueAsIcebergLiteral(column arrow.Array, row
int, sourceType icebe
case *array.LargeBinary:
return iceberg.NewLiteral(arr.Value(row)), nil
+ case *array.FixedSizeBinary:
+ return iceberg.NewLiteral(arr.Value(row)).To(sourceType)
Review Comment:
`NewLiteral([]byte)` gives a `BinaryLiteral`, and `.To` only succeeds for
`Fixed` (matching length), `Binary`, or `UUID`. A `FixedSizeBinary(4)` against
an Iceberg `fixed[3]`, or any other `sourceType`, comes back as a bare
`ErrBadCast` that propagates up and aborts the whole write with no column or
field ID to go on.
It fails loudly rather than corrupting data, but the diagnostic is rough.
I'd assert `sourceType` is Fixed/Binary/UUID up front, or read `ByteWidth` off
the Arrow type and build the `FixedLiteral` directly when the widths agree, so
a mismatch names the column.
##########
table/partitioned_fanout_writer.go:
##########
@@ -500,6 +547,8 @@ func getArrowValueAsIcebergLiteral(column arrow.Array, row
int, sourceType icebe
case *array.LargeBinary:
return iceberg.NewLiteral(arr.Value(row)), nil
+ case *array.FixedSizeBinary:
Review Comment:
The other cases here — including the `LargeBinary` one right above — have a
blank line between the `case` label and the `return`. This one doesn't, so
`nlreturn` will fail CI. Add the blank line to match.
##########
table/partitioned_fanout_writer_test.go:
##########
@@ -282,6 +289,66 @@ func (s *FanoutWriterTestSuite) TestIdentityTransform() {
s.testTransformPartition(iceberg.IdentityTransform{}, "large_name",
"identity_large_string", testRecord, 5)
}
+func (s *FanoutWriterTestSuite) TestBinaryPartitionValuesUseComparableKeys() {
+ tests := []struct {
+ name string
+ arrowType arrow.DataType
+ icebergType iceberg.Type
+ }{
+ {name: "binary", arrowType: arrow.BinaryTypes.Binary,
icebergType: iceberg.PrimitiveTypes.Binary},
+ {name: "fixed", arrowType:
&arrow.FixedSizeBinaryType{ByteWidth: 4}, icebergType: iceberg.FixedTypeOf(4)},
+ }
+
+ for _, test := range tests {
+ s.Run(test.name, func() {
+ arrowSchema := arrow.NewSchema([]arrow.Field{{Name:
"part", Type: test.arrowType}}, nil)
+ record := s.createCustomTestRecord(arrowSchema,
[][]any{{[]byte{1, 2, 3, 4}}, {[]byte{1, 2, 3, 4}}, {[]byte{5, 6, 7, 8}}})
+ defer record.Release()
+
+ icebergSchema := iceberg.NewSchema(1,
iceberg.NestedField{ID: 1, Name: "part", Type: test.icebergType})
+ spec := iceberg.NewPartitionSpec(iceberg.PartitionField{
+ SourceIDs: []int{1}, FieldID: 1000, Name:
"part", Transform: iceberg.IdentityTransform{},
+ })
+
+ partitions, err := getRecordPartitions(spec,
icebergSchema, record)
+ s.Require().NoError(err)
+ s.Require().Len(partitions, 2)
+ switch values := record.Column(0).(type) {
Review Comment:
This is meant to prove the stored key is a clone rather than an alias of the
Arrow buffer, but the proof hinges on the mutation taking effect. Arrow-Go
doesn't guarantee `Value(row)` hands back writable memory — if a release
returns a read-only or copied slice, the mutation no-ops, both keys stay
`{1,2,3,4}`, and the test passes whether or not `clonePartitionValue` exists.
And with no `default` arm, a column that's neither `Binary` nor
`FixedSizeBinary` skips the mutation silently and the assertions pass vacuously.
I'd prove the clone structurally — compare
`unsafe.SliceData(partitionRec[0].([]byte))` against the Arrow buffer pointer,
or `Release` the record before reading `partitionRec` — and add a `default:
s.FailNow(...)`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
