laskoviymishka commented on code in PR #1593:
URL: https://github.com/apache/iceberg-go/pull/1593#discussion_r3681123891
##########
partitions.go:
##########
@@ -469,6 +488,30 @@ 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{}{}
+
+ if _, ok := field.Transform.(VoidTransform); ok {
+ continue
+ }
+
+ definition := fmt.Sprintf("%v:%s", field.SourceIDs,
field.Transform)
Review Comment:
This keys redundancy off `Transform.String()`, which assumes `String()` is
injective per transform. The constructor path uses `Transform.Equals()`, so we
now have two independent notions of "same field" that can drift — a shared
helper both paths call would keep the semantics in one place.
It also carries forward a pre-existing gap rather than adding one: Java's
`dedupName()` collapses year/month/day/hour to `"time"` and rejects e.g.
`year(ts)` + `month(ts)` on the same column, whereas both our paths accept it.
Not this PR's job to fix, just flagging. (Minor: the `fmt.Sprintf` might trip
perfsprint depending on the version pinned in CI — worth a quick check.)
##########
partitions_test.go:
##########
@@ -647,10 +647,73 @@ 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")
Review Comment:
The subtest name still says "unmarshal with no source id" (describing the
input) but the outcome flipped from pass to reject — I'd rename it to something
like "...is rejected" so the intent is obvious.
Also, whichever way the void decision lands above, there's no positive case
that a void field *with* a source-id still unmarshals cleanly — worth adding
one asserting `SourceID() == 1`.
##########
partitions.go:
##########
@@ -143,10 +145,21 @@ func (p *PartitionField) UnmarshalJSON(b []byte) error {
} else {
p.SourceIDs = []int{aux.SourceID}
}
+ for _, sourceID := range p.SourceIDs {
Review Comment:
`{"source-ids":[]}` slips past the else-if above (the key is present), then
falls through to `p.SourceIDs = []int{aux.SourceID}` = `[]int{0}`, so this loop
reports "must be positive: 0" — which points at the wrong thing; the real
problem is the empty array. PyIceberg rejects this explicitly with "Empty
source-ids is not allowed."
The "empty source IDs" test asserts on that "must be positive" message, so
it's green only because of this side effect — if we ever allowed source-id 0
it'd silently regress. I'd add an explicit empty-array check with its own
message and update the test to match.
##########
partitions.go:
##########
@@ -121,6 +121,8 @@ func (p *PartitionField) UnmarshalJSON(b []byte) error {
if _, ok := raw["source-ids"]; ok {
return errors.New("partition field cannot contain both
source-id and source-ids")
Review Comment:
This one returns a bare `errors.New`, but the new branch just below wraps
`ErrInvalidPartitionSpec`. So `errors.Is(err, ErrInvalidPartitionSpec)` is true
for one source-id problem and false for the other, which callers will trip
over. I'd wrap this one too while we're here.
##########
partitions.go:
##########
@@ -121,6 +121,8 @@ func (p *PartitionField) UnmarshalJSON(b []byte) error {
if _, ok := raw["source-ids"]; ok {
return errors.New("partition field cannot contain both
source-id and source-ids")
}
+ } else if _, ok := raw["source-ids"]; !ok {
+ return fmt.Errorf("%w: partition field requires source-id or
source-ids", ErrInvalidPartitionSpec)
Review Comment:
This branch rejects any field carrying neither source-id nor source-ids, and
the changed test now expects `void` with no source-id to fail. void is the
tombstone we write for dropped partition fields, and we used to accept it
without a source column — the old test asserted `field.SourceID() == 0` on
exactly this input. So this turns a documented contract into a hard parse
failure for any metadata that already has a source-id-less void field.
@tanmayrauth already called this out and I agree it's the crux. The spec
does cut the other way — Java's `buildFromJsonFields` and PyIceberg both
require source-id — so I don't think "accept everything" is obviously right
either. I'd make it an explicit decision: either a void carve-out that
synthesizes a sentinel source-id when neither key is present, or keep the hard
reject but land it with a changelog/migration note. Either is fine, but it
shouldn't be a side effect of this branch. wdyt?
##########
table/metadata_internal_test.go:
##########
@@ -564,6 +564,25 @@ func TestRejectInvalidSchemaEntries(t *testing.T) {
})
}
+func TestRejectStructurallyInvalidHistoricalPartitionSpec(t *testing.T) {
Review Comment:
This is named for historical specs but only exercises the negative spec-id
rejection. The PR's rationale is that historical specs may still reference
dropped source columns — that's the case I'd most want pinned down: load a
two-spec history where the earlier spec has an identity transform on a column
that's gone from the later schema, and assert it still parses. Right now
nothing locks that 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]
