laskoviymishka commented on code in PR #1296:
URL: https://github.com/apache/iceberg-go/pull/1296#discussion_r3467418939
##########
table/compaction/compaction.go:
##########
@@ -279,3 +315,31 @@ func (cfg Config) isCandidate(t table.FileScanTask) bool {
// Undersized — candidate.
return true
}
+
+// fileScopedDeletedRows sums the record counts of the task's file-scoped
+// deletes — deletion vectors and path-scoped positional deletes. These apply
+// to exactly one data file, so their counts are attributable to it. The sum is
+// not capped here; callers cap at the data file's record count.
+func fileScopedDeletedRows(t table.FileScanTask) int64 {
+ var deleted int64
+ for _, d := range t.DeletionVectorFiles {
+ if isFileScoped(d) {
+ deleted += d.Count()
+ }
+ }
+ for _, d := range t.DeleteFiles {
+ if isFileScoped(d) {
+ deleted += d.Count()
+ }
+ }
+
+ return deleted
+}
+
+// isFileScoped reports whether a delete file applies to exactly one data file.
+// Mirrors Java ContentFileUtil.isFileScoped: deletion vectors and path-scoped
+// positional deletes carry a referenced data file; partition-scoped positional
+// deletes and equality deletes do not.
+func isFileScoped(d iceberg.DataFile) bool {
Review Comment:
This only catches DVs in practice, not the path-scoped positional deletes
the PR is really aiming at.
`referenced_data_file` is optional in V2 — the Java/PyIceberg/iceberg-go
pos-delete writers don't set it, they signal file-scope through equal
`file_path` lower/upper bounds. Java's `ContentFileUtil.referencedDataFile`
does a two-step check: return the explicit ref if present, else fall back to
`lower_bounds[file_path] == upper_bounds[file_path]`. We only do the first
step, so any V2 table accumulating Parquet pos-delete rows on right-sized files
never gets compacted by this rule — which is the dominant case it's meant to
handle. (In our own scan planning, `matchDeletesToData` never populates
`referenced_data_file` on `DeleteFiles`, so the path-scoped test only passes
because the fixture sets it.)
I'd add the bounds fallback, and while we're here drop out eq-deletes up
front to match Java (it returns null for them regardless):
```go
func isFileScoped(d iceberg.DataFile) bool {
if d.ContentType() == iceberg.EntryContentEqDeletes {
return false
}
if d.ReferencedDataFile() != nil {
return true
}
const filePathFieldID = 2147483546
lb := d.LowerBoundValues()[filePathFieldID]
ub := d.UpperBoundValues()[filePathFieldID]
return len(lb) > 0 && bytes.Equal(lb, ub)
}
```
##########
table/compaction/compaction.go:
##########
@@ -51,6 +52,14 @@ type Config struct {
// Default: 5.
DeleteFileThreshold int
+ // DeleteRatioThreshold forces a data file into compaction when the
fraction
+ // of its rows shadowed by a deletion vector reaches this value,
regardless
+ // of file size. Deletion vectors are 1:1 with their data file and
carry an
+ // exact deleted-row count, so size-based selection alone never
reclaims a
+ // right-sized file's DV dead space; this rule does. Range [0,1]; 0
disables
+ // it. Default: DefaultDeleteRatioThreshold.
+ DeleteRatioThreshold float64
Review Comment:
The doc says "deletion vector" throughout, but the rule (correctly) also
counts path-scoped positional deletes via `fileScopedDeletedRows`. I'd broaden
the wording to "file-scoped deletes (deletion vectors and path-scoped
positional deletes)" so the scope isn't misleading — same applies to the
`DefaultDeleteRatioThreshold` const comment below.
##########
table/compaction/compaction.go:
##########
@@ -259,13 +276,32 @@ func (cfg Config) PlanCompaction(tasks
[]table.FileScanTask) (Plan, error) {
// isCandidate returns true if a file should be considered for compaction.
func (cfg Config) isCandidate(t table.FileScanTask) bool {
size := t.File.FileSizeBytes()
- deleteCount := len(t.DeleteFiles) + len(t.EqualityDeleteFiles)
+ deleteCount := len(t.DeleteFiles) + len(t.EqualityDeleteFiles) +
len(t.DeletionVectorFiles)
Review Comment:
I'm of two minds on folding DVs into `deleteCount`. It matches Java's
unified `task.deletes()` list, so there's a parity argument for it.
But the count rule is for "lots of separate delete files piling up," and a
file has at most one DV — so at `DeleteFileThreshold=1` (the minimum valid
value) a single DV trips the count rule before the ratio rule ever runs, which
makes the ratio rule dead code for DV-backed files at that threshold and
inflates `group.DeleteFileCount` by 1 per DV-backed file for downstream
consumers. Was the intent to have both signals fire on DVs, or should DVs stay
out of the count and be owned solely by the ratio rule? wdyt?
##########
table/compaction/compaction.go:
##########
@@ -21,6 +21,7 @@ package compaction
import (
"fmt"
+ iceberg "github.com/apache/iceberg-go"
Review Comment:
The alias is an identity alias (the package is already named `iceberg`), and
every other file in this package imports it unaliased — I'd drop it for
consistency.
##########
table/compaction/compaction.go:
##########
@@ -123,6 +137,9 @@ func (cfg Config) Validate() error {
if cfg.DeleteFileThreshold < 1 {
return fmt.Errorf("delete file threshold must be >= 1, got %d",
cfg.DeleteFileThreshold)
}
+ if cfg.DeleteRatioThreshold < 0 || cfg.DeleteRatioThreshold > 1 {
+ return fmt.Errorf("delete ratio threshold must be in [0,1], got
%v", cfg.DeleteRatioThreshold)
Review Comment:
`%g` reads better than `%v` for a float64 here, and it's closer to the
sibling `%d` int errors above in being explicit about the type.
##########
table/compaction/compaction_test.go:
##########
@@ -261,6 +308,109 @@ func TestPlanCompaction_DeleteFilesForcesCompaction(t
*testing.T) {
assert.Equal(t, 25, totalDeletes)
}
+// dvRatioConfig sizes files (500 MB) well under target (2600 MB) so candidates
+// co-pack into one bin, while keeping each file right-sized (>= 384 MB min) so
+// size-based selection alone would skip it — isolating the deletion-vector
+// ratio as the only thing that can force compaction.
+func dvRatioConfig() compaction.Config {
+ return compaction.Config{
+ TargetFileSizeBytes: 2600 * 1024 * 1024,
+ MinFileSizeBytes: 384 * 1024 * 1024,
+ MaxFileSizeBytes: 3000 * 1024 * 1024,
+ MinInputFiles: 2,
+ DeleteFileThreshold: 5,
+ DeleteRatioThreshold: 0.3,
Review Comment:
`dvRatioConfig` hardcodes `0.3` while `PackingLookback` just above uses the
exported const. If the default ever moves, this helper silently keeps testing
the old value — I'd use `compaction.DefaultDeleteRatioThreshold` here too.
##########
table/compaction/compaction.go:
##########
@@ -85,19 +94,24 @@ const (
// DefaultPackingLookback is the default packing lookback.
DefaultPackingLookback uint = 128
+
+ // DefaultDeleteRatioThreshold mirrors Java's delete-ratio default: a
file
+ // whose deletion vector shadows at least 30% of its rows is rewritten.
+ DefaultDeleteRatioThreshold = 0.3
Review Comment:
The siblings are explicitly typed (`DefaultMinInputFiles uint = 5`,
`DefaultPackingLookback uint = 128`). I'd make this `float64` too —
`DefaultDeleteRatioThreshold float64 = 0.3` — otherwise it's an untyped
constant in a block of typed ones.
--
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]
