[I] Add a flag to warn and continue sync if deletion files are not supported in the Target [incubator-xtable]

["ashvina (via GitHub)" <gi...@apache.org>]

ashvina opened a new issue, #657:
URL: https://github.com/apache/incubator-xtable/issues/657

                 In my opinion, a warning log doesn't really fix the data inconsistency issue. However, I agree that adding a flag could be a good idea in the short term. I would suggest that the flag's default behavior should be to throw an error. We should also consider phasing it out in future releases.
   
   _Originally posted by @ashvina in https://github.com/apache/incubator-xtable/pull/634#discussion_r1953894471_
               


-- 
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: commits-unsubscribe@xtable.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Add a flag to warn and continue sync if deletion files are not supported in the Target [incubator-xtable]

["slachiewicz (via GitHub)" <gi...@apache.org>]

slachiewicz commented on issue #657:
URL: https://github.com/apache/incubator-xtable/issues/657#issuecomment-5382309772

   A reproduction of the known limitation tracked in #339, with an independent reader as the
   oracle, in case it helps move this flag along. It supports @ashvina's position above that the
   default should be an error rather than a warning.
   
   Setup: a local Delta table with `delta.enableDeletionVectors = true`, 1,000 rows, synced
   DELTA → ICEBERG with the bundled jar from `main`.
   
   ```
   sync 1  -> Iceberg snapshot: total-records=1000, total-data-files=4, total-delete-files=0   correct
   DELETE FROM delta.`...` WHERE id < 100
           -> Delta now reads 900 rows; one deletion_vector_*.bin on disk
   sync 2  -> "Sync is successful", sourceIdentifier advances 1 -> 2, a NEW snapshot is written
           -> that snapshot: total-records=1000, total-data-files=4, total-delete-files=0
   ```
   
   Read back with DuckDB, which reads both formats and is neither XTable nor Spark — its Delta
   support is delta-kernel-rs, so both sides of the comparison are foreign readers:
   
   ```sql
   SELECT count(*) FROM delta_scan('.../dv_table');                    -- 900
   SELECT count(*) FROM iceberg_scan('.../metadata/v4.metadata.json'); -- 1000
   
   SELECT id FROM iceberg_scan(...) EXCEPT SELECT id FROM delta_scan(...) ORDER BY id LIMIT 10;
   -- 0,1,2,3,4,5,6,7,8,9   -- exactly the deleted rows
   ```
   
   Delta says 900, Iceberg says 1000, and the extra rows are precisely the ones that were deleted.
   Exit code 0, nothing in the log — `grep -i "deletion vector"` over the run output returns nothing.
   pyiceberg gives the same answer independently.
   
   For what it is worth as a control, the same harness over five tables converted without deletion
   vectors reports matching row counts *and* identical value-level checksums between
   `delta_scan` and `iceberg_scan`. The ordinary conversion path is faithful; this is a specific
   hole, not general drift.
   
   The path is in `DeltaConversionSource`. A Delta DELETE with deletion vectors emits
   `RemoveFile(old)` plus `AddFile(same path, with DV)`, and the pair is cancelled out of both the
   added and removed maps, since at file level nothing changed:
   
   ```java
   for (String deletionVector : deletionVectors) {
     if (removedFiles.containsKey(deletionVector)) {
       addedFiles.remove(deletionVector);
       removedFiles.remove(deletionVector);
     }
   ```
   
   That is correct as file-level bookkeeping. The consequence is that the row-level delete leaves
   no trace in the diff at all, so the incremental sync is a well-formed no-op over a table whose
   contents changed.
   
   Why the flag matters. The RFC merged in #634 (`rfc/rfc-2`) already describes the intended
   conversion, and notes its cost is "proportional to the number of records deleted, and not the
   size of metadata or number of files" — so this is tractable work rather than a blocked design.
   But it has been open since, and in the meantime the failure mode is silent. Deleted rows remain
   readable through the translated Iceberg metadata, and nothing in the run says so. That is the
   case for erroring by default, with an opt-in flag to warn and continue, exactly as suggested
   above.
   
   One thing that may be worth folding into the plan later, explicitly **not** as a reason to
   delay any of the above. The RFC targets Iceberg v2 positional delete files. The Iceberg spec now
   reads:
   
   > Position deletes are encoded in a _position delete file_ (V2) or _deletion vector_ (V3 or above).
   
   XTable writes v2 today — `iceberg-core` 1.9.2 has `SUPPORTED_TABLE_FORMAT_VERSION = 3` but
   `DEFAULT_TABLE_FORMAT_VERSION = 2`, and I found no `format-version` handling in the codebase to
   override it. A v3 deletion vector is closer in shape to what Delta already stores, so it may
   eventually be the better encoding. But v2 metadata is readable by v3 readers, so implementing
   rfc-2 as specified is not wasted work. Suggested ordering: the fail-loudly flag now, rfc-2's v2
   path as designed, and v3 deletion vectors as a later optimisation if it proves worthwhile.
   
   Versions, so the repro is actionable: XTable `main` at dafce0a (0.5.0-SNAPSHOT, bundled jar) on
   Corretto 11; source table written by delta-spark 3.2.0 / PySpark 3.5.1; `iceberg-core` 1.9.2;
   read back with DuckDB 1.4.5 and PyIceberg.
   
   Happy to contribute the reproduction as a test case if that would help.
   
   *This comment was created with AI assistance.*
   


-- 
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: commits-unsubscribe@xtable.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org