tanmayrauth commented on code in PR #1593:
URL: https://github.com/apache/iceberg-go/pull/1593#discussion_r3670176012
##########
partitions.go:
##########
@@ -469,6 +488,26 @@ func (ps *PartitionSpec) UnmarshalJSON(b []byte) error {
return nil
}
+func validatePartitionFields(fields []PartitionField) error {
+ names := make(map[string]struct{}, len(fields))
+ definitions := make(map[string]struct{}, len(fields))
+ for _, field := range fields {
+ if _, ok := names[field.Name]; ok {
+ return fmt.Errorf("%w: duplicate partition name: %s",
ErrInvalidPartitionSpec, field.Name)
+ }
+ names[field.Name] = struct{}{}
+
+ definition := fmt.Sprintf("%v:%s", field.SourceIDs,
field.Transform)
Review Comment:
This rejects two fields that share (source-ids, transform), but `void` is
the drop marker: dropping a partition field in a v1 spec rewrites it to void
while keeping its source-id, so a column that had two
partition fields both later dropped legally ends up with two
`[source]:void` entries (distinct names/field-ids). This check collapses them
and fails the whole spec on decode — so a table Java/PyIceberg
wrote, and older iceberg-go read fine, now fails to open with
ErrInvalidPartitionSpec. It's also self-inconsistent: NewPartitionSpecID builds
such a spec with no complaint, MarshalJSON emits it, and this
path then refuses to read it back.
Exempt void from the redundancy check (keep the name-uniqueness check):
after `names[field.Name] = struct{}{}`, add `if _, ok :=
field.Transform.(VoidTransform); ok { continue }` before building
`definition`. ParseTransform returns value-typed transforms, so the
non-pointer VoidTransform assertion is right. That still rejects two
identity/bucket/etc. on the same source, which is the case you're
actually targeting.
##########
partitions_test.go:
##########
@@ -647,10 +647,65 @@ func TestPartitionFieldUnmarshalJSON(t *testing.T) {
}`
var field iceberg.PartitionField
err := json.Unmarshal([]byte(jsonData), &field)
- require.NoError(t, err)
- assert.Zero(t, field.SourceID())
- assert.Equal(t, 1003, field.FieldID)
- assert.Equal(t, "void", field.Name)
- assert.Equal(t, iceberg.VoidTransform{}, field.Transform)
+ require.ErrorIs(t, err, iceberg.ErrInvalidPartitionSpec)
+ assert.ErrorContains(t, err, "requires source-id or source-ids")
})
}
+
+func TestPartitionSpecUnmarshalRejectsInvalidStructure(t *testing.T) {
+ tests := []struct {
+ name string
+ data string
+ message string
+ }{
+ {
+ name: "negative spec ID",
+ data: `{"spec-id":-1,"fields":[]}`,
+ message: "spec ID must be non-negative",
+ },
+ {
+ name: "zero source ID",
+ data:
`{"spec-id":0,"fields":[{"source-id":0,"field-id":1000,"name":"part","transform":"identity"}]}`,
+ message: "source ID must be positive",
+ },
+ {
+ name: "empty source IDs",
+ data:
`{"spec-id":0,"fields":[{"source-ids":[],"field-id":1000,"name":"part","transform":"identity"}]}`,
+ message: "source ID must be positive",
+ },
+ {
+ name: "empty name",
+ data:
`{"spec-id":0,"fields":[{"source-id":1,"field-id":1000,"name":"","transform":"identity"}]}`,
+ message: "partition name cannot be empty",
+ },
+ {
+ name: "duplicate names",
+ data:
`{"spec-id":0,"fields":[{"source-id":1,"field-id":1000,"name":"part","transform":"identity"},{"source-id":2,"field-id":1001,"name":"part","transform":"identity"}]}`,
+ message: "duplicate partition name",
+ },
+ {
+ name: "duplicate field IDs",
+ data:
`{"spec-id":0,"fields":[{"source-id":1,"field-id":1000,"name":"first","transform":"identity"},{"source-id":2,"field-id":1000,"name":"second","transform":"identity"}]}`,
+ message: "duplicate field ID provided",
+ },
+ {
+ name: "redundant fields",
Review Comment:
This covers two identity fields on one source being rejected, but there's no
positive case for two `void` fields on the same source-id — which is exactly
the legal historical spec the redundancy check above
wrongly rejects. Add a case that unmarshals `{spec-id,
fields:[{source-id:1,field-id:1000,name:"a",transform:"void"},{source-id:1,field-id:1001,name:"b",transform:"void"}]}`
and asserts NoError, so the void exemption is locked in.
--
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]
