u70b3 commented on PR #2752: URL: https://github.com/apache/iceberg-rust/pull/2752#issuecomment-5695172008
Thanks for the thorough pass — all eleven comments are addressed in 66c74f2, just pushed. The short version: - **Data-loss path**: the orchestrator now derives `changed || output.is_none()` itself instead of trusting the rewriter's flag, with regression tests on exactly the silent-drop scenario. - **All-deleted files**: candidates with delete files that read as zero rows now land in `removed_data_files` with no replacement, matching RewriteDataFiles. Caveat noted on the thread: no e2e test until the delete-commit fixtures from #2185/#2203 exist. - **Object storage layout**: `write.object-storage.enabled` did not exist in this repo at all — added the property and the `ObjectStorageLocationGenerator` branch, with a layout-shape test. - **`output_rows`** now counts only rows actually written to replacement files, so it cross-checks against `added_data_files`. - **Contract decisions documented**: sync `rewrite_batch` (object safety) and read-only `CowRewriteFile` are settled in the docs; `CowBatchRewrite` is `Debug`; the prefix buffer's whole-file worst case is stated. - Small stuff: let-else instead of `expect`, per-candidate clones gone, the `partition_type` pre-call explained. Full test suite (1768 tests), clippy and fmt are clean, and `public-api.txt` is regenerated. Ready for another look. -- 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]
