Merge branch 'develop' into myrrc/duckdb-vortex-versoin

Author: myrrc

scripts/tests/test_benchmark_reporting.py

@@ -26,7 +26,7 @@ def load_compare_module() -> ModuleType:
     return module
 
 
-def load_random_access_split_module():
+def load_random_access_split_module() -> ModuleType:
     spec = importlib.util.spec_from_file_location("random_access_split", RANDOM_ACCESS_SPLIT_SCRIPT)
     assert spec is not None
     module = importlib.util.module_from_spec(spec)

scripts/tests/test_measurement_id.py

@@ -63,7 +63,7 @@ def test_every_fact_table_is_covered() -> None:
     assert covered_tables == set(module.MEASUREMENT_ID_BY_TABLE)
 
 
-def test_random_access_open_mode_preserves_cached_ids_and_separates_reopen():
+def test_random_access_open_mode_preserves_cached_ids_and_separates_reopen() -> None:
     module = load_measurement_id_module()
     dimensions = {
         "commit_sha": "0123456789abcdef0123456789abcdef01234567",

vortex-cloud/src/opendal/cos.rs

@@ -13,6 +13,7 @@ use vortex_utils::aliases::hash_map::HashMap;
 
 use crate::opendal::OpenDALStoreError;
 use crate::opendal::build_operator;
+use crate::opendal::property_as_bool;
 use crate::opendal::property_or_env;
 use crate::opendal::warn_on_unknown_properties;
 
@@ -129,8 +130,7 @@ where
             &env_lookup,
         ),
         root: properties.get("root").cloned(),
-        disable_config_load: properties.get("disable_config_load").map(String::as_str)
-            == Some("true"),
+        disable_config_load: property_as_bool(properties, "disable_config_load"),
     })
 }
 

vortex-cloud/src/opendal/mod.rs

@@ -204,6 +204,26 @@ where
     properties.get(key).cloned().or_else(|| env_lookup(env_var))
 }
 
+/// Take `key` from `properties` as a boolean, accepting the spellings `object_store` accepts in
+/// its own configuration: `1`/`true`/`on`/`yes`/`y` and their negatives, case-insensitively.
+///
+/// An absent key is `false`. A value that is not a boolean is warned about and read as `false`,
+/// which is how [`warn_on_unknown_properties`] already treats a key the service cannot use.
+#[cfg(any(feature = "cos", feature = "oss"))]
+pub(crate) fn property_as_bool(properties: &HashMap<String, String>, key: &str) -> bool {
+    let Some(value) = properties.get(key) else {
+        return false;
+    };
+    match value.trim().to_ascii_lowercase().as_str() {
+        "1" | "true" | "on" | "yes" | "y" => true,
+        "0" | "false" | "off" | "no" | "n" => false,
+        _ => {
+            warn!("ignoring OpenDAL store property {key}: `{value}` is not a boolean");
+            false
+        }
+    }
+}
+
 /// Log a warning for every property key the service does not recognize.
 #[cfg(any(feature = "cos", feature = "goosefs", feature = "oss"))]
 pub(crate) fn warn_on_unknown_properties(properties: &HashMap<String, String>, known: &[&str]) {
@@ -265,4 +285,38 @@ mod tests {
             );
         }
     }
+
+    /// The spellings `object_store` accepts for its own boolean configuration, so that the same
+    /// property means the same thing whether a URL resolves to a native store or an OpenDAL one.
+    /// Values reach us verbatim from the caller's property map, hence the case and whitespace
+    /// cases; anything that is not a boolean stays `false` rather than becoming an error.
+    #[cfg(any(feature = "cos", feature = "oss"))]
+    #[rstest::rstest]
+    #[case("true", true)]
+    #[case("True", true)]
+    #[case("TRUE", true)]
+    #[case(" true ", true)]
+    #[case("1", true)]
+    #[case("on", true)]
+    #[case("yes", true)]
+    #[case("y", true)]
+    #[case("false", false)]
+    #[case("False", false)]
+    #[case("0", false)]
+    #[case("off", false)]
+    #[case("no", false)]
+    #[case("n", false)]
+    #[case("maybe", false)]
+    #[case("", false)]
+    fn property_as_bool_matches_object_store(#[case] value: &str, #[case] expected: bool) {
+        let mut props = HashMap::new();
+        props.insert("skip_signature".to_string(), value.to_string());
+        assert_eq!(property_as_bool(&props, "skip_signature"), expected);
+    }
+
+    #[cfg(any(feature = "cos", feature = "oss"))]
+    #[test]
+    fn property_as_bool_is_false_when_absent() {
+        assert!(!property_as_bool(&HashMap::new(), "skip_signature"));
+    }
 }

