laskoviymishka commented on code in PR #2309:
URL: https://github.com/apache/iceberg-go/pull/2309#discussion_r4368746797
##########
table/transaction.go:
##########
@@ -1798,21 +1798,12 @@ type DeleteFileAddition struct {
DataSequenceNumber int64
}
-// ReplaceFiles atomically replaces data files and removes associated delete
files
-// in a single snapshot. This is the commit primitive for compaction: old data
files
-// are replaced with new (compacted) data files, and delete files that are
fully
-// applied are removed. On v3 a removed data file's live deletion vector is
-// removed with it, whether or not it is in deleteFilesToRemove.
-func (t *Transaction) ReplaceFiles(ctx context.Context, dataFilesToDelete,
dataFilesToAdd, deleteFilesToRemove []iceberg.DataFile, snapshotProps
iceberg.Properties, opts ...WriteOption) error {
- return t.replaceFiles(ctx, dataFilesToDelete, dataFilesToAdd,
deleteFilesToRemove, nil, nil, snapshotProps, opts...)
-}
-
-// ReplaceFilesWithDeleteFiles atomically replaces data files, adds new delete
+// ReplaceFiles atomically replaces data files, adds new delete
Review Comment:
If we keep the combined `ReplaceFiles`, could this doc carry over the
compaction and v3 DV sentences from [main's
`ReplaceFiles`](https://github.com/apache/iceberg-go/blob/b7792e9e8799664f2ca8cd7f4ea94c97515c9833/table/transaction.go#L1801-L1808)?
As merged it reads as delete-file rewrites only, but `rewriteDataFilesPartial`
still commits compaction through it, and a removed data file's live DV is still
removed with it, listed in `deleteFilesToRemove` or not
(`TestRemovingDataFileRemovesItsUnnamedDV` pins that).
##########
table/transaction.go:
##########
@@ -1798,21 +1798,12 @@ type DeleteFileAddition struct {
DataSequenceNumber int64
}
-// ReplaceFiles atomically replaces data files and removes associated delete
files
-// in a single snapshot. This is the commit primitive for compaction: old data
files
-// are replaced with new (compacted) data files, and delete files that are
fully
-// applied are removed. On v3 a removed data file's live deletion vector is
-// removed with it, whether or not it is in deleteFilesToRemove.
-func (t *Transaction) ReplaceFiles(ctx context.Context, dataFilesToDelete,
dataFilesToAdd, deleteFilesToRemove []iceberg.DataFile, snapshotProps
iceberg.Properties, opts ...WriteOption) error {
- return t.replaceFiles(ctx, dataFilesToDelete, dataFilesToAdd,
deleteFilesToRemove, nil, nil, snapshotProps, opts...)
-}
-
-// ReplaceFilesWithDeleteFiles atomically replaces data files, adds new delete
+// ReplaceFiles atomically replaces data files, adds new delete
// files, and removes superseded delete files in one snapshot. Every added
delete
// file must carry the data sequence number of the delete files it replaces.
// This is intended for rewrites that change delete-file representation, such
as
// rewriting position deletes into deletion vectors.
-func (t *Transaction) ReplaceFilesWithDeleteFiles(ctx context.Context,
dataFilesToDelete, dataFilesToAdd, deleteFilesToRemove []iceberg.DataFile,
deleteFilesToAdd []DeleteFileAddition, snapshotProps iceberg.Properties, opts
...WriteOption) error {
+func (t *Transaction) ReplaceFiles(ctx context.Context, dataFilesToDelete,
dataFilesToAdd, deleteFilesToRemove []iceberg.DataFile, deleteFilesToAdd
[]DeleteFileAddition, snapshotProps iceberg.Properties, opts ...WriteOption)
error {
Review Comment:
Should we send delete-file replacement only through `NewRewrite` and keep
the 5-argument `ReplaceFiles` we've shipped [since
v0.6.0](https://github.com/apache/iceberg-go/blob/350ae7270d7c44578638ef55e7e0392227745e8a/table/transaction.go#L844)?
Widening it gives us the positional 4-collection shape Java [deprecated in
1.3.0](https://github.com/apache/iceberg/blob/7dbdfd33a667a721fbb21c7c7d06fec9daa30b88/api/src/main/java/org/apache/iceberg/RewriteFiles.java#L160-L175)
for the builder `NewRewrite` mirrors.
They commit differently, though: `NewRewrite` records `replace` and
`ReplaceFiles` [records
`overwrite`](https://github.com/apache/iceberg-go/blob/53c23562b381833ea9b47e3a391f9578b55e5835/table/transaction.go#L2166-L2168),
so callers moving off `ReplaceFilesWithDeleteFiles` would get a different
snapshot operation.
--
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]
