You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "twuebi (via GitHub)" <gi...@apache.org> on 2026/06/24 12:23:29 UTC
[PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
twuebi opened a new pull request, #1296:
URL: https://github.com/apache/iceberg-go/pull/1296
The candidate planner only counted positional and equality delete files toward delete pressure, so a right-sized data file accumulating file-scoped deletes (a deletion vector, or a path-scoped positional delete) was never rewritten — its dead rows persisted as read amplification indefinitely.
Add a DeleteRatioThreshold (default 0.3, mirroring Java's delete-ratio-threshold). A file is forced into compaction when its file-scoped deletes shadow at least that fraction of its rows. Following Java's BinPackRewriteFilePlanner.tooHighDeleteRatio, the deleted-row count sums the record counts of file-scoped deletes only — deletion vectors and path-scoped positional deletes (those carrying a referenced data file), identified by isFileScoped (= referenced data file present, matching ContentFileUtil.isFileScoped) — capped at the data file's record count. Equality deletes and partition-scoped positional deletes are not file-scoped and stay under the existing count rule, to which deletion vectors are now also added.
Unlike Java, a threshold of 0 disables the rule so a zero-valued Config stays valid; the default remains 0.3.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1296:
URL: https://github.com/apache/iceberg-go/pull/1296#discussion_r3467836624
##########
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:
went for parity with java, which as far as i understand unifies deletes, so a single dv may trip the count rule, but at default 5 it wouldn't. happy to go the other way if you prefer
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1296:
URL: https://github.com/apache/iceberg-go/pull/1296#discussion_r3467891933
##########
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:
fixed
##########
table/compaction/compaction.go:
##########
@@ -21,6 +21,7 @@ package compaction
import (
"fmt"
+ iceberg "github.com/apache/iceberg-go"
Review Comment:
dropped
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1296:
URL: https://github.com/apache/iceberg-go/pull/1296#discussion_r3467894713
##########
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:
fixed
##########
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:
fixed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1296:
URL: https://github.com/apache/iceberg-go/pull/1296#discussion_r3467820583
##########
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:
done, added the eq delete short circuit and equal file path fallback & added a test exercising the fallback
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1296:
URL: https://github.com/apache/iceberg-go/pull/1296#discussion_r3467887134
##########
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:
broadened to "file-scoped deletes"
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(compaction): force compaction on file-scoped delete dead space [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1296:
URL: https://github.com/apache/iceberg-go/pull/1296
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org