laskoviymishka commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3987734230
##########
table/metadata.go:
##########
@@ -252,9 +252,9 @@ type Metadata interface {
// DefaultPartitionSpec is the ID of the current spec that
writerFactory should
// use by default.
DefaultPartitionSpec() int
- // LastPartitionSpecID is the highest assigned partition field ID across
- // all partition specs for the table. This is used to ensure partition
- // fields are always assigned an unused ID when evolving specs.
+ // LastPartitionSpecID returns the persisted last assigned partition
field ID.
+ // Allocation also scans partition spec history because metadata
written by
Review Comment:
This second sentence describes `partitionFieldIDFloor`, not this method.
`LastPartitionSpecID` just returns the stored `*int` and scans nothing.
I'd keep the doc focused on what it returns (the persisted counter, which
may be stale) and move the "allocation scans spec history" note down onto
`partitionFieldIDFloor`, which has no doc today. That way a `Metadata`
implementer reading this knows the value can be stale and that allocation needs
the floor helper rather than this value alone.
##########
table/metadata_builder_internal_test.go:
##########
@@ -357,6 +357,33 @@ func TestAddRemovePartitionSpec(t *testing.T) {
require.ErrorContains(t, err, "id 1")
}
+func TestAddPartitionSpecAllocatesAfterHistoricalFieldID(t *testing.T) {
+ data := strings.Replace(ExampleTableMetadataV2,
+ `"last-partition-id": 1000`, `"last-partition-id": 999`, 1)
+ require.Contains(t, data, `"last-partition-id": 999`)
+
+ metadata, err := ParseMetadataBytes([]byte(data))
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(metadata, "")
+ require.NoError(t, err)
+
+ addedSpec, err := iceberg.NewPartitionSpecOpts(
+ iceberg.WithSpecID(1),
+ iceberg.AddPartitionFieldBySourceID(1, "x_bucket",
iceberg.BucketTransform{NumBuckets: 16}, builder.CurrentSchema(), nil),
+ )
+ require.NoError(t, err)
+ require.NoError(t, builder.AddPartitionSpec(&addedSpec, false))
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ added := rebuilt.PartitionSpecByID(1)
+ require.NotNil(t, added)
+ require.Equal(t, 1, added.NumFields())
+ assert.Equal(t, 1001, added.Field(0).FieldID)
+ require.NotNil(t, rebuilt.LastPartitionSpecID())
+ assert.Equal(t, 1001, *rebuilt.LastPartitionSpecID())
Review Comment:
This covers the stale-999 path nicely. The one branch it doesn't reach is
nil `lastPartitionID` with a spec above the start, a V1 table, or metadata with
no last-partition-id. If someone drops the spec-scan loop in the helper, that
branch would regress and this test would still pass.
Could we add a case with `b.lastPartitionID = nil` and a spec containing
`field-id` 1000, asserting the next allocated ID is 1001?
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema
*iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs
[]iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
+ if lastPartitionID != nil {
+ floor = max(floor, *lastPartitionID)
+ }
+ for _, spec := range specs {
+ floor = max(floor, spec.LastAssignedFieldID())
+ }
+
+ return floor
+}
+
func (b *MetadataBuilder) AddPartitionSpec(spec *iceberg.PartitionSpec,
initial bool) error {
newSpecID := b.reuseOrCreateNewPartitionSpecID(*spec)
curSchema := b.CurrentSchema()
if curSchema == nil {
return errors.New("can't add sort order with no current schema")
}
- freshSpec, err := spec.BindToSchema(curSchema, b.lastPartitionID,
&newSpecID)
+ fieldIDFloor := partitionFieldIDFloor(b.lastPartitionID, b.specs)
+ freshSpec, err := spec.BindToSchema(curSchema, &fieldIDFloor,
&newSpecID)
Review Comment:
The floor made it into `BindToSchema`, but the `b.lastPartitionID`
write-back a few lines down still uses `max(maxFieldID, prev)` with `prev =
*b.lastPartitionID`. For a non-empty spec that's fine, `maxFieldID` heals it.
For an empty spec (unpartition) `maxFieldID` is 0 and `prev` is the stale 999,
so the counter we serialize stays stale, and a reader that trusts it without
scanning specs can still allocate 1000 and collide, the exact case this PR is
closing on the `NewUpdateSpec` path.
`fieldIDFloor` is already in scope and always `>= prev`, so:
```go
lastPartitionID := max(maxFieldID, fieldIDFloor)
```
The `prev` block and its nil-guard fall out as dead code, and the assertion
value is unaffected since that's drawn from the base `Metadata`. wdyt?
##########
table/metadata.go:
##########
@@ -700,14 +700,27 @@ func (b *MetadataBuilder) AddSchema(schema
*iceberg.Schema) error {
return nil
}
+func partitionFieldIDFloor(lastPartitionID *int, specs
[]iceberg.PartitionSpec) int {
+ floor := partitionFieldStartID - 1
Review Comment:
`partitionFieldStartID` and `iceberg.PartitionDataIDStart` are both 1000,
but the `assignMissingPartitionFieldIDs` / nil-guard paths use the latter and
there's no compile-time link between them. This helper is now a third call site
where a silent divergence would produce a wrong floor. I'd use
`iceberg.PartitionDataIDStart - 1` here so they can't drift.
--
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]
