CTTY commented on PR #1490: URL: https://github.com/apache/iceberg-rust/pull/1490#issuecomment-3042319919
Hi @ZENOTME , thanks for working on this! I went thru the change and have some general thoughts: > ManifestProcess and SnapshotProduceOperation can be consider as custom extension for SnapshotProducer and they would reuse SnapshotProducer as for common usage. So we should include it in their function param - This makes sense to me >At the same time, include Table in SnapshotProducer make thing easier they can use SnapshotProducer diretcly. - As a helper class, `SnapshotProducer` should only be used when a `TransactionAction` commits (where you are actually generating the new snapshot). Since `TransactionAction::commit` already takes in a `&Table`, I don't think having `SnapshotProducer` hold its own table reference adds much value, and my concern is that allowing it to take another `Table` may lead to error-prone usages. I believe this is related to how `MergeAppendAction` will be integrated with `SnapshotProducer` though, do you think we can have `MergeAppendAction` reuse the existing design? -- 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]
