fix: to_uri() in the logstore implementation should use Url::join

Any places where we see hard-coded string formatting (e.g. format!())
around Url is likely to be **wrong** and need to be removed.

This commit fixes the failing integration tests with the lakefs
connector

Signed-off-by: R. Tyler Croy <rtyler@brokenco.de>

Author: rtyler

crates/core/src/logstore/mod.rs

@@ -551,7 +551,9 @@ pub(crate) fn object_store_path(table_root: &Url) -> DeltaResult<Path> {
     })
 }
 
-/// TODO
+/// Join the given `root` [Url] with the [Path] to produce a URI (String) of the two together.
+///
+/// This is largely a convenience function to help with the nuances of empty [Path] and file [Url]s
 pub fn to_uri(root: &Url, location: &Path) -> String {
     match root.scheme() {
         "file" => {
@@ -575,7 +577,9 @@ pub fn to_uri(root: &Url, location: &Path) -> String {
             if location.as_ref().is_empty() || location.as_ref() == "/" {
                 root.as_ref().to_string()
             } else {
-                format!("{}/{}", root.as_ref(), location.as_ref())
+                root.join(location.as_ref())
+                    .expect("Somehow failed to join on a Url!")
+                    .to_string()
             }
         }
     }

crates/lakefs/src/logstore.rs

@@ -5,6 +5,7 @@ use bytes::Bytes;
 use deltalake_core::logstore::{
     DefaultObjectStoreRegistry, ObjectStoreRegistry, commit_uri_from_version,
 };
+use deltalake_core::table::normalize_table_url;
 use deltalake_core::{
     DeltaResult, kernel::transaction::TransactionError, logstore::ObjectStoreRef,
 };
@@ -116,7 +117,13 @@ impl LakeFSLogStore {
             "lakefs://{repo}/{}/{table}",
             self.client.get_transaction(operation_id)?,
         );
-        Ok(Url::parse(&string_url).unwrap())
+        Ok(normalize_table_url(&Url::parse(&string_url).map_err(
+            |_| {
+                DeltaTableError::NotATable(format!(
+                    "Could not convert {string_url} into a table URL"
+                ))
+            },
+        )?))
     }
 
     fn get_transaction_objectstore(
@@ -399,3 +406,6 @@ fn put_options() -> &'static PutOptions {
         extensions: Default::default(),
     })
 }
+
+#[cfg(test)]
+mod tests {}

python/tests/test_fs.py

@@ -96,7 +96,7 @@ def test_read_simple_table_from_remote(s3_localstack):
         "part-00007-3a0e4727-de0d-41b6-81ef-5223cf40f025-c000.snappy.parquet",
     ]
 
-    assert dt.file_uris() == [table_path + "/" + path for path in expected_files]
+    assert dt.file_uris() == [table_path + path for path in expected_files]
 
 
 @pytest.mark.pyarrow