laskoviymishka commented on code in PR #1392:
URL: https://github.com/apache/iceberg-go/pull/1392#discussion_r3535328489
##########
table/rewrite_data_files.go:
##########
@@ -301,23 +301,17 @@ func ExecuteCompactionGroup(ctx context.Context, tbl
*Table, group CompactionTas
}
// Preserve row lineage only when every source file in the group carries
- // it. A mixed group (some files with FirstRowID, some without — e.g.
- // legacy files on a v3 table) would otherwise produce one output where
- // post-lineage rows have explicit _row_id values and pre-lineage rows
- // have nulls, which violates the per-file uniqueness/coverage
- // invariant the v3 spec requires. Splitting mixed groups into separate
- // outputs is a larger refactor and is left as a follow-up; for now we
- // degrade gracefully (the rewrite still succeeds, but lineage is not
- // preserved for the surviving rows).
+ // it. A mixed group (some files with FirstRowID, some without) would
+ // otherwise produce one output where some rows keep explicit _row_id
+ // values and others do not, which violates the per-file uniqueness and
+ // coverage invariants the v3 spec requires. Spec-compliant v3 planning
Review Comment:
I think this framing is spec-incorrect for the migration case, and it's the
one that landed. Pre-upgrade snapshots are spec-valid with null `first_row_id`
— the spec assigns row IDs lazily "in the first commit after upgrading to v3,"
at manifest-list write time (see `manifest.go` ~1558). A file carried forward
from v2 that hasn't yet been through a v3 manifest-list write has a
legitimately nil `FirstRowID` (scanner copies it through at `scanner.go:805`;
`inheritRowIDs` is false when the value is nil, `manifest.go:720`).
So a mixed group here is the common v1/v2→v3 migration case, not "invalid
metadata or caller-constructed input" — and the drop-lineage branch below fires
on a normal workload. The comment this replaced said exactly that. I'd restore
the accurate framing: mixed groups are expected during migration, row IDs get
assigned at the next v3 manifest-list write, and until then lineage is dropped
for the merged output. wdyt?
##########
table/rewrite_data_files.go:
##########
@@ -301,23 +301,17 @@ func ExecuteCompactionGroup(ctx context.Context, tbl
*Table, group CompactionTas
}
// Preserve row lineage only when every source file in the group carries
- // it. A mixed group (some files with FirstRowID, some without — e.g.
- // legacy files on a v3 table) would otherwise produce one output where
- // post-lineage rows have explicit _row_id values and pre-lineage rows
- // have nulls, which violates the per-file uniqueness/coverage
- // invariant the v3 spec requires. Splitting mixed groups into separate
- // outputs is a larger refactor and is left as a follow-up; for now we
- // degrade gracefully (the rewrite still succeeds, but lineage is not
- // preserved for the surviving rows).
+ // it. A mixed group (some files with FirstRowID, some without) would
+ // otherwise produce one output where some rows keep explicit _row_id
+ // values and others do not, which violates the per-file uniqueness and
+ // coverage invariants the v3 spec requires. Spec-compliant v3 planning
+ // should already inherit first_row_id for every task, including files
+ // carried forward across a v2->v3 upgrade, so a mixed group here is
+ // treated conservatively as invalid metadata or caller-constructed
input.
preserveLineage := tbl.metadata.Version() >= 3 &&
allTasksHaveRowLineage(group.Tasks)
if preserveLineage {
scanOpts = append(scanOpts, WithRowLineage())
} else if tbl.metadata.Version() >= 3 {
Review Comment:
Dropping the inline comment on this branch leaves the `if lineageFiles > 0`
guard below looking like a bug to a fresh reader — it's asymmetric
(`lineageFiles`, not `legacyFiles`), and an all-legacy v3 group falls through
silently with no log. I'd keep a condensed comment here explaining the actual
action: lineage is dropped for the whole group and a `slog.Warn` is emitted
only when at least one source file actually carried lineage, rather than
returning an error or splitting the group.
##########
table/rewrite_data_files.go:
##########
@@ -327,7 +321,7 @@ func ExecuteCompactionGroup(ctx context.Context, tbl
*Table, group CompactionTas
}
}
if lineageFiles > 0 {
- slog.Warn("compaction group has mixed row lineage;
dropping _row_id on output",
+ slog.Warn("compaction group has unexpected mixed row
lineage on v3 table; dropping _row_id on output",
Review Comment:
"on v3 table" is redundant given the `Version() >= 3` guard, and
"unexpected" is non-actionable — and, per the comment above, a mixed group
isn't actually unexpected during migration. The original string read cleaner;
I'd leave it as it was.
##########
table/rewrite_data_files.go:
##########
@@ -390,9 +384,9 @@ func ExecuteCompactionGroup(ctx context.Context, tbl
*Table, group CompactionTas
// allTasksHaveRowLineage returns true iff every task in the group has a
// non-nil FirstRowID — i.e. every source file already carries v3 row lineage.
-// Used to gate the preservation path on compaction: mixed groups (some
-// lineage, some legacy) would otherwise produce per-file invariant
-// violations, so the gate is conservative.
+// Used to gate the preservation path on compaction: mixed groups are treated
+// conservatively because spec-compliant v3 planning should already surface an
Review Comment:
Same spec claim as above, and this godoc now justifies caller policy rather
than describing what the helper does. I'd keep the first sentence, note it
returns false for an empty slice, and drop the second sentence.
--
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]