vortex-cloud/src/opendal/oss.rs

@@ -13,6 +13,7 @@ use vortex_utils::aliases::hash_map::HashMap;
 
 use crate::opendal::OpenDALStoreError;
 use crate::opendal::build_operator;
+use crate::opendal::property_as_bool;
 use crate::opendal::property_or_env;
 use crate::opendal::warn_on_unknown_properties;
 
@@ -140,7 +141,7 @@ where
             &env_lookup,
         ),
         root: properties.get("root").cloned(),
-        skip_signature: properties.get("skip_signature").map(String::as_str) == Some("true"),
+        skip_signature: property_as_bool(properties, "skip_signature"),
     })
 }
 

vortex-ffi/cmake/tests/support.py

@@ -13,12 +13,20 @@
 import textwrap
 import unittest
 from pathlib import Path
+from typing import Any, TypedDict, Unpack
 
 REPO_ROOT = Path(__file__).resolve().parents[3]
 CMAKE_DIR = REPO_ROOT / "vortex-ffi/cmake"
 
 
-def rust_toolchain_environment():
+class CommandOptions(TypedDict, total=False):
+    env: dict[str, str] | None
+    cwd: str | Path | None
+    success: bool
+    timeout: int
+
+
+def rust_toolchain_environment() -> dict[str, str]:
     """Keep the repository's Rust selection when fixtures run outside its tree."""
     env = os.environ.copy()
     if not env.get("RUSTUP_TOOLCHAIN") and shutil.which("rustup"):
@@ -36,25 +44,32 @@ def rust_toolchain_environment():
 
 
 class CMakeTest(unittest.TestCase):
-    def setUp(self):
+    def setUp(self) -> None:
         temporary = tempfile.TemporaryDirectory(prefix="vortex-cmake-")
         self.addCleanup(temporary.cleanup)
         self.work = Path(temporary.name).resolve()
         self.repo = REPO_ROOT
         self.env = os.environ.copy()
 
-    def write(self, path, contents):
+    def write(self, path: str | Path, contents: str) -> Path:
         path = self.work / path
         path.parent.mkdir(parents=True, exist_ok=True)
         path.write_text(textwrap.dedent(contents), encoding="utf-8")
         return path
 
-    def executable(self, name, body):
+    def executable(self, name: str, body: str) -> str:
         path = self.write(name, f"#!{sys.executable}\n" + textwrap.dedent(body))
         path.chmod(0o755)
         return str(path)
 
