mattfaltyn commented on code in PR #1988:
URL: https://github.com/apache/iceberg-go/pull/1988#discussion_r3987845556
##########
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:
> “Could we add a case with `b.lastPartitionID = nil` and a spec containing
`field-id` 1000?”
Added. The nil-counter subtest confirms the next field ID is 1001. Thanks!
##########
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:
> “Use `iceberg.PartitionDataIDStart - 1` here so they cannot drift.”
Updated. The helper now uses the canonical Iceberg constant. Nice catch!
##########
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:
> “`fieldIDFloor` is already in scope and always `>= prev`.”
Updated. Write-back now uses `max(maxFieldID, fieldIDFloor)`, with an
empty-spec regression test. Thanks!
##########
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.”
Updated. The method doc now focuses on the persisted counter, and the
history scan is documented on the helper. Thanks!
--
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]
