zhengxiang378928908-code commented on PR #18361: URL: https://github.com/apache/iceberg/pull/18361#issuecomment-6594220860
Thanks for the update. The CVE leftover is gone, and the tests now consume `planFiles()`. One test still can't catch the bug, plus a smaller point: 1. `equalityDeleteBeforePromotionAppliedToDataAfter` can't fail on the promotion bug. The equality delete is committed before the data file, so it has a lower sequence number, and equality deletes only apply to data files with a *strictly lower* sequence number. `data-after-eq.parquet` gets no delete whether or not partition matching works, and `hasSize(1)` holds either way. Also, the comment "Equality deletes are handled in DeleteFileIndex, not task.deletes()" isn't accurate: `FileScanTask.deletes()` does include equality deletes, since `DeleteFileIndex.forDataFile` returns both kinds. To cover the case, the equality delete has to come after the data file it should apply to. For example: append an `int` data file, promote `region` to `long`, commit an equality delete with partition `5L`, then assert that the pre-promotion file's task has exactly one delete. Or commit the delete before promotion but after the data file, and assert the same thing after promotion. 2. Nit: in `positionDeleteBeforePromotionCommitsWithRowDelta` and `bothDataAndDeleteFilesBeforePromotion`, `sum >= 1` would also pass if the delete got attached to the wrong file. Since there are two tasks, could you assert per file instead: the pre-promotion file has one delete and `data-after*.parquet` has none? -- 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]
