zeroshade commented on code in PR #1992:
URL: https://github.com/apache/iceberg-go/pull/1992#discussion_r3937636792
##########
table/transaction.go:
##########
@@ -1214,14 +1273,14 @@ func (t *Transaction)
validateDeleteFilesToAdd(deleteFiles []rewriteDeleteFileAd
return nil, fmt.Errorf("position delete file %s
must be a deletion vector for v%d table for %s",
path, meta.formatVersion, operation)
}
- if _, ok := setToAdd.regularPaths[path]; ok {
+ if pathAlreadyAdded {
+ if _, ok := setToAdd.dvPaths[path]; ok {
Review Comment:
**major** — Restructured delete-file path uniqueness/conflict checks are
unpinned by any test
The PR replaces the dedicated `regularPaths` set with state derived from
`paths` + `dvPaths` via `pathAlreadyAdded` (transaction.go:1236, 1276, 1324)
and rewrites the matching guard in replaceFiles (transaction.go:1976). I proved
the rewrite is semantically equivalent, but nothing in the test suite pins it:
each of the three checks can be deleted outright and `go test ./table` still
passes. No test in ./table asserts the strings 'add delete file paths must be
unique' or 'cannot identify both a deletion vector container and a regular
delete file'. The PR adds regression tests only for the partition-tuple half.
Please add cases covering (a) two regular delete files with the same path, (b)
a DV and an equality delete sharing a path in both orders on a v3 table, and
(c) the replaceFiles collision path, so a future refactor of this derived state
cannot silently drop a uniqueness guarantee.
<details><summary>Evidence</summary>
```text
Mutation runs with probe files removed: M1 (delete `if pathAlreadyAdded
{...}` block at :1276) -> `ok github.com/apache/iceberg-go/table 8.198s`; M2
(delete DV-branch conflict block at :1324) -> `ok ... 6.472s`; M3 (delete
replaceFiles block at :1976) -> `ok ... 5.763s`. Control M4 (force `if true` on
the partition early return) -> `--- FAIL:
TestPartitionValidationPlanPreservesPartitionValidationErrors`, confirming the
new partition test is non-vacuous and the harness detects mutations. grep of
./table/*_test.go for both delete-path error strings returns zero matches.
```
</details>
##########
table/transaction.go:
##########
@@ -1018,28 +1018,84 @@ func (t *Transaction) ReplaceDataFiles(ctx
context.Context, filesToDelete, files
return t.apply(updates, reqs)
}
-// validateDataFilePartitionData verifies that DataFile partition values match
-// the given partition spec's fields by ID without reading file contents.
-func validateDataFilePartitionData(df iceberg.DataFile, spec
*iceberg.PartitionSpec) error {
- partitionData := dataFilePartition(df)
+type partitionValidationField struct {
+ id int
+ name string
+}
+
+// partitionValidationPlan contains the metadata needed to validate
+// a data file's partition tuple against one partition spec. The field slice
+// preserves spec order for deterministic missing-field errors. The lookup map
+// for unknown fields is built only when a malformed tuple needs that check.
+type partitionValidationPlan struct {
+ specID int
+ fields []partitionValidationField
+ expectedFieldCount int
+ expectedFieldIDs map[int]string
+}
+
+func newPartitionValidationPlan(spec *iceberg.PartitionSpec)
*partitionValidationPlan {
+ plan := &partitionValidationPlan{
+ specID: spec.ID(),
+ fields: make([]partitionValidationField, 0, spec.NumFields()),
+ }
- expectedFieldIDs := make(map[int]string)
for _, field := range spec.Fields() {
- expectedFieldIDs[field.FieldID] = field.Name
- if _, ok := partitionData[field.FieldID]; !ok {
- return fmt.Errorf("missing partition value for field id
%d (%s)", field.FieldID, field.Name)
+ duplicateID := false
+ for _, expected := range plan.fields {
+ if expected.id == field.FieldID {
+ duplicateID = true
+
+ break
+ }
+ }
+ if !duplicateID {
+ plan.expectedFieldCount++
+ }
+ plan.fields = append(plan.fields, partitionValidationField{
+ id: field.FieldID,
+ name: field.Name,
+ })
+ }
+
+ return plan
+}
+
+func (p *partitionValidationPlan) validate(df iceberg.DataFile) error {
+ partitionData := dataFilePartition(df)
+
+ for _, field := range p.fields {
+ if _, ok := partitionData[field.id]; !ok {
+ return fmt.Errorf("missing partition value for field id
%d (%s)", field.id, field.name)
+ }
+ }
+
+ if len(partitionData) == p.expectedFieldCount {
+ return nil
+ }
+
+ if p.expectedFieldIDs == nil {
+ p.expectedFieldIDs = make(map[int]string, len(p.fields))
+ for _, field := range p.fields {
+ p.expectedFieldIDs[field.id] = field.name
}
}
for fieldID := range partitionData {
- if _, ok := expectedFieldIDs[fieldID]; !ok {
- return fmt.Errorf("unknown partition field id %d for
spec id %d", fieldID, spec.ID())
+ if _, ok := p.expectedFieldIDs[fieldID]; !ok {
+ return fmt.Errorf("unknown partition field id %d for
spec id %d", fieldID, p.specID)
}
}
return nil
}
+// validateDataFilePartitionData verifies that DataFile partition values match
+// the given partition spec's fields by ID without reading file contents.
+func validateDataFilePartitionData(df iceberg.DataFile, spec
*iceberg.PartitionSpec) error {
+ return newPartitionValidationPlan(spec).validate(df)
Review Comment:
**minor** — validateDataFilePartitionData is now dead code
After the refactor the function is a one-line wrapper (`return
newPartitionValidationPlan(spec).validate(df)`) with no callers anywhere in the
module, including tests. Go does not reject unused package-level funcs so the
build stays green. Either delete it, or if it is intended as the single-file
convenience entry point, use it from a test so it cannot rot.
##########
table/transaction.go:
##########
@@ -1018,28 +1018,84 @@ func (t *Transaction) ReplaceDataFiles(ctx
context.Context, filesToDelete, files
return t.apply(updates, reqs)
}
-// validateDataFilePartitionData verifies that DataFile partition values match
-// the given partition spec's fields by ID without reading file contents.
-func validateDataFilePartitionData(df iceberg.DataFile, spec
*iceberg.PartitionSpec) error {
- partitionData := dataFilePartition(df)
+type partitionValidationField struct {
+ id int
+ name string
+}
+
+// partitionValidationPlan contains the metadata needed to validate
+// a data file's partition tuple against one partition spec. The field slice
+// preserves spec order for deterministic missing-field errors. The lookup map
+// for unknown fields is built only when a malformed tuple needs that check.
+type partitionValidationPlan struct {
+ specID int
+ fields []partitionValidationField
+ expectedFieldCount int
+ expectedFieldIDs map[int]string
+}
+
+func newPartitionValidationPlan(spec *iceberg.PartitionSpec)
*partitionValidationPlan {
+ plan := &partitionValidationPlan{
+ specID: spec.ID(),
+ fields: make([]partitionValidationField, 0, spec.NumFields()),
+ }
- expectedFieldIDs := make(map[int]string)
for _, field := range spec.Fields() {
- expectedFieldIDs[field.FieldID] = field.Name
- if _, ok := partitionData[field.FieldID]; !ok {
- return fmt.Errorf("missing partition value for field id
%d (%s)", field.FieldID, field.Name)
+ duplicateID := false
+ for _, expected := range plan.fields {
+ if expected.id == field.FieldID {
+ duplicateID = true
+
+ break
+ }
+ }
+ if !duplicateID {
+ plan.expectedFieldCount++
+ }
+ plan.fields = append(plan.fields, partitionValidationField{
+ id: field.FieldID,
+ name: field.Name,
+ })
+ }
+
+ return plan
+}
+
+func (p *partitionValidationPlan) validate(df iceberg.DataFile) error {
+ partitionData := dataFilePartition(df)
+
+ for _, field := range p.fields {
+ if _, ok := partitionData[field.id]; !ok {
+ return fmt.Errorf("missing partition value for field id
%d (%s)", field.id, field.name)
+ }
+ }
+
+ if len(partitionData) == p.expectedFieldCount {
+ return nil
+ }
+
+ if p.expectedFieldIDs == nil {
+ p.expectedFieldIDs = make(map[int]string, len(p.fields))
Review Comment:
**minor** — validate() mutates the shared cached plan for no measurable gain
`(*partitionValidationPlan).validate` lazily populates `p.expectedFieldIDs`,
making a method that reads as pure into a mutator on a struct whose entire
purpose is to be cached and reused (validationPlansByID). It is safe today
because plans are function-local and the loops are sequential, but 'reuse plans
across files' is one step from 'reuse plans across a transaction or across
goroutines', at which point this is a silent data race that -race will only
catch by luck. I measured the payoff: building the map eagerly in
newPartitionValidationPlan costs 2 extra allocs per 128-file batch and is not
slower. Suggest building it eagerly (it is O(numFields), once per spec) or
documenting the type as not safe for concurrent use.
--
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]
