From 9afb81eb577d9b8dac6a45e3d8b1cb49be31a97d Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Sat, 25 May 2024 11:08:28 +0200 Subject: [PATCH 01/11] Fix issue #5800: Handle missing files in list_with_delimiter --- object_store/src/local.rs | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 95b50d6743f0..1ce588af2144 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -504,7 +504,7 @@ impl ObjectStore for LocalFileSystem { match config.filesystem_to_path(entry.path()) { Ok(path) => match is_valid_file_path(&path) { - true => Some(convert_entry(entry, path)), + true => convert_entry(entry, path).transpose(), false => None, }, Err(e) => Some(Err(e)), @@ -581,8 +581,8 @@ impl ObjectStore for LocalFileSystem { if is_directory { common_prefixes.insert(prefix.child(common_prefix)); - } else { - objects.push(convert_entry(entry, entry_location)?); + } else if let Some(metadata) = convert_entry(entry, entry_location)? { + objects.push(metadata); } } } @@ -894,12 +894,21 @@ fn open_file(path: &PathBuf) -> Result<(File, Metadata)> { Ok(ret) } -fn convert_entry(entry: DirEntry, location: Path) -> Result { - let metadata = entry.metadata().map_err(|e| Error::Metadata { - source: e.into(), - path: location.to_string(), - })?; - convert_metadata(metadata, location) +fn convert_entry(entry: DirEntry, location: Path) -> Result> { + match entry.metadata() { + Ok(metadata) => convert_metadata(metadata, location).map(Some), + Err(e) => { + if let Some(io_err) = e.io_error() { + if io_err.kind() == ErrorKind::NotFound { + return Ok(None); + } + } + Err(Error::Metadata { + source: e.into(), + path: location.to_string(), + })? + } + } } fn last_modified(metadata: &Metadata) -> DateTime { From a194b5ef9a27c01d1fadc00f4f059f983b614d15 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Sat, 1 Jun 2024 12:31:23 +0200 Subject: [PATCH 02/11] draft --- object_store/src/local.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 1ce588af2144..dab24fd434d0 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -293,7 +293,15 @@ impl LocalFileSystem { path: location.as_ref() } ); - self.config.prefix_to_filesystem(location) + let path = self.config.prefix_to_filesystem(location)?; + + #[cfg(target_os = "windows")] + let path = { + let mut path = path; + PathBuf::from(path.to_string_lossy().replace(":", "%3A")) + }; + + Ok(path) } } @@ -1446,6 +1454,23 @@ mod tests { list.sort_unstable(); assert_eq!(list, vec![c, a]); } + + + #[tokio::test] + #[cfg(target_os = "windows")] + async fn filesystem_filename_with_colon() { + 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 list = flatten_list_stream(&integration, None).await.unwrap(); + assert_eq!(list, vec![path.clone()]); + + let result = integration.get(&location).await.unwrap().bytes().await.unwrap(); + assert_eq!(result, Bytes::from("test")); + } } #[cfg(not(target_arch = "wasm32"))] From e72c7826edcffd66763cfa598154cd1d5c57889b Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Sat, 1 Jun 2024 12:37:55 +0200 Subject: [PATCH 03/11] cargo fmt --- object_store/src/local.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index dab24fd434d0..83bdf76ed0d0 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -1455,7 +1455,6 @@ mod tests { assert_eq!(list, vec![c, a]); } - #[tokio::test] #[cfg(target_os = "windows")] async fn filesystem_filename_with_colon() { @@ -1468,7 +1467,13 @@ mod tests { let list = flatten_list_stream(&integration, None).await.unwrap(); assert_eq!(list, vec![path.clone()]); - let result = integration.get(&location).await.unwrap().bytes().await.unwrap(); + let result = integration + .get(&location) + .await + .unwrap() + .bytes() + .await + .unwrap(); assert_eq!(result, Bytes::from("test")); } } From 6fa6c3b2d4ff5a806d1fb7d9c949c6e0bf6aefd7 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Tue, 25 Jun 2024 21:21:31 +0200 Subject: [PATCH 04/11] Handle leading colon --- object_store/src/local.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 83bdf76ed0d0..c29f13f0b436 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -297,8 +297,15 @@ impl LocalFileSystem { #[cfg(target_os = "windows")] let path = { - let mut path = path; - PathBuf::from(path.to_string_lossy().replace(":", "%3A")) + let path = path.to_string_lossy(); + + // Assume the first char is the drive letter and the next is a colon. + let mut out = String::new(); + let drive = &path[..2]; // The drive letter and colon (e.g., "C:") + let filepath = &path[2..].replace(':', "%3A"); // Replace subsequent colons + out.push_str(drive); + out.push_str(filepath); + PathBuf::from(out) }; Ok(path) From 80a0deb0f55aa17049745cf9487e405a46586c82 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Tue, 2 Jul 2024 11:09:55 +0200 Subject: [PATCH 05/11] Add windows CI --- .github/workflows/object_store.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/object_store.yml b/.github/workflows/object_store.yml index 422f676cb7d9..7cb71833990b 100644 --- a/.github/workflows/object_store.yml +++ b/.github/workflows/object_store.yml @@ -198,3 +198,18 @@ jobs: run: cargo build --target wasm32-unknown-unknown - name: Build wasm32-wasi run: cargo build --target wasm32-wasi + + windows: + name: cargo test (win64) + runs-on: windows-latest + defaults: + run: + working-directory: object_store + steps: + - uses: actions/checkout@v4 + with: + submodules: true + - name: Setup Rust toolchain + uses: ./.github/actions/setup-windows-builder + - name: Run object_store tests + run: cargo test --features=aws,azure,gcp,http From 787d274f3c8031a230248478fae28705f99a1b15 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Tue, 2 Jul 2024 11:23:08 +0200 Subject: [PATCH 06/11] Fix CI job --- .github/workflows/object_store.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/object_store.yml b/.github/workflows/object_store.yml index 7cb71833990b..3dc491d0f63e 100644 --- a/.github/workflows/object_store.yml +++ b/.github/workflows/object_store.yml @@ -209,7 +209,5 @@ jobs: - uses: actions/checkout@v4 with: submodules: true - - name: Setup Rust toolchain - uses: ./.github/actions/setup-windows-builder - name: Run object_store tests run: cargo test --features=aws,azure,gcp,http From 2c936a9c331ac74e709ab11cc6b1761ef3613ea0 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Tue, 2 Jul 2024 11:40:42 +0200 Subject: [PATCH 07/11] Only run local tests and set target family for failing tests --- .github/workflows/object_store.yml | 6 +++--- object_store/src/local.rs | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/object_store.yml b/.github/workflows/object_store.yml index 3dc491d0f63e..bdbfc0bec4bb 100644 --- a/.github/workflows/object_store.yml +++ b/.github/workflows/object_store.yml @@ -200,7 +200,7 @@ jobs: run: cargo build --target wasm32-wasi windows: - name: cargo test (win64) + name: cargo test LocalFileSystem (win64) runs-on: windows-latest defaults: run: @@ -209,5 +209,5 @@ jobs: - uses: actions/checkout@v4 with: submodules: true - - name: Run object_store tests - run: cargo test --features=aws,azure,gcp,http + - name: Run LocalFileSystem tests + run: cargo test local::tests diff --git a/object_store/src/local.rs b/object_store/src/local.rs index c29f13f0b436..4d1be8b6b15c 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -1034,6 +1034,7 @@ mod tests { use super::*; #[tokio::test] + #[cfg(target_family = "unix")] async fn file_test() { let root = TempDir::new().unwrap(); let integration = LocalFileSystem::new_with_prefix(root.path()).unwrap(); @@ -1050,6 +1051,7 @@ mod tests { } #[test] + #[cfg(target_family = "unix")] fn test_non_tokio() { let root = TempDir::new().unwrap(); let integration = LocalFileSystem::new_with_prefix(root.path()).unwrap(); From 9b3ffb26356b30f94189c7294d10e875c9a0b167 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Tue, 2 Jul 2024 11:54:30 +0200 Subject: [PATCH 08/11] Run all tests without my changes and removed target os --- .github/workflows/object_store.yml | 2 +- object_store/src/local.rs | 26 ++++++++++++-------------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/.github/workflows/object_store.yml b/.github/workflows/object_store.yml index bdbfc0bec4bb..9de3b3ce4cb2 100644 --- a/.github/workflows/object_store.yml +++ b/.github/workflows/object_store.yml @@ -210,4 +210,4 @@ jobs: with: submodules: true - name: Run LocalFileSystem tests - run: cargo test local::tests + run: cargo test diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 4d1be8b6b15c..32bd0c5417ba 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -295,18 +295,18 @@ impl LocalFileSystem { ); let path = self.config.prefix_to_filesystem(location)?; - #[cfg(target_os = "windows")] - let path = { - let path = path.to_string_lossy(); - - // Assume the first char is the drive letter and the next is a colon. - let mut out = String::new(); - let drive = &path[..2]; // The drive letter and colon (e.g., "C:") - let filepath = &path[2..].replace(':', "%3A"); // Replace subsequent colons - out.push_str(drive); - out.push_str(filepath); - PathBuf::from(out) - }; + // #[cfg(target_os = "windows")] + // let path = { + // let path = path.to_string_lossy(); + + // // Assume the first char is the drive letter and the next is a colon. + // let mut out = String::new(); + // let drive = &path[..2]; // The drive letter and colon (e.g., "C:") + // let filepath = &path[2..].replace(':', "%3A"); // Replace subsequent colons + // out.push_str(drive); + // out.push_str(filepath); + // PathBuf::from(out) + // }; Ok(path) } @@ -1034,7 +1034,6 @@ mod tests { use super::*; #[tokio::test] - #[cfg(target_family = "unix")] async fn file_test() { let root = TempDir::new().unwrap(); let integration = LocalFileSystem::new_with_prefix(root.path()).unwrap(); @@ -1051,7 +1050,6 @@ mod tests { } #[test] - #[cfg(target_family = "unix")] fn test_non_tokio() { let root = TempDir::new().unwrap(); let integration = LocalFileSystem::new_with_prefix(root.path()).unwrap(); From 0c9c04b3d57286309c729daf33699e5e71504921 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Tue, 2 Jul 2024 12:04:41 +0200 Subject: [PATCH 09/11] Restore changes again --- .github/workflows/object_store.yml | 2 +- object_store/src/local.rs | 26 ++++++++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/.github/workflows/object_store.yml b/.github/workflows/object_store.yml index 9de3b3ce4cb2..bdbfc0bec4bb 100644 --- a/.github/workflows/object_store.yml +++ b/.github/workflows/object_store.yml @@ -210,4 +210,4 @@ jobs: with: submodules: true - name: Run LocalFileSystem tests - run: cargo test + run: cargo test local::tests diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 32bd0c5417ba..4d1be8b6b15c 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -295,18 +295,18 @@ impl LocalFileSystem { ); let path = self.config.prefix_to_filesystem(location)?; - // #[cfg(target_os = "windows")] - // let path = { - // let path = path.to_string_lossy(); - - // // Assume the first char is the drive letter and the next is a colon. - // let mut out = String::new(); - // let drive = &path[..2]; // The drive letter and colon (e.g., "C:") - // let filepath = &path[2..].replace(':', "%3A"); // Replace subsequent colons - // out.push_str(drive); - // out.push_str(filepath); - // PathBuf::from(out) - // }; + #[cfg(target_os = "windows")] + let path = { + let path = path.to_string_lossy(); + + // Assume the first char is the drive letter and the next is a colon. + let mut out = String::new(); + let drive = &path[..2]; // The drive letter and colon (e.g., "C:") + let filepath = &path[2..].replace(':', "%3A"); // Replace subsequent colons + out.push_str(drive); + out.push_str(filepath); + PathBuf::from(out) + }; Ok(path) } @@ -1034,6 +1034,7 @@ mod tests { use super::*; #[tokio::test] + #[cfg(target_family = "unix")] async fn file_test() { let root = TempDir::new().unwrap(); let integration = LocalFileSystem::new_with_prefix(root.path()).unwrap(); @@ -1050,6 +1051,7 @@ mod tests { } #[test] + #[cfg(target_family = "unix")] fn test_non_tokio() { let root = TempDir::new().unwrap(); let integration = LocalFileSystem::new_with_prefix(root.path()).unwrap(); From 04c3b10068f2517beac8030a03d31d3612f6e10c Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Thu, 11 Jul 2024 14:37:03 +0200 Subject: [PATCH 10/11] Add back newline (removed by mistake) --- object_store/src/local.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 3ebb58ffd5af..99d5bae50aa3 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -1535,6 +1535,7 @@ mod tests { .bytes() .await .unwrap(); + assert_eq!(&*read_data, data); assert!(fs::read_dir(root.path()).unwrap().count() > 0); integration.delete(&location).await.unwrap(); From 7552b3882ee96a03065419d25bb7a43984ef0ff9 Mon Sep 17 00:00:00 2001 From: Hesam Pakdaman Date: Thu, 11 Jul 2024 14:42:59 +0200 Subject: [PATCH 11/11] Fix test after merge with master --- object_store/src/local.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/object_store/src/local.rs b/object_store/src/local.rs index 99d5bae50aa3..db4b4b05031e 100644 --- a/object_store/src/local.rs +++ b/object_store/src/local.rs @@ -1511,6 +1511,12 @@ mod tests { assert_eq!(list, vec![path.clone()]); let result = integration + .get(&location) + .await + .unwrap() + .bytes() + .await + .unwrap(); assert_eq!(result, Bytes::from("test")); }