ZENOTME commented on PR #738: URL: https://github.com/apache/iceberg-rust/pull/738#issuecomment-2595160984
> Thanks @ZENOTME for this pr, generally LGTM! I still have concerns with the add/eixsting/delete api, and prefer the approach used in java api: org.apache.iceberg.ManifestWriter#add(F), which provides better api safety. For what I mentioned in comments, it's possible to add some check, but it's not a good api for user which throws error at runtime. Hi @liurenjie1024, seems iceberg-java also provide the interface for entry in https://github.com/apache/iceberg/blob/d96901b843395fe669f6bd4f618f8e5e46c0eed4/core/src/main/java/org/apache/iceberg/ManifestWriter.java#L157, and use them in ManifsetMergeManager: https://github.com/apache/iceberg/blob/d96901b843395fe669f6bd4f618f8e5e46c0eed4/core/src/main/java/org/apache/iceberg/ManifestMergeManager.java#L188. And it also don't check some case like existing entry with null data sequence number. Is the case that iceberg-java missed or it's acceptable. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
