lidavidm commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2184115938
##########
rust/core/src/driver_manager.rs:
##########
@@ -184,6 +200,65 @@ impl ManagedDriver {
Ok(ManagedDriver { inner })
}
+ /// Load a driver either by name, filename, path, or via locating a toml
manifest file.
+ /// The LoadFlags control what directories are searched to locate a
manifest.
+ pub fn find_load_from_name(
Review Comment:
Shouldn't this be something like `load_dynamic_from_name`?
##########
rust/core/src/driver_manager.rs:
##########
@@ -184,6 +200,65 @@ impl ManagedDriver {
Ok(ManagedDriver { inner })
}
+ /// Load a driver either by name, filename, path, or via locating a toml
manifest file.
+ /// The LoadFlags control what directories are searched to locate a
manifest.
+ pub fn find_load_from_name(
+ name: impl AsRef<OsStr>,
+ entrypoint: Option<&[u8]>,
+ version: AdbcVersion,
+ load_flags: LoadFlags,
+ ) -> Result<Self> {
+ let driver_path = Path::new(name.as_ref());
+ let allow_relative = load_flags & LOAD_FLAG_ALLOW_RELATIVE_PATHS != 0;
+ let ext = driver_path.extension();
+ if ext.is_some() {
Review Comment:
```suggestion
if let Some(ext) = driver_path.extension() {
```
There's generally little reason to use is_some.
##########
rust/core/src/driver_manager.rs:
##########
@@ -276,6 +351,23 @@ impl ManagedDriver {
Ok(database)
}
+
+ fn load_driver_from_manifest(
+ driver_path: &Path,
+ entrypoint: Option<&[u8]>,
+ version: AdbcVersion,
+ ) -> Result<Self> {
+ let mut info = DriverInfo {
+ manifest_file: driver_path.to_path_buf(),
+ driver_name: String::new(),
+ lib_path: PathBuf::new(),
+ entrypoint: entrypoint.map(|e| e.to_vec()),
+ version: String::new(),
+ source: String::new(),
Review Comment:
I think it works to just write `Default::default()` instead of explicitly
calling `new`
##########
rust/core/src/driver_manager.rs:
##########
@@ -1310,3 +1461,489 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+const fn current_arch() -> &'static str {
+ #[cfg(target_arch = "x86_64")]
+ const ARCH: &str = "amd64";
+ #[cfg(target_arch = "aarch64")]
+ const ARCH: &str = "arm64";
+ #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
+ const ARCH: &str = std::env::consts::ARCH;
+
+ #[cfg(target_os = "macos")]
+ const OS: &str = "osx";
+ #[cfg(not(target_os = "macos"))]
+ const OS: &str = std::env::consts::OS;
+
+ #[cfg(target_env = "musl")]
+ const EXTRA: &str = "_musl";
+ #[cfg(all(target_os = "windows", target_env = "gnu"))]
+ const EXTRA: &str = "_mingw";
+ #[cfg(not(any(target_env = "musl", all(target_os = "windows", target_env =
"gnu"))))]
+ const EXTRA: &str = "";
+
+ concat!(OS, "_", ARCH, EXTRA)
Review Comment:
Instead of the extra dependency, could we just return a 3-tuple and concat
at runtime? (Or return a newtype 3-tuple with a custom Display to do the concat)
##########
rust/core/src/driver_manager.rs:
##########
@@ -316,6 +408,65 @@ impl Driver for ManagedDriver {
}
}
+fn get_optional_key(manifest: &Table, key: &str) -> String {
+ manifest
+ .get(key)
+ .and_then(Value::as_str)
+ .unwrap_or_default()
+ .to_string()
+}
+
+fn load_driver_manifest(info: &mut DriverInfo) -> Result<()> {
Review Comment:
I think it might be more idiomatic to take info by value and return it by
value (rather than requiring a mutable reference)
##########
rust/core/src/driver_manager.rs:
##########
@@ -316,6 +408,65 @@ impl Driver for ManagedDriver {
}
}
+fn get_optional_key(manifest: &Table, key: &str) -> String {
+ manifest
+ .get(key)
+ .and_then(Value::as_str)
+ .unwrap_or_default()
+ .to_string()
+}
+
+fn load_driver_manifest(info: &mut DriverInfo) -> Result<()> {
Review Comment:
You can then write
```rust
let info = load_driver_manifest(DriverInfo { ... })?;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
