anuragmantri commented on pull request #2658: URL: https://github.com/apache/iceberg/pull/2658#issuecomment-865616632
Thanks for the review @pvary :) > 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. - In this change, we only want to replace the references of paths in the content of `metadata` files. We do not wish to change where data/metadata is actually written. My understanding of `LocationProviders` is that they are used to get locations to write the data files and hence for this change, we don't want to change any LocationProvider 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. - I tried to keep this change small initially. Specifically, in this PR, I tried to just introduce the new property and modify the read and write paths to use table property to do the necessary changes. Property is turned off my default. Yet, this turned out to be huge :) There will be several follow-up PRs such has handling metadata tables and writing unit tests. I will be happy to split this further if you think we can logically do so. -- 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]
