pvary commented on pull request #2658: URL: https://github.com/apache/iceberg/pull/2658#issuecomment-857548271
@anuragmantri: Tried to go through the PR, but this one is huge 😄 Some general takeaways: - Can we keep the original signatures for the extended methods as well? They still can call the new ones, but if we kept the original ones then we do not have to change the callers to add `null, null` which could reduce the number of changes greatly - Can we decrease the number of parameters we push down the new methods? I would prefer to send a boolean `useRelativePath` instead of the whole property list, or even only send just the baseLocation if we have to use it. - Would it be possible to hide the whole stuff inside a LocationProvider? It could be a base implementation for all of the LocationProviders, and would be a natural place in my opinion. We can avoid changing the public APIs if we provide a default implementation. - If there is any way to split this PR to smaller, more reviewer friendly ones that would be a big help for us, reviewers. Thanks, Peter -- 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]
