Allow executing optimize procedure for Iceberg v2 table

[ebyhr]

Description

Allow optimizing v2 table if delete files don't exist in Iceberg

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) Release notes entries required with the following suggested text:

# Iceberg
* Allow executing `optimize` procedure for v2 table. ({issue}`12351`)

[findepi]

Allow optimizing v2 table if delete files don't exist in Iceberg

What would it take to extend this to support v2 table with deletes?

cc @alexjo2144

[ebyhr]

I tried committing delete-files beforehand like this, but it still failed "Cannot commit, found new delete for replaced data file" error.

        DeleteFiles deleteFiles = transaction.newDelete();
        for (DeleteFile deletedFile : deletedFiles) {
            deleteFiles.deleteFile(deletedFile.path());
        }
        deleteFiles.commit();

        RewriteFiles rewriteFiles = transaction.newRewrite();
...

[findepi]

@ebyhr how do we know which delete files we can delete?

[homar]

Could you please write similar test or modify this in such a way that deletion is not the last operation ? So delete files are not part of the current snapshot?

[alexjo2144]

I tried committing delete-files beforehand like this, but it still failed "Cannot commit, found new delete for replaced data file" error.
...

I think you want the RewriteFiles#rewriteFiles method that takes 4 args.

RewriteFiles rewriteFiles = transaction.newRewrite();
rewriteFiles.rewriteFiles(scannedFiles, scannedDeleteFiles, newFiles, newDeleteFiles);
rewriteFiles.validateFromSnapshot(readSnapshotId).
rewriteFiles.commit();

[alexjo2144]

@ebyhr how do we know which delete files we can delete?

This could be tricky since delete-files may have deletes for multiple data-files. Some of which may be rewritten by optimize and some of which might not.

We skip scanning files during optimize if they're bigger than a given size, but we could change that to scan them files if they are small or have any delete files. That way we ensure that when the procedure completes all delete files can be removed.

[findepi]

we could change that to scan them files if they are small or have any delete files

yes

That way we ensure that when the procedure completes all delete files can be removed.

no, because there may be additional filters

This could be tricky since delete-files may have deletes for multiple data-files.

yes, tricky. What would happen if we don't delete any deletion files? Will Iceberg library prune them?

[alexjo2144]

no, because there may be additional filters

Right now alter table execute filters have to describe a partition, they can't be more specific than that. So we should be able to remove any delete files in that partition.

yes, tricky. What would happen if we don't delete any deletion files? Will Iceberg library prune them?

You won't be able to commit a snapshot in this state. The validations ensure that the data-files a delete-file references exist in the current snapshot.

[findepi]

The validations ensure that the data-files a delete-file references exist in the current snapshot.

Okay, but that requires reading all manifest files.
If we do this, we can as well find out the deletion files to forget about. Basically same logic as the validation, just a different conclusion.

[alexjo2144]

You won't be able to commit a snapshot in this state. The validations ensure that the data-files a delete-file references exist in the current snapshot.

I was wrong about the validations that run here, you can commit a table in this state, it just has a dangling delete-file that is never used.

[findepi]

Test failure is related

TestIcebergSparkCompatibility > testOptimizeFailsOnV2IcebergTable [groups: profile_specific_tests, iceberg]
Expected callback to throw QueryExecutionException

[ebyhr]

@findepi @homar Updated.

[findepi]

Keep, just change to 3

[findepi]

Keep the condition, just remove this comment.
We don't know what Iceberg v3 brings us.

[findepi]

Suggested change

	// org.apache.iceberg.Table.snapshot method returns null if there is no matching snapshot
	// Table.snapshot method returns null if there is no matching snapshot