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" <robert@spiraldb.com>
Signed-off-by: Robert Kruszewski <github@robertk.io>

Author: robert3005

Cargo.lock

@@ -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<Row> 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<Row> 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<Row> replacementDf = createTestDataFrame(25);
+        replacementDf
+                .write()
+                .format("vortex")
+                .option("path", barePath)
+                .mode(SaveMode.Overwrite)
+                .save();
+
+        Dataset<Row> 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 {

java/vortex-spark/src/test/java/dev/vortex/spark/VortexDataSourceWriteTest.java

@@ -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> {
-    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<FileSystemRef> {
     let object_store: Arc<dyn ObjectStore> = match base_url.scheme() {
         "file" => Arc::new(LocalFileSystem::new()),
@@ -104,7 +71,7 @@ pub fn bind_multi_file_scan(input: &BindInputRef) -> VortexResult<MultiLayoutDat
     // Parse each glob URL and resolve its filesystem.
     let mut glob_urls: Vec<Url> = 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<MultiLayoutDat
         builder.build().await
     })
 }
-
-#[cfg(test)]
-mod tests {
-    use rstest::rstest;
-
-    use super::*;
-
-    #[test]
-    fn test_parse_glob_url_s3() -> 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(())
-    }
-}

vortex-duckdb/src/multi_file.rs

@@ -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 }

vortex-file/Cargo.toml

@@ -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;