Fix 5592: Colon (:) in in object_store::path::{Path} is not handled on Windows

[hesampakdaman]

Which issue does this PR close?

Closes #5592.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[thomasfrederikhoeck]

@hesampakdaman is this ready for review? :-)

[hesampakdaman]

@thomasfrederikhoeck I believe so, although I haven't been able to run an integration test (don't have Windows accessible).

[alamb]

Can someone please verify that this works on Windows? I don't have access to a windows machine at the moment

[thomasfrederikhoeck]

I don't think this fixes it. If I run the following on Windows with the fix

use object_store::path::{Path};

fn main() {
    let path: String = r"C:\table\time=2021-01-02 03:04:06.000003\file.parquet".to_string();
    let path_from: Path = Path::from(path);
    println!( "{:?}", path_from);

    let path: String = r"C:\table\time=2021-01-02 03:04:06.000003\file.parquet".to_string();
    let path_parse: Result<Path, object_store::path::Error> = Path::parse(path);
    println!( "{:?}", path_parse);
}

I still see:

Path { raw: "C:%5Ctable%5Ctime=2021-01-02 03:04:06.000003%5Cfile.parquet" }
Ok(Path { raw: "C:\\table\\time=2021-01-02 03:04:06.000003\\file.parquet" })

where I would have expected:

Path { raw: "C:%5Ctable%5Ctime=2021-01-02 03%3A04%3A06.000003%5Cfile.parquet" }
Ok(Path { raw: "C:\\table\\time=2021-01-02 03:04:06.000003\\file.parquet" })

But I might be assuming something wrong? @alamb @hesampakdaman

[hesampakdaman]

@thomasfrederikhoeck You're not thinking wrong, my initial suggestion was to change how Path behaves. In the discussions we had in the issue, @tustvold mentioned that that would be a breaking change and affect all paths for all stores.

So instead we agreed to change how LocalFileSystem behaves when using it from Windows. If you use the put/get methods it should convert the : to %3A. Although, I'm a little uncertain if I'm correctly handling the leading C:\.

At any rate, if you could test this that would be awesome! I would have done it myself but I don't have access to Windows unfortunately.

[thomasfrederikhoeck]

@hesampakdaman I just tested with below and it doesn't appear like it is handeling the : in the drive correctly which also are changes but shouldn't. This results in the file being created in a relative position with the name as C:\projects\delta\pathtester\C%3A\Users\tfh\AppData\Local\Temp\.tmpwqJ9tp\file%3Aname.parquet

The script I ran was

use object_store::path::{Path};
use tempfile::{TempDir};
use object_store::local::LocalFileSystem;
use object_store::ObjectStore;
use bytes::Bytes;

#[tokio::main]
async fn main() {
    let root = TempDir::new().unwrap();
    let integration = LocalFileSystem::new_with_prefix(root.path()).unwrap();
    let path = Path::parse("file%3Aname.parquet").unwrap();
    let location = Path::parse("file:name.parquet").unwrap();

    integration.put(&location, "test".into()).await.unwrap();

    let result = integration
        .get(&location)
        .await
        .unwrap()
        .bytes()
        .await
        .unwrap();
    assert_eq!(result, Bytes::from("test"));
}

[thomasfrederikhoeck]

I can also add that if I run the following it creates a file at C:\projects\delta\pathtester\C%3A\file%3Aname.parquet

use object_store::path::{Path};
use object_store::local::LocalFileSystem;
use object_store::ObjectStore;
use bytes::Bytes;

#[tokio::main]
async fn main() {
    let integration = LocalFileSystem::new();
    let location = Path::parse("C:\\file:name.parquet").unwrap();
    println!("{}",location);
    integration.put(&location, "test".into()).await.unwrap();

    let result = integration
        .get(&location)
        .await
        .unwrap()
        .bytes()
        .await
        .unwrap();
    assert_eq!(result, Bytes::from("test"));
}

[thomasfrederikhoeck]

FYI the code I run is running with C:\projects\delta\pathtester as cwd :-)

[hesampakdaman]

@thomasfrederikhoeck Thank you for testing, it helps a lot! Yes, as suspected I did not handle the leading colon from the drive. I made a change and hopefully it will work now.

@alamb I marked this as draft until it is tested again by @thomasfrederikhoeck.

[thomasfrederikhoeck]

@hesampakdaman I can confirm it gives the expected results for these two :-D :-D :

use object_store::path::{Path};
use object_store::local::LocalFileSystem;
use object_store::ObjectStore;
use bytes::Bytes;
use tempfile::{NamedTempFile, TempDir};

#[tokio::main]
async fn main() {
    let root_temp = TempDir::new().unwrap();

    // full path
    let integration_full = LocalFileSystem::new();
    let location_full = Path::parse(r"C:\Users\tfh\file:name-full.parquet").unwrap();
    println!("{}",location_full);
    integration_full.put(&location_full, "test".into()).await.unwrap();

    let result = integration_full
        .get(&location_full)
        .await
        .unwrap()
        .bytes()
        .await
        .unwrap();
    assert_eq!(result, Bytes::from("test"));


    let integration_prefefix = LocalFileSystem::new_with_prefix(r"C:\Users\tfh\").unwrap();
    let location_prefefix = Path::parse(r"file:name-prefix.parquet").unwrap();
    println!("{}",location_prefefix);
    integration_prefefix.put(&location_prefefix, "test".into()).await.unwrap();
    let result = integration_prefefix
        .get(&location_prefefix)
        .await
        .unwrap()
        .bytes()
        .await
        .unwrap();
    assert_eq!(result, Bytes::from("test"));

}

[alamb]

Thanks for the check @thomasfrederikhoeck

Would someone be willing to setup a github runner to test object_store on windows as part of CI? That would ensure we don't introduce regressions going forward.

The CI jobs are defined here: https://github.com/apache/arrow-rs/blob/master/.github/workflows/object_store.yml

Here is an example of testing on windows from DataFusion: https://github.com/apache/datafusion/blob/dd56dbe67c9c61fbe13fefda367fe50503921351/.github/workflows/rust.yml#L325

[hesampakdaman]

@alamb I'll take a look later in the week and will add it in this PR!

[alamb]

Thank you @hesampakdaman

[hesampakdaman]

@alamb I finally added a CI job. However, I noticed that some tests failed if I ran them all under windows. Some of these were outside of local.rs. I did two things to address (ignore) this:

1. I run only the tests in local.rs
2. I added a target = "unix" on two tests that were failing under windows.

I will try to verify if the tests are failing without my changes.

[hesampakdaman]

I re-ran the CI tests for windows and removed my changes. It seems that they just fail under windows.

[alamb]

Thank you again @hesampakdaman and @thomasfrederikhoeck for this contribution -- this looks good to me. I also appreciate the effort to make a test.

It looks like this PR has a conflict to resolve but then it should be good to go.

[alamb]

👍

[hesampakdaman]

@alamb Done 🚀

[alamb]

Thanks again everyone!