-    def command(self, *args, env=None, cwd=None, success=True, timeout=120):
+    def command(
+        self,
+        *args: str | Path,
+        env: dict[str, str] | None = None,
+        cwd: str | Path | None = None,
+        success: bool = True,
+        timeout: int = 120,
+    ) -> subprocess.CompletedProcess[str]:
         result = subprocess.run(
             list(map(str, args)),
             cwd=self.work if cwd is None else cwd,
@@ -71,13 +86,17 @@ def command(self, *args, env=None, cwd=None, success=True, timeout=120):
             self.assertNotEqual(result.returncode, 0, output)
         return result
 
-    def cmake_configure(self, source, build, *options, generator="Ninja", **kwargs):
+    def cmake_configure(
+        self, source: Path, build: Path, *options: str, generator: str = "Ninja", **kwargs: Unpack[CommandOptions]
+    ) -> subprocess.CompletedProcess[str]:
         return self.command("cmake", "-G", generator, "-S", source, "-B", build, *options, **kwargs)
 
-    def cmake_build(self, build, *options, **kwargs):
+    def cmake_build(
+        self, build: Path, *options: str, **kwargs: Unpack[CommandOptions]
+    ) -> subprocess.CompletedProcess[str]:
         return self.command("cmake", "--build", build, *options, **kwargs)
 
-    def recording_cargo(self):
+    def recording_cargo(self) -> str:
         """Record the handoff and emit an archive without compiling Vortex."""
         return self.executable(
             "cargo",
@@ -96,7 +115,7 @@ def option(name):
             """,
         )
 
-    def fake_rustc(self, release="1.95.0", host=None):
+    def fake_rustc(self, release: str = "1.95.0", host: str | None = None) -> str:
         arch = {"arm64": "aarch64", "AMD64": "x86_64"}.get(platform.machine(), platform.machine())
         host = host or f"{arch}-{'apple-darwin' if sys.platform == 'darwin' else 'unknown-linux-gnu'}"
         return self.executable(
@@ -109,5 +128,5 @@ def fake_rustc(self, release="1.95.0", host=None):
             """,
         )
 
-    def cargo_recording(self, target_dir):
+    def cargo_recording(self, target_dir: Path) -> dict[str, Any]:
         return json.loads((target_dir / "environment.json").read_text(encoding="utf-8"))

vortex-ffi/cmake/tests/test_compiler_commands.py

@@ -7,19 +7,20 @@
 import json
 import shlex
 import shutil
+import subprocess
 import tomllib
 import unittest
 from pathlib import Path
 
 from support import CMakeTest, rust_toolchain_environment
 
 
-def snapshot(*paths):
+def snapshot(*paths: Path) -> dict[Path, tuple[int, bytes]]:
     return {path: (path.stat().st_mtime_ns, hashlib.sha256(path.read_bytes()).digest()) for path in paths}
 
 
 class CompilerCommandTests(CMakeTest):
-    def setUp(self):
+    def setUp(self) -> None:
         super().setUp()
         packages = tomllib.loads((self.repo / "Cargo.lock").read_text())["package"]
         [cc] = [package for package in packages if package["name"] == "cc"]
@@ -50,7 +51,9 @@ def setUp(self):
         self.env.update(CARGO_NET_OFFLINE="true", CARGO_BUILD_JOBS="2")
         self.command("cargo", "generate-lockfile", "--offline", cwd=self.source)
 
-    def configure(self, value=7, argument=7, policy=True, instrumentation=False, generator="Ninja", build_name=None):
+    def configure(
+        self, value=7, argument=7, policy=True, instrumentation=False, generator="Ninja", build_name=None
+    ) -> None:
         self.build_dir = self.work / (build_name or f"{generator} build directory's")
         self.target_dir = self.build_dir / "ffi/cargo-target"
         options = []
@@ -70,15 +73,15 @@ def configure(self, value=7, argument=7, policy=True, instrumentation=False, gen
             ]
         self.cmake_configure(self.source, self.build_dir, "-DCMAKE_BUILD_TYPE=Debug", *options, generator=generator)
 
-    def build(self, target="vortex_ffi_cargo_build", success=True):
+    def build(self, target: str = "vortex_ffi_cargo_build", success: bool = True) -> subprocess.CompletedProcess[str]:
         return self.cmake_build(self.build_dir, "--target", target, success=success)
 
-    def archives(self):
+    def archives(self) -> dict[Path, tuple[int, bytes]]:
         archives = sorted(self.target_dir.rglob("libnative_*.a"))
         self.assertEqual(len(archives), 4, archives)
         return snapshot(*archives)
 
-    def test_compiler_arguments_warning_policy_and_freshness(self):
+    def test_compiler_arguments_warning_policy_and_freshness(self) -> None:
         self.configure()
         rejected = self.build("parent_native", success=False)
         self.assertIn("error: unused variable 'vendored_unused'", rejected.stdout + rejected.stderr)
@@ -103,7 +106,7 @@ def test_compiler_arguments_warning_policy_and_freshness(self):
                     self.assertNotEqual(changed[path][1], original[path][1], path)
                 original = changed
 
-    def test_header_lifecycle(self):
+    def test_header_lifecycle(self) -> None:
         # Incremental rustc changes archive member names even when the object bytes are identical.
         self.env["CARGO_INCREMENTAL"] = "0"
         for generator in ("Ninja", "Unix Makefiles"):
@@ -135,7 +138,7 @@ def test_header_lifecycle(self):
                 self.assertEqual(self.command(consumer).stdout.strip(), "2")
                 self.assertEqual(staged.read_bytes(), source_header.read_bytes())
 
-    def test_host_target_instrumentation_and_cache_boundary(self):
+    def test_host_target_instrumentation_and_cache_boundary(self) -> None:
         log = self.work / "cache calls.jsonl"
         # Record the cache boundary, not cache behavior; every call still runs the real compiler.
         self.env["RUSTC_WRAPPER"] = self.executable(

vortex-ffi/cmake/tests/test_configure.py

@@ -5,14 +5,16 @@
 
 import json
 import os
+import subprocess
 import tomllib
 import unittest
+from pathlib import Path
 
 from support import CMakeTest
 
 
 class ConfigureTests(CMakeTest):
-    def setUp(self):
+    def setUp(self) -> None:
         super().setUp()
         for name in ("CMAKE_BUILD_TYPE", "RUSTUP_TOOLCHAIN"):
             self.env.pop(name, None)
@@ -35,7 +37,9 @@ def setUp(self):
             """,
         )
 
-    def configure(self, name, *options, source=None, success=True):
+    def configure(
+        self, name: str, *options: str, source: Path | None = None, success: bool = True
+    ) -> subprocess.CompletedProcess[str]:
         return self.cmake_configure(
             source or self.repo / "vortex-ffi",
             self.work / name,
@@ -49,7 +53,7 @@ def configure(self, name, *options, source=None, success=True):
             success=success,
         )
 
-    def test_standalone_defaults_and_ffi_default_build(self):
+    def test_standalone_defaults_and_ffi_default_build(self) -> None:
         for name, source, directories in (
             ("root", ".", ("ffi", "cpp")),
             ("ffi", "vortex-ffi", (".",)),
@@ -74,7 +78,7 @@ def test_standalone_defaults_and_ffi_default_build(self):
         expected = config["target"]['cfg(target_family="unix")']["rustflags"] + ["-C", "relocation-model=pic"]
         self.assertEqual(recorded["env"]["CARGO_ENCODED_RUSTFLAGS"].split("\x1f"), expected)
 
-    def test_embedded_root_preserves_parent_variables(self):
+    def test_embedded_root_preserves_parent_variables(self) -> None:
         source = self.write(
             "parent/CMakeLists.txt",
             f"""\
@@ -98,7 +102,7 @@ def test_embedded_root_preserves_parent_variables(self):
         self.cmake_build(build)
         self.assertFalse(list(build.rglob("libvortex_ffi.a")))
 
-    def test_unused_embedded_ffi_keeps_cargo_lazy(self):
+    def test_unused_embedded_ffi_keeps_cargo_lazy(self) -> None:
         source = self.write(
             "parent/CMakeLists.txt",
             f"""\
@@ -114,7 +118,7 @@ def test_unused_embedded_ffi_keeps_cargo_lazy(self):
         self.cmake_build(build, "--target", "vortex_ffi_cargo_build")
         self.assertEqual((build / "ffi/vortex-artifacts/libvortex_ffi.a").read_bytes(), b"recorded archive")
 
-    def test_profile_mapping_and_override(self):
+    def test_profile_mapping_and_override(self) -> None:
         for config, override, expected in (
             ("Release", "", "release"),
             ("RelWithDebInfo", "", "release_debug"),
@@ -128,7 +132,7 @@ def test_profile_mapping_and_override(self):
                 self.assertEqual(args[args.index("--profile") + 1], expected)
                 self.assertEqual((build / "vortex-artifacts/libvortex_ffi.a").read_bytes(), b"recorded archive")
 
-    def test_sanitizer_rejections(self):
+    def test_sanitizer_rejections(self) -> None:
         result = self.configure("unknown", "-DVORTEX_SANITIZER=typo", success=False)
         self.assertIn("got 'typo'", result.stdout + result.stderr)
         for compiler, message in (
@@ -149,11 +153,11 @@ def test_sanitizer_rejections(self):
                 )
                 self.assertIn(message, result.stdout + result.stderr)
 
-    def test_toolchain_selection_survives_reconfigure_and_explicit_updates(self):
+    def test_toolchain_selection_survives_reconfigure_and_explicit_updates(self) -> None:
         rustc_log = self.work / "rustc.json"
         build = self.work / "toolchain"
 
-        def assert_selection(selected):
+        def assert_selection(selected: str | None) -> None:
             self.assertEqual(json.loads(rustc_log.read_text()), [selected, str(self.repo)])
             recorded = self.cargo_recording(build / "cargo-target")
             self.assertEqual(recorded["env"].get("RUSTUP_TOOLCHAIN"), selected)

vortex-ffi/cmake/tests/test_cuda_architectures.py

@@ -17,7 +17,7 @@
 
 
 class CudaArchitectureTests(CMakeTest):
-    def test_policy_preserves_parent(self):
+    def test_policy_preserves_parent(self) -> None:
         for architectures, expected in (
             (None, "-arch=native"),
             ("native", "-arch=native"),
@@ -53,7 +53,7 @@ def test_policy_preserves_parent(self):
                 else:
                     self.assertEqual((self.work / "flags.txt").read_text(encoding="utf-8"), expected)
 
-    def test_configure_forwards_flags_and_cpu_ignores_policy(self):
+    def test_configure_forwards_flags_and_cpu_ignores_policy(self) -> None:
         cuda_root = self.work / "fake CUDA toolkit's"
         nvcc = self.executable("fake CUDA toolkit's/bin/nvcc", "raise SystemExit('No native compilation expected')\n")
         # Stub discovery only; production Configure.cmake and its Cargo driver run unchanged.