zeroshade commented on code in PR #1434: URL: https://github.com/apache/iceberg-go/pull/1434#discussion_r3560752663
########## table/compaction/pos_delete_collect.go: ########## @@ -0,0 +1,217 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package compaction + +import ( + "context" + "slices" + + "github.com/apache/iceberg-go" + iceio "github.com/apache/iceberg-go/io" + "github.com/apache/iceberg-go/table" +) + +// CollectDeadPositionDeletes walks the given snapshot's manifests and returns +// the position-delete files made dead by a rewrite that removes rewrittenPaths. +// A dead position delete references only data files the rewrite is removing, so +// after the rewrite it applies to nothing and is safe to expunge in the same +// commit. +// +// Scope resolution mirrors Java ContentFileUtil.referencedDataFile and +// DeleteFileIndex: +// +// - File-scoped — explicit referenced_data_file, or equal file_path +// lower/upper bounds — is dead iff its single target is in rewrittenPaths. +// - Partition-scoped — no single target — is dead iff no surviving +// (non-rewritten) data file in the same (specID, partition) has a sequence +// number <= the delete's. Such a survivor is one the delete still applies +// to, so the delete must be retained for a later dangling-delete pass. +// +// Deletion vectors (Puffin position deletes) are intentionally excluded: they +// are 1:1 with their data file and expunged per-group via +// [table.CompactionGroupResult.SafeDeletionVectors]. +// +// rewrittenPaths is the union of every old data file path being replaced across +// all rewrite groups. Like [CollectDeadEqualityDeletes], the returned files are +// safe to remove in the same commit that stages the rewrite: a concurrent +// commit cannot resurrect them, because any concurrent delete touching a +// rewritten file is rejected by the rewrite validator and any concurrent data +// file gets a sequence number strictly greater than every preexisting delete. +// +// Two-pass design: the first pass walks delete manifests to resolve candidates, +// deciding file-scoped ones immediately; the more expensive data-manifest walk +// runs only when a partition-scoped candidate needs a survivor check. +func CollectDeadPositionDeletes( Review Comment: question (not blocking): `CollectDeadPositionDeletes` is exported but has no in-repo caller yet, unlike `CollectDeadEqualityDeletes` which `cmd/iceberg/compact.go` feeds into `RewriteDataFilesOptions.ExtraDeleteFilesToRemove`. The title says "for leader-side expunge" — is that integration a planned follow-up, or is this meant purely as an API for external coordinators (like `CollectSafePositionDeletes`)? Either is fine; a one-line note on the intended consumer in the doc comment would help readers. ########## table/compaction/pos_delete_collect_ext_test.go: ########## @@ -0,0 +1,112 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package compaction_test + +import ( + "fmt" + "testing" + + "github.com/apache/iceberg-go" + iceio "github.com/apache/iceberg-go/io" + "github.com/apache/iceberg-go/table" + "github.com/apache/iceberg-go/table/compaction" + "github.com/stretchr/testify/require" +) + +// TestCollectDeadPositionDeletes drives the full manifest walk: a +// partition-scoped position delete (no referenced_data_file, no file_path +// bounds — Spark's default granularity) covering three data files is dead only +// when every file it covers is rewritten, and retained the moment one is not. +func TestCollectDeadPositionDeletes(t *testing.T) { + ctx := t.Context() + fs := iceio.LocalFS{} + tbl := newCDCStressTable(t) Review Comment: nit (test coverage): `newCDCStressTable` uses `UnpartitionedSpec`, so every data file and the delete collapse to the same `partitionBucketKey` (`"0:_"`). That means the partition-scoped survivor check — the subtlest part of `minSurvivorSeqByPartition` / `decideDeadPositionDeletes`, and the reason position deletes key on `(specID, partition)` — is never exercised end-to-end here. (`TestDecideDeadPositionDeletes` covers the decision with hand-built keys, but not key derivation or cross-partition exclusion.) Consider a partitioned-table case: a partition-scoped delete in partition A that stays alive while a same-partition survivor with `seq <= delete.seq` exists, and becomes dead once the only survivors live in a different partition. That's the behavior that distinguishes this from a naive implementation, so it's worth a regression guard. ########## table/compaction/compaction.go: ########## @@ -344,23 +344,38 @@ func fileScopedDeletedRows(t table.FileScanTask) int64 { } // isFileScoped reports whether a delete file applies to exactly one data file. -// Mirrors Java ContentFileUtil.isFileScoped/referencedDataFile: a deletion -// vector or path-scoped positional delete carries a referenced data file -// either explicitly or, when referenced_data_file is absent (it is optional in -// V2 and our own scan planning never sets it — see matchDeletesToData), through -// equal file_path lower/upper bounds. Equality deletes and partition-scoped -// positional deletes are not file-scoped. +// Equality deletes and partition-scoped positional deletes are not file-scoped. func isFileScoped(d iceberg.DataFile) bool { if d.ContentType() == iceberg.EntryContentEqDeletes { return false } - if d.ReferencedDataFile() != nil { - return true + + return referencedDataFilePath(d) != "" +} + +// referencedDataFilePath resolves the single data file a delete targets, or "" +// when it is partition-scoped (no single target). Mirrors Java +// ContentFileUtil.referencedDataFile: explicit referenced_data_file first, then +// equal file_path lower/upper bounds — the bounds fallback matters because +// referenced_data_file is optional in V2 and our own scan planning never sets +// it (see matchDeletesToData). +func referencedDataFilePath(d iceberg.DataFile) string { + if ref := d.ReferencedDataFile(); ref != nil && *ref != "" { + return *ref } + lower := d.LowerBoundValues()[filePathFieldID] upper := d.UpperBoundValues()[filePathFieldID] + if len(lower) == 0 || !bytes.Equal(lower, upper) { + return "" + } + + lit, err := iceberg.LiteralFromBytes(iceberg.PrimitiveTypes.String, lower) + if err != nil { + return "" + } - return len(lower) > 0 && bytes.Equal(lower, upper) + return lit.(iceberg.TypedLiteral[string]).Value() Review Comment: nit (defensiveness): this assertion panics if `lit` isn't a string-typed literal. It's safe today — `LiteralFromBytes(PrimitiveTypes.String, …)` always returns one — but since it decodes arbitrary `file_path` bounds from file metadata, the comma-ok form is more robust and consistent with the `err` check just above: ```go s, ok := lit.(iceberg.TypedLiteral[string]) if !ok { return "" } return s.Value() ``` ########## table/compaction/compaction.go: ########## @@ -344,23 +344,38 @@ func fileScopedDeletedRows(t table.FileScanTask) int64 { } // isFileScoped reports whether a delete file applies to exactly one data file. -// Mirrors Java ContentFileUtil.isFileScoped/referencedDataFile: a deletion -// vector or path-scoped positional delete carries a referenced data file -// either explicitly or, when referenced_data_file is absent (it is optional in -// V2 and our own scan planning never sets it — see matchDeletesToData), through -// equal file_path lower/upper bounds. Equality deletes and partition-scoped -// positional deletes are not file-scoped. +// Equality deletes and partition-scoped positional deletes are not file-scoped. func isFileScoped(d iceberg.DataFile) bool { if d.ContentType() == iceberg.EntryContentEqDeletes { return false } - if d.ReferencedDataFile() != nil { - return true + + return referencedDataFilePath(d) != "" +} + +// referencedDataFilePath resolves the single data file a delete targets, or "" +// when it is partition-scoped (no single target). Mirrors Java +// ContentFileUtil.referencedDataFile: explicit referenced_data_file first, then +// equal file_path lower/upper bounds — the bounds fallback matters because +// referenced_data_file is optional in V2 and our own scan planning never sets +// it (see matchDeletesToData). +func referencedDataFilePath(d iceberg.DataFile) string { + if ref := d.ReferencedDataFile(); ref != nil && *ref != "" { Review Comment: FYI (no action needed): folding `*ref != ""` in here slightly changes `isFileScoped` — a non-nil-but-empty `referenced_data_file` previously counted as file-scoped and now falls through to the `file_path` bounds. This is more correct, only affects the delete-ratio heuristic, and is captured by the new `TestReferencedDataFilePath` "empty referenced_data_file falls through to bounds" case. Noting for the record. -- 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]
