dannycjones commented on issue #2312: URL: https://github.com/apache/iceberg-rust/issues/2312#issuecomment-4189265565
I agree that the scheme is wrong here and the issue was introduced in iceberg-rust 0.9. I've taken a quick look at the code and believe that it wouldn't be possible to load tables from S3 Tables catalog nor write out to those tables, so I don't believe there's a risk of breaking anyone on 0.9 who has been using this catalog the past 3 weeks. - We can't read/load that table because I expect we'll fail to read the manifest with a `s3://` URI from the service). - We can't write out to those tables since we'll try to metadata layer files and data files with the wrong scheme. I see that we previously used `s3://` before this PR: https://github.com/apache/iceberg-rust/pull/2116/changes#diff-451c025a4aa08feaa36ea69026645beeb15ddbe8128f899b29f61c6ddfa93392. Xander also notes that the Java implementation uses `s3://` in S3FileIO: https://github.com/apache/iceberg-rust/pull/2313#issuecomment-4185562780 Note, there's a proposal to add integration tests that would have caught this issue: https://github.com/apache/iceberg-rust/issues/2211 -- 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]
