fix(table): prevent index out of range error in buildManifestEvaluator

[ErenDursun]

fixes #691

Previously, the buildManifestEvaluator method directly accessed the PartitionSpecs array by index without validating that the requested specID actually exists in the partition specs. This could cause an index out of bounds error.

This change adds a proper lookup using slices.IndexFunc to find the partition spec by its ID and returns an error if the spec is not found, improving error handling and robustness.

[zeroshade]

Would it be relatively easy to construct a contrived test that would have triggered the out of range error? Just to ensure we don't introduce this back later

[ErenDursun]

@zeroshade I've added a short unit test.
We also received an answer on apache/iceberg#15108 confirming it is not required that metadata has to contain the complete history of all partition specs. Therefore I also removed the wrapped ErrInvalidMetadata from the error I added - it might have been misleading. Hope you agree.

[ErenDursun]

Also apache/iceberg#15108 (comment) suggests the partition specs should be tracked in a map<spec id, spec>. But that would necessitate a change in the Metadata interface, which is user-facing and thus a breaking change.

iceberg-go/table/metadata.go

Line 92 in ad086d2

PartitionSpecs() []iceberg.PartitionSpec

[zeroshade]

But that would necessitate a change in the Metadata interface, which is user-facing and thus a breaking change.

We are still pre-version 1. Thus I'm okay with a breaking change here. These sort of situations are precisely why we haven't hit version 1.0 and called it stable yet. Just make sure we're very clear in the commit notice about the breaking change (so the release notes make it clear)

[zeroshade]

Alternately, we could store it as a map internally and just construct the slice in the function on the fly (maps.Values) which might make sense to avoid the breaking change and ensure that a user can't change the internal map on us

[ErenDursun]

Just had a closer look into it and it seems the field Specs is a list

iceberg-go/table/metadata.go

Line 1210 in 3ce3b4b

Specs []iceberg.PartitionSpec `json:"partition-specs"`

because it is defined as such in the Iceberg docs:

partition-specs: A list of partition specs, stored as full partition spec objects.

Changing its type in the commonMetadata struct would interfere with the (un)marshalling logic and changing it in the (user facing) Metadata interface would let it diverge from the Iceberg specs. I could instead add an internal field specsByID map[int]iceberg.PartitionSpec and an internal method partitionSpecsByID() map[int]iceberg.PartitionSpec with lazy-loading logic. That way the methods

iceberg-go/table/metadata.go

Lines 1316 to 1324 in 3ce3b4b

	func (c *commonMetadata) PartitionSpec() iceberg.PartitionSpec {
	for _, s := range c.Specs {
	if s.ID() == c.DefaultSpecID {
	return s
	}
	}
	return *iceberg.UnpartitionedSpec
	}

and

iceberg-go/table/metadata.go

Lines 1430 to 1439 in 3ce3b4b

	func (c *commonMetadata) checkPartitionSpecs() error {
	for _, spec := range c.Specs {
	if spec.ID() == c.DefaultSpecID {
	return nil
	}
	}
	return fmt.Errorf("%w: default-spec-id %d can't be found",
	ErrInvalidMetadata, c.DefaultSpecID)
	}

could use the map for better performance.

If we'd also add the new internal lazy-loading method to the Metadata interface, users wouldn't be affected but all these methods could use the map instead:

iceberg-go/table/scanner.go

Lines 244 to 249 in 8ff2e91

	func (scan *Scan) buildPartitionProjection(specID int) (iceberg.BooleanExpression, error) {
	project := newInclusiveProjection(scan.metadata.CurrentSchema(),
	scan.metadata.PartitionSpecs()[specID], true)
	return project(scan.rowFilter)
	}

iceberg-go/table/scanner.go

Lines 251 to 256 in 8ff2e91

	func (scan *Scan) buildManifestEvaluator(specID int) (func(iceberg.ManifestFile) (bool, error), error) {
	spec := scan.metadata.PartitionSpecs()[specID]
	return newManifestEvaluator(spec, scan.metadata.CurrentSchema(),
	scan.partitionFilters.Get(specID), scan.caseSensitive)
	}

iceberg-go/table/scanner.go

Lines 258 to 272 in 8ff2e91

	func (scan *Scan) buildPartitionEvaluator(specID int) func(iceberg.DataFile) (bool, error) {
	spec := scan.metadata.PartitionSpecs()[specID]
	partType := spec.PartitionType(scan.metadata.CurrentSchema())
	partSchema := iceberg.NewSchema(0, partType.FieldList...)
	partExpr := scan.partitionFilters.Get(specID)
	return func(d iceberg.DataFile) (bool, error) {
	fn, err := iceberg.ExpressionEvaluator(partSchema, partExpr, scan.caseSensitive)
	if err != nil {
	return false, err
	}
	return fn(getPartitionRecord(d, partType))
	}
	}

iceberg-go/table/update_spec.go

Lines 407 to 411 in 8ff2e91

	func (us *UpdateSpec) isNewPartitionSpec(newSpecId int) bool {
	return !slices.ContainsFunc(us.txn.tbl.Metadata().PartitionSpecs(), func(s iceberg.PartitionSpec) bool {
	return s.ID() == newSpecId
	})
	}

Buuuut... I'm not sure if the minor performance boost justifies the additional lazy-loading complexity 😅 there's the risk of future changes to the code leading to diverging states between the partition-spec list and map. Should I give it a try or do we keep the lists and just prevent the out of range errors for now?

[ErenDursun]

@zeroshade the methods buildPartitionProjection and buildPartitionEvaluator had the same issue. Added a fix and unit tests for those two as well.

[zeroshade]

Thanks for this, looks good!

Buuuut... I'm not sure if the minor performance boost justifies the additional lazy-loading complexity 😅 there's the risk of future changes to the code leading to diverging states between the partition-spec list and map. Should I give it a try or do we keep the lists and just prevent the out of range errors for now?

I think we should keep the lists and prevent the out of range errors for now, and we can look into switching to the proper map approach/lazy-loading/etc. as a separate PR.

I've got just one nitpick question for you, otherwise this is good to go!

[zeroshade]

Thanks for reviewing @ferhatelmas, any further comments/issues on this?

[ferhatelmas]

My pleasure. Overall looks good to me. #692 (comment) is the only concern but not a blocker to merge this PR I think

[zeroshade]

I agree with your statement there, it'd be better to address all at once and for now let's keep the consistency