Core: Classify RowDelta with data files only as APPEND

[mxm]

When RowDelta only adds data files without adding delete files or deleting data files, the snapshot should be classified as an APPEND operation instead of OVERWRITE. The resulting operation is quite important because readers which read only append snapshots won't process overwrite snapshots.

The patch here is similar to the existing logic in OverwriteFiles: https://github.com/apache/iceberg/blame/main/core/src/main/java/org/apache/iceberg/BaseOverwriteFiles.java#L56

Merging this would avoid patches like #14559, where separate code paths are required for the overwrite and the append path.

[pvary]

Based on the discussion here, I think this should be enough:

   if (addsDataFiles() && !addsDeleteFiles()) { 
     return DataOperations.APPEND;
   }

I think if (!addsDataFiles() && addsDeleteFiles()) is correct as the RowDelta API only provides addRows(DataFile) and addDeletes(DeleteFile). deletesDataFiles() would apply for the RewriteFiles API.

[singhpk234]

thinking of a case like this, let's say engine wanna delete a record from a file which just had one record in that case an engine might just optimize by saying lets remove the whole file instead of producing a delete ? in this case condition like below protects us :

if (addsDataFiles() && !addsDeleteFiles() && !deletesDataFiles()) {

my understanding is we do support support removing data file in row delta post this

• API, Core: Add deleteFile to RowDelta API #12861

[pvary]

Good point @singhpk234!

In this case the other condition is bugous:

    if (addsDeleteFiles() && !addsDataFiles()) {
      return DataOperations.DELETE;
    }

We could add back data with removeDeletes.

To make things even more problematic, in V3 we will have DV updates which is likely a deleteDeleteFile(DV1) + an addDeleteFile(DV2). But nothing guarantees in this case that only new deletes happen. Theoretically this could bring back new records.

So this means that @mxm's change is correct, but likely the other path where we fall back to DataOperations.DELETE is problematic

[mxm]

Thanks for the review @singhpk234 and @pvary! Should we change the DELETE path to something like:

    if (addsDeleteFiles() && !addsDataFiles() && !deletesDeleteFiles()) {
      return DataOperations.DELETE;
    }

Or do you prefer to do this in a separate PR? Originally, this PR is meant to optimize the APPEND case only.

[pvary]

For V2 tables, the condition above works correctly for deletes. However, for V3 tables, if there are already deletes for a data file (i.e., an existing DV), we always remove the old DV and add a new one. This means the condition above is never triggered. On the other hand, we typically don’t re-add rows, so the DELETE operation remains the correct outcome.

My main concern with changing this behavior is that it could be unexpected for users. That’s why I’d prefer to separate the two cases.

In the meantime, if you have some time, could you please test my theory? (Totally understand if you’re busy—I’m just hopeful 😄)

[mxm]

Indeed, it seems rare that we would delete a delete file to resurrect deleted data, but you are right about V3+ DV delete files triggering the above condition due to removing the old delete file and adding the rewritten one, e.g.:

iceberg/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java

Line 232 in 571b696

rowDelta.removeDeletes(deleteFile);

Maybe we just leave things as-is for DELETE? My intention anyways was to fix the APPEND case :)