From 35ad5bf52a13a9180bcece8b06071d845e651b5d Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Fri, 3 Jul 2026 11:56:38 +0000 Subject: [PATCH] Centralise path normalisation in vortex-file module Existing logic was duplicated between jni and duckdb with slight differences. JNI logic didn't always handle local paths. Signed-off-by: "robert" Signed-off-by: Robert Kruszewski --- Cargo.lock | 1 + .../spark/VortexDataSourceWriteTest.java | 39 ++++ vortex-duckdb/src/multi_file.rs | 180 +--------------- vortex-file/Cargo.toml | 1 + vortex-file/src/multi/mod.rs | 2 + vortex-file/src/multi/uri.rs | 203 ++++++++++++++++++ vortex-jni/src/data_source.rs | 88 +------- vortex-jni/src/file.rs | 11 +- vortex-jni/src/writer.rs | 27 ++- 9 files changed, 269 insertions(+), 283 deletions(-) create mode 100644 vortex-file/src/multi/uri.rs diff --git a/Cargo.lock b/Cargo.lock index d99d2124e45..3dd1da9ced0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10321,6 +10321,7 @@ dependencies = [ "rstest", "tokio", "tracing", + "url", "vortex-alp", "vortex-array", "vortex-btrblocks", diff --git a/java/vortex-spark/src/test/java/dev/vortex/spark/VortexDataSourceWriteTest.java b/java/vortex-spark/src/test/java/dev/vortex/spark/VortexDataSourceWriteTest.java index 325da0bc98c..90fb1959ade 100644 --- a/java/vortex-spark/src/test/java/dev/vortex/spark/VortexDataSourceWriteTest.java +++ b/java/vortex-spark/src/test/java/dev/vortex/spark/VortexDataSourceWriteTest.java @@ -129,6 +129,45 @@ public void testWriteAndReadVortexFiles() throws IOException { verifyDataContent(originalDf, readDf); } + @Test + @DisplayName("Write and read Vortex files with a bare path (no URI scheme)") + public void testWriteAndReadWithBarePath() throws IOException { + int numRows = 50; + Dataset originalDf = createTestDataFrame(numRows); + + // A bare filesystem path, without a file:// scheme. + String barePath = tempDir.resolve("bare_path_output").toString(); + originalDf + .write() + .format("vortex") + .option("path", barePath) + .mode(SaveMode.Overwrite) + .save(); + + assertFalse(findVortexFiles(tempDir.resolve("bare_path_output")).isEmpty(), "Write should create files"); + + // Reading a bare directory path exercises schema inference via file listing. + Dataset readDf = + spark.read().format("vortex").option("path", barePath).load(); + + assertSchemaEquals(originalDf.schema(), readDf.schema()); + assertEquals(numRows, readDf.count(), "Read DataFrame should have same number of rows as original"); + verifyDataContent(originalDf, readDf); + + // Overwriting a bare path exercises file listing and deletion of the existing files. + Dataset replacementDf = createTestDataFrame(25); + replacementDf + .write() + .format("vortex") + .option("path", barePath) + .mode(SaveMode.Overwrite) + .save(); + + Dataset reread = + spark.read().format("vortex").option("path", barePath).load(); + assertEquals(25, reread.count(), "Should have data from second write after overwrite"); + } + @Test @DisplayName("Write empty DataFrame as Vortex") public void testWriteEmptyDataFrame() throws IOException { diff --git a/vortex-duckdb/src/multi_file.rs b/vortex-duckdb/src/multi_file.rs index bb9e015af5c..247b6a19909 100644 --- a/vortex-duckdb/src/multi_file.rs +++ b/vortex-duckdb/src/multi_file.rs @@ -1,8 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright the Vortex contributors -use std::path::Path; -use std::path::absolute; use std::sync::Arc; use itertools::Itertools; @@ -14,6 +12,7 @@ use vortex::error::VortexResult; use vortex::error::vortex_bail; use vortex::error::vortex_err; use vortex::file::multi::MultiFileDataSource; +use vortex::file::multi::parse_uri_or_path; use vortex::io::compat::Compat; use vortex::io::filesystem::FileSystemRef; use vortex::io::object_store::ObjectStoreFileSystem; @@ -26,38 +25,6 @@ use crate::SESSION; use crate::duckdb::BindInputRef; use crate::duckdb::ExtractedValue; -/// Parse a glob string into a [`Url`]. -/// -/// Accepts full URLs (e.g. `s3://bucket/prefix/*.vortex`, `file:///data/*.vortex`) as well as -/// bare file paths. For bare paths, the path is made absolute (without requiring it to exist) -/// so that relative paths such as `./data/*.vortex` or `../data/*.vortex` are resolved correctly. -fn parse_glob_url(glob_url_str: &str) -> VortexResult { - Url::parse(glob_url_str).or_else(|_| { - let path = absolute(Path::new(glob_url_str)) - .map_err(|e| vortex_err!("Failed making {glob_url_str} absolute: {e}"))?; - // `absolute()` does not normalize `..` components, so `/a/b/../c` stays as-is. - // Normalizing manually avoids `..` being percent-encoded in the resulting URL. - let path = normalize_path(path); - Url::from_file_path(path).map_err(|_| vortex_err!("Neither URL nor path: {glob_url_str}")) - }) -} - -/// Normalize a path by resolving `.` and `..` components without accessing the filesystem. -fn normalize_path(path: std::path::PathBuf) -> std::path::PathBuf { - use std::path::Component; - let mut normalized = std::path::PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::ParentDir => { - normalized.pop(); - } - c => normalized.push(c), - } - } - normalized -} - fn resolve_filesystem(base_url: &Url) -> VortexResult { let object_store: Arc = match base_url.scheme() { "file" => Arc::new(LocalFileSystem::new()), @@ -104,7 +71,7 @@ pub fn bind_multi_file_scan(input: &BindInputRef) -> VortexResult = Vec::with_capacity(glob_strings.len()); for glob_str in &glob_strings { - glob_urls.push(parse_glob_url(glob_str)?); + glob_urls.push(parse_uri_or_path(glob_str)?); } // Cache filesystems by base URL to avoid resolving the same filesystem multiple times. @@ -134,146 +101,3 @@ pub fn bind_multi_file_scan(input: &BindInputRef) -> VortexResult VortexResult<()> { - let url = parse_glob_url("s3://my-bucket/prefix/*.vortex")?; - assert_eq!(url.scheme(), "s3"); - assert_eq!(url.host_str(), Some("my-bucket")); - assert_eq!(url.path(), "/prefix/*.vortex"); - Ok(()) - } - - #[test] - fn test_parse_glob_url_file_scheme() -> VortexResult<()> { - let url = parse_glob_url("file:///absolute/path/data.vortex")?; - assert_eq!(url.scheme(), "file"); - assert_eq!(url.path(), "/absolute/path/data.vortex"); - Ok(()) - } - - #[test] - fn test_parse_glob_url_absolute_glob_path() -> VortexResult<()> { - let tmpdir = tempfile::tempdir()?; - let glob = format!("{}/*.vortex", tmpdir.path().display()); - let url = parse_glob_url(&glob)?; - assert_eq!(url.scheme(), "file"); - assert!(url.path().ends_with("/*.vortex")); - Ok(()) - } - - #[test] - fn test_parse_glob_url_absolute_existing_path() -> VortexResult<()> { - let tmpfile = tempfile::NamedTempFile::new()?; - let canonical = std::fs::canonicalize(tmpfile.path())?; - let path_str = canonical - .to_str() - .ok_or_else(|| vortex_err!("canonical path is not valid UTF-8"))?; - let url = parse_glob_url(path_str)?; - assert_eq!(url.scheme(), "file"); - assert_eq!(url.path(), path_str); - Ok(()) - } - - #[test] - fn test_parse_glob_url_relative_path() -> VortexResult<()> { - // Create a tempfile in the current working directory so we can refer to it - // by a relative name (just the filename, without any directory component). - let tmpfile = tempfile::NamedTempFile::new_in(".")?; - let filename = tmpfile - .path() - .file_name() - .ok_or_else(|| vortex_err!("temp file missing file name"))? - .to_str() - .ok_or_else(|| vortex_err!("temp file name is not valid UTF-8"))?; - - let url = parse_glob_url(filename)?; - assert_eq!(url.scheme(), "file"); - // The relative name must have been resolved to an absolute path. - assert!(url.path().ends_with(filename)); - assert!(url.path().starts_with('/')); - Ok(()) - } - - #[test] - fn test_parse_glob_url_relative_glob_path() -> VortexResult<()> { - // A relative path with a glob character (e.g. `./data/*.vortex`) must also resolve - // correctly. - let tmpdir = tempfile::tempdir_in(".")?; - let dir_name = tmpdir - .path() - .file_name() - .ok_or_else(|| vortex_err!("temp dir missing file name"))? - .to_str() - .ok_or_else(|| vortex_err!("temp dir name is not valid UTF-8"))?; - let glob = format!("./{dir_name}/*.vortex"); - let url = parse_glob_url(&glob)?; - assert_eq!(url.scheme(), "file"); - assert!(url.path().starts_with('/')); - assert!(url.path().ends_with("/*.vortex")); - Ok(()) - } - - #[test] - fn test_parse_glob_url_nonexistent_path() -> VortexResult<()> { - // absolute() does not require the path to exist, so a non-existent path succeeds. - let url = parse_glob_url("/nonexistent/path/file.vortex")?; - assert_eq!(url.scheme(), "file"); - assert_eq!(url.path(), "/nonexistent/path/file.vortex"); - Ok(()) - } - - #[test] - fn test_parse_glob_url_parent_relative_path() -> VortexResult<()> { - // A path starting with `..` must be resolved to an absolute path without - // percent-encoding the `..` component in the resulting URL. - let tmpfile = tempfile::NamedTempFile::new_in("..")?; - let filename = tmpfile - .path() - .file_name() - .ok_or_else(|| vortex_err!("temp file missing file name"))? - .to_str() - .ok_or_else(|| vortex_err!("temp file name is not valid UTF-8"))?; - let relative = format!("../{filename}"); - - let url = parse_glob_url(&relative)?; - assert_eq!(url.scheme(), "file"); - // The resolved path must be absolute and must not contain encoded dots. - assert!(url.path().starts_with('/')); - assert!( - !url.path().contains("%2E"), - "path must not contain percent-encoded dots" - ); - assert!(url.path().ends_with(filename)); - Ok(()) - } - - // Use absolute paths so the expected result is cwd-independent. - #[rstest] - #[case("/a/./b", "/a/b")] - #[case("/a/b/./c", "/a/b/c")] - #[case("/a/../b", "/b")] - #[case("/a/b/../c", "/a/c")] - #[case("/a/b/../../c", "/c")] - #[case("/a/./b/.././c", "/a/c")] - #[case("/a/b/../..", "/")] - fn test_parse_glob_url_dot_normalization( - #[case] input: &str, - #[case] expected_path: &str, - ) -> VortexResult<()> { - let url = parse_glob_url(input)?; - assert_eq!(url.scheme(), "file"); - assert_eq!( - url.path(), - expected_path, - "input {input:?} should normalize to {expected_path:?}" - ); - Ok(()) - } -} diff --git a/vortex-file/Cargo.toml b/vortex-file/Cargo.toml index 752ccbc753d..a7af1b293dd 100644 --- a/vortex-file/Cargo.toml +++ b/vortex-file/Cargo.toml @@ -30,6 +30,7 @@ parking_lot = { workspace = true } pin-project-lite = { workspace = true } tokio = { workspace = true, features = ["rt"], optional = true } tracing = { workspace = true } +url = { workspace = true } vortex-alp = { workspace = true } vortex-array = { workspace = true } vortex-btrblocks = { workspace = true } diff --git a/vortex-file/src/multi/mod.rs b/vortex-file/src/multi/mod.rs index 215331f0540..587768f17d0 100644 --- a/vortex-file/src/multi/mod.rs +++ b/vortex-file/src/multi/mod.rs @@ -4,6 +4,7 @@ //! Builder for constructing a [`MultiLayoutDataSource`] from multiple Vortex files. mod session; +mod uri; use std::sync::Arc; @@ -14,6 +15,7 @@ use futures::stream; pub use session::MultiFileSession; use session::MultiFileSessionExt; use tracing::debug; +pub use uri::parse_uri_or_path; use vortex_error::VortexError; use vortex_error::VortexResult; use vortex_error::vortex_bail; diff --git a/vortex-file/src/multi/uri.rs b/vortex-file/src/multi/uri.rs new file mode 100644 index 00000000000..821eb008e7f --- /dev/null +++ b/vortex-file/src/multi/uri.rs @@ -0,0 +1,203 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Parsing of user-supplied URI-or-path strings into URLs, shared by the language bindings +//! that construct a [`MultiFileDataSource`](super::MultiFileDataSource). + +#[cfg(any( + unix, + windows, + target_os = "redox", + target_os = "wasi", + target_os = "hermit" +))] +use std::path::Component; +#[cfg(any( + unix, + windows, + target_os = "redox", + target_os = "wasi", + target_os = "hermit" +))] +use std::path::Path; +#[cfg(any( + unix, + windows, + target_os = "redox", + target_os = "wasi", + target_os = "hermit" +))] +use std::path::PathBuf; +#[cfg(any( + unix, + windows, + target_os = "redox", + target_os = "wasi", + target_os = "hermit" +))] +use std::path::absolute; + +use url::Url; +use vortex_error::VortexResult; +use vortex_error::vortex_err; + +/// Parse a URI-or-path string into a [`Url`]: +/// * full URLs (`s3://...`, `file:///...`) are used as-is, +/// * bare (relative or absolute) file paths are made absolute, have `.`/`..` components +/// normalized, and become `file://` URLs. On targets without a filesystem (e.g. +/// `wasm32-unknown-unknown`), bare paths are rejected instead. +/// +/// Glob characters are preserved, so glob patterns like `/data/*.vortex` parse as expected. +pub fn parse_uri_or_path(uri_or_path: &str) -> VortexResult { + // `Url::parse` accepts Windows absolute paths like `C:\foo` as a URL with a + // single-letter scheme (`c`). No real URL scheme is one character, so treat any + // single-letter scheme as a filesystem path instead. + if let Ok(url) = Url::parse(uri_or_path) + && url.scheme().len() > 1 + { + return Ok(url); + } + file_path_to_url(uri_or_path) +} + +/// Convert a bare filesystem path into a `file://` URL, absolutizing it and normalizing +/// `.`/`..` components. The cfg predicate matches `Url::from_file_path`'s availability in +/// the `url` crate. +#[cfg(any( + unix, + windows, + target_os = "redox", + target_os = "wasi", + target_os = "hermit" +))] +fn file_path_to_url(path: &str) -> VortexResult { + let abs = + absolute(Path::new(path)).map_err(|e| vortex_err!("failed to absolutize {path}: {e}"))?; + Url::from_file_path(normalize_path(abs)) + .map_err(|_| vortex_err!("neither URL nor path: {path}")) +} + +/// `Url::from_file_path` does not exist on targets without a filesystem, such as +/// `wasm32-unknown-unknown`, so bare paths cannot be interpreted there. +#[cfg(not(any( + unix, + windows, + target_os = "redox", + target_os = "wasi", + target_os = "hermit" +)))] +fn file_path_to_url(path: &str) -> VortexResult { + Err(vortex_err!( + "bare file paths are not supported on this platform: {path}" + )) +} + +/// Normalize `.` and `..` without touching the filesystem. +#[cfg(any( + unix, + windows, + target_os = "redox", + target_os = "wasi", + target_os = "hermit" +))] +fn normalize_path(path: PathBuf) -> PathBuf { + let mut out = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + out.pop(); + } + c => out.push(c), + } + } + out +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + + use super::*; + + #[test] + fn test_full_url() -> VortexResult<()> { + let url = parse_uri_or_path("s3://bucket/prefix/*.vortex")?; + assert_eq!(url.scheme(), "s3"); + assert_eq!(url.host_str(), Some("bucket")); + assert_eq!(url.path(), "/prefix/*.vortex"); + Ok(()) + } + + #[test] + fn test_file_scheme() -> VortexResult<()> { + let url = parse_uri_or_path("file:///absolute/path/data.vortex")?; + assert_eq!(url.scheme(), "file"); + assert_eq!(url.path(), "/absolute/path/data.vortex"); + Ok(()) + } + + #[test] + fn test_absolute_path() -> VortexResult<()> { + // Use a drive-prefixed input on Windows so `absolute()` doesn't inject the cwd drive + // and the expected URL path is predictable. The path does not need to exist. + #[cfg(unix)] + let (input, expected_path) = ("/tmp/data/*.vortex", "/tmp/data/*.vortex"); + #[cfg(windows)] + let (input, expected_path) = (r"C:\tmp\data\*.vortex", "/C:/tmp/data/*.vortex"); + let url = parse_uri_or_path(input)?; + assert_eq!(url.scheme(), "file"); + assert_eq!(url.path(), expected_path); + Ok(()) + } + + #[test] + fn test_relative_path_resolves_against_cwd() -> VortexResult<()> { + for input in ["data/table", "./data/table", "../data/table"] { + let url = parse_uri_or_path(input)?; + assert_eq!(url.scheme(), "file"); + assert!(url.path().starts_with('/')); + assert!(url.path().ends_with("/data/table")); + assert!( + !url.path().contains("%2E"), + "path must not contain percent-encoded dots" + ); + } + Ok(()) + } + + #[test] + fn test_single_letter_scheme_is_path() -> VortexResult<()> { + // Regression: `Url::parse("C:\\tmp")` succeeds with scheme="c"; the function must + // treat that as a filesystem path, not a URL. Exercised on all platforms because + // the check lives in `parse_uri_or_path`, not in an OS-specific branch. + let url = parse_uri_or_path(r"C:\tmp\data\*.vortex")?; + assert_eq!(url.scheme(), "file"); + assert_ne!(url.scheme(), "c"); + Ok(()) + } + + // Use absolute paths so the expected result is cwd-independent. + #[cfg(unix)] + #[rstest] + #[case("/a/./b", "/a/b")] + #[case("/a/b/./c", "/a/b/c")] + #[case("/a/../b", "/b")] + #[case("/a/b/../c", "/a/c")] + #[case("/a/b/../../c", "/c")] + #[case("/a/./b/.././c", "/a/c")] + #[case("/a/b/../..", "/")] + fn test_dot_normalization( + #[case] input: &str, + #[case] expected_path: &str, + ) -> VortexResult<()> { + let url = parse_uri_or_path(input)?; + assert_eq!(url.scheme(), "file"); + assert_eq!( + url.path(), + expected_path, + "input {input:?} should normalize to {expected_path:?}" + ); + Ok(()) + } +} diff --git a/vortex-jni/src/data_source.rs b/vortex-jni/src/data_source.rs index 5f244998f67..ab86e78a056 100644 --- a/vortex-jni/src/data_source.rs +++ b/vortex-jni/src/data_source.rs @@ -4,16 +4,10 @@ //! JNI bindings for [`vortex::scan::DataSource`] (see the equivalent types in //! `vortex-ffi/src/data_source.rs`). //! -//! Glob handling mirrors `vortex-duckdb`'s `VortexMultiFileScan`: -//! * full URLs (`s3://...`, `file:///...`) are used as-is, -//! * bare file paths are made absolute and have `.`/`..` components normalized, -//! * filesystems are cached per base URL so repeated globs against the same bucket share -//! a single client. +//! Globs are parsed with [`parse_uri_or_path`], so full URLs (`s3://...`, `file:///...`) +//! and bare file paths are both accepted. Filesystems are cached per base URL so repeated +//! globs against the same bucket share a single client. -use std::path::Component; -use std::path::Path; -use std::path::PathBuf; -use std::path::absolute; use std::sync::Arc; use jni::EnvUnowned; @@ -28,6 +22,7 @@ use vortex::error::VortexResult; use vortex::error::vortex_err; use vortex::expr::stats::Precision; use vortex::file::multi::MultiFileDataSource; +use vortex::file::multi::parse_uri_or_path; use vortex::io::filesystem::FileSystemRef; use vortex::io::runtime::BlockingRuntime; use vortex::io::session::RuntimeSessionExt; @@ -91,7 +86,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeDataSource_open( let glob_urls: Vec = glob_strings .iter() - .map(|g| parse_glob_url(g.as_str())) + .map(|g| parse_uri_or_path(g.as_str())) .collect::>()?; let mut fs_cache: HashMap = HashMap::new(); @@ -120,38 +115,6 @@ pub extern "system" fn Java_dev_vortex_jni_NativeDataSource_open( }) } -/// Parse a glob string into a [`Url`]. Accepts full URLs and bare (relative or absolute) -/// file paths — see the module docs for details. -fn parse_glob_url(glob: &str) -> VortexResult { - // `Url::parse` accepts Windows absolute paths like `C:\foo` as a URL with a - // single-letter scheme (`c`). No real URL scheme is one character, so treat any - // single-letter scheme as a filesystem path instead. - if let Ok(url) = Url::parse(glob) - && url.scheme().len() > 1 - { - return Ok(url); - } - let path = - absolute(Path::new(glob)).map_err(|e| vortex_err!("failed to absolutize {glob}: {e}"))?; - let path = normalize_path(path); - Url::from_file_path(path).map_err(|_| vortex_err!("neither URL nor path: {glob}")) -} - -/// Normalize `.` and `..` without touching the filesystem. -fn normalize_path(path: PathBuf) -> PathBuf { - let mut out = PathBuf::new(); - for component in path.components() { - match component { - Component::CurDir => {} - Component::ParentDir => { - out.pop(); - } - c => out.push(c), - } - } - out -} - /// URL with the path cleared, used as a cache key for filesystem reuse. fn base_url(url: &Url) -> Url { let mut base = url.clone(); @@ -236,47 +199,6 @@ pub extern "system" fn Java_dev_vortex_jni_NativeDataSource_byteSize( mod tests { use super::*; - #[test] - fn test_parse_glob_url_full_url() { - let url = parse_glob_url("s3://bucket/prefix/*.vortex").unwrap(); - assert_eq!(url.scheme(), "s3"); - assert_eq!(url.host_str(), Some("bucket")); - assert_eq!(url.path(), "/prefix/*.vortex"); - } - - #[test] - fn test_parse_glob_url_absolute_path() { - // Use a drive-prefixed input on Windows so `absolute()` doesn't inject the cwd drive - // and the expected URL path is predictable. - #[cfg(unix)] - let (input, expected_path) = ("/tmp/data/*.vortex", "/tmp/data/*.vortex"); - #[cfg(windows)] - let (input, expected_path) = (r"C:\tmp\data\*.vortex", "/C:/tmp/data/*.vortex"); - let url = parse_glob_url(input).unwrap(); - assert_eq!(url.scheme(), "file"); - assert_eq!(url.path(), expected_path); - } - - #[test] - fn test_parse_glob_url_normalizes_dots() { - #[cfg(unix)] - let (input, expected_path) = ("/a/b/../c/./d", "/a/c/d"); - #[cfg(windows)] - let (input, expected_path) = (r"C:\a\b\..\c\.\d", "/C:/a/c/d"); - let url = parse_glob_url(input).unwrap(); - assert_eq!(url.path(), expected_path); - } - - #[test] - fn test_parse_glob_url_single_letter_scheme_is_path() { - // Regression: `Url::parse("C:\\tmp")` succeeds with scheme="c"; the function must - // treat that as a filesystem path, not a URL. Exercised on all platforms because - // the check lives in `parse_glob_url`, not in an OS-specific branch. - let url = parse_glob_url(r"C:\tmp\data\*.vortex").unwrap(); - assert_eq!(url.scheme(), "file"); - assert_ne!(url.scheme(), "c"); - } - #[test] fn test_base_url_strips_path() { let url = Url::parse("s3://bucket/a/b/c").unwrap(); diff --git a/vortex-jni/src/file.rs b/vortex-jni/src/file.rs index 3abb6ef9ffb..343215e533a 100644 --- a/vortex-jni/src/file.rs +++ b/vortex-jni/src/file.rs @@ -13,9 +13,9 @@ use jni::objects::JString; use jni::sys::jlong; use jni::sys::jobject; use object_store::path::Path; -use url::Url; use vortex::error::VortexResult; use vortex::error::vortex_err; +use vortex::file::multi::parse_uri_or_path; use vortex::io::runtime::BlockingRuntime; use vortex::io::session::RuntimeSessionExt; use vortex::utils::aliases::hash_map::HashMap; @@ -58,10 +58,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFiles_listFiles( try_or_throw(&mut env, |env| { let session = unsafe { session_ref(session_ptr) }; let root_path: String = path.try_to_string(env)?; - - let Ok(url) = Url::parse(&root_path) else { - throw_runtime!("invalid URL: {root_path}"); - }; + let url = parse_uri_or_path(&root_path)?; let properties = extract_properties(env, &options)?; @@ -122,7 +119,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFiles_delete( return Ok(()); } - let store_url = Url::parse(&delete_uris[0]).map_err(|e| vortex_err!(External: e))?; + let store_url = parse_uri_or_path(&delete_uris[0])?; let properties = extract_properties(env, &options)?; @@ -130,7 +127,7 @@ pub extern "system" fn Java_dev_vortex_jni_NativeFiles_delete( RUNTIME.block_on(async { for uri in delete_uris { - let url = Url::parse(&uri).map_err(|e| vortex_err!(External: e))?; + let url = parse_uri_or_path(&uri)?; fs.delete(url.path()).await?; } VortexResult::Ok(()) diff --git a/vortex-jni/src/writer.rs b/vortex-jni/src/writer.rs index 95d3ae1c400..78e7c268f19 100644 --- a/vortex-jni/src/writer.rs +++ b/vortex-jni/src/writer.rs @@ -27,7 +27,6 @@ use jni::sys::jboolean; use jni::sys::jlong; use object_store::ObjectStore; use object_store::path::Path as ObjectStorePath; -use url::Url; use vortex::array::ArrayRef; use vortex::array::VTable; use vortex::array::arrow::ArrowSessionExt; @@ -40,6 +39,7 @@ use vortex::error::vortex_err; use vortex::file::WriteOptionsSessionExt; use vortex::file::WriteStrategyBuilder; use vortex::file::WriteSummary; +use vortex::file::multi::parse_uri_or_path; use vortex::io::VortexWrite; use vortex::io::compat::Compat; use vortex::io::object_store::ObjectStoreWrite; @@ -71,20 +71,17 @@ fn resolve_store( url_or_path: &str, properties: &HashMap, ) -> VortexResult { - match Url::parse(url_or_path) { - Ok(url) if url.scheme() == "file" => { - let path = url - .to_file_path() - .map_err(|_| vortex_err!("invalid file URL: {url_or_path}"))?; - Ok(ResolvedStore::Path(path)) - } - Ok(url) => { - let path = ObjectStorePath::from_url_path(url.path()) - .map_err(|_| vortex_err!("invalid object_store path: {}", url.path()))?; - let store = make_object_store(&url, properties)?; - Ok(ResolvedStore::ObjectStore(store, path)) - } - Err(_) => Ok(ResolvedStore::Path(PathBuf::from(url_or_path))), + let url = parse_uri_or_path(url_or_path)?; + if url.scheme() == "file" { + let path = url + .to_file_path() + .map_err(|_| vortex_err!("invalid file URL: {url_or_path}"))?; + Ok(ResolvedStore::Path(path)) + } else { + let path = ObjectStorePath::from_url_path(url.path()) + .map_err(|_| vortex_err!("invalid object_store path: {}", url.path()))?; + let store = make_object_store(&url, properties)?; + Ok(ResolvedStore::ObjectStore(store, path)) } }