Fix table location URL

[tdikland]

What changes are proposed in this pull request?

If the table is created from a URI, and that URI does not end with a trailing slash, then subsequent operations (i.e. deriving the path to the _delta_log folder) overwrite the last URL path segment.

The implemented fix is not very "nice", I'd be happy to do a more elaborate fix (e.g. wrapping table location in a newtype and exposing methods like delta_log(), last_checkpoint(), etc. All depending on your appetite for change and review bandwith of course.

How was this change tested?

Added unit test

[codecov]

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.56%. Comparing base (a2cade8) to head (bdd8c81).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/table.rs	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
+ Coverage   79.54%   79.56%   +0.02%     
==========================================
  Files          56       56              
  Lines       12259    12271      +12     
  Branches    12259    12271      +12     
==========================================
+ Hits         9751     9763      +12     
  Misses       1981     1981              
  Partials      527      527              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nicklan]

Thanks, this is a good catch. I don't love that "feature" of the url crate, but we do need to work within it. I think it would make more sense though to do this in resolve_uri_type where you could do something like:

    let url = if table_uri.ends_with('/') {
        Url::parse(table_uri)
    } else {
        let mut s = table_uri.to_string();
        s.push('/');
        Url::parse(&s)
    };
    if let Ok(url) = url {
...

[scovich]

Thanks, this is a good catch. I don't love that "feature" of the url crate, but we do need to work within it. I think it would make more sense though to do this in resolve_uri_type

I tend to agree... pushing it down would fix path handling as well, tho maybe our path handling was already hardened previously?

Code golf nitpick:

you could do something like:

    let url = if table_uri.ends_with('/') {
        Url::parse(table_uri)
    } else {
        let mut s = table_uri.to_string();
        s.push('/');
        Url::parse(&s)
    };
    if let Ok(url) = url {
...

Or just

let table_uri = if table_uri.ends_with('/') {
  Cow::Borrowed(table_uri)
} else {
  Cow::Owned(format!("{table_uri}/"))
}
if let Ok(url) = Url::parse(&table_uri) {

[tdikland]

Thanks for the suggestions - applied the changes. From my personal experience, the subtle differences between std::path::Path, url::Url and object_store::path::Path often lead to little bugs like these!

[scovich]

Nice!

[scovich]

side question:" Why does this LocalPath use table_uri but the other (L108) uses url.to_file_path?
Is it because we don't have a file:// scheme here?

[nicklan]

Yeah. True to form, the URL crate isn't really helpful. From the to_file_path docs:

Assuming the URL is in the file scheme or similar, convert its path to an absolute std::path::Path.
Note: This does not actually check the URL’s scheme, and may give nonsensical results for other schemes. It is the user’s responsibility to check the URL’s scheme before calling this.

So this seems safer.

[nicklan]

Thanks @tdikland , appreciate the PR!