aokolnychyi commented on pull request #2591: URL: https://github.com/apache/iceberg/pull/2591#issuecomment-858912406
I know this is a bit behind on what Russell has locally but a few high-level points: - Shall we consider a separate quick PR for tiny updates to the existing code? Like fixing typos in constants and in Javadoc? Should make the review of this one a bit easier. - Shall we addd validation for TableScanUtil into a separate PR? Comment [here](https://github.com/apache/iceberg/pull/2591#discussion_r649433302). - RewriteStrategy - Shall we move it to core for now even though it will be public? - I am still not sure the current design of `RewriteStrategy` is the best we can come up with. Strategies are Serializable so I assume they can be sent to other nodes in case of distributed planning? However, strategies also define `rewriteFiles` that I think is going to be called on the driver? Plus, right now both the action as well as the strategy interact with the file rewrite coordinator and other things. Ideally, this should be a single entity, in my view. [Here](https://github.com/apache/iceberg/pull/2591#discussion_r634864457) is the relevant comment. -- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
