This is an automated email from the ASF dual-hosted git repository.
felipecrv pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new ed2fe871c fix(rust)!: Let immutable drivers create connections (#2788)
ed2fe871c is described below
commit ed2fe871ce9f7378b9aa9a03b51a9e498dd73207
Author: Felipe Oliveira Carvalho <[email protected]>
AuthorDate: Wed May 7 11:42:00 2025 -0300
fix(rust)!: Let immutable drivers create connections (#2788)
Since the removal of the global driver lock, there is no risk of
deadlocks between the locks on the driver and on the database objects.
IIUC, this was why member functions on the database struct took a
mutable `self` parameter:
```rust
/// Initialize the given database using the loaded driver.
///
/// This uses `&mut self` to prevent a deadlock.
fn database_init(
&mut self,
mut database: ffi::FFI_AdbcDatabase,
) -> Result<ffi::FFI_AdbcDatabase> {
```
Requiring `&mut self` is overkill and requires another layer of locking
at the application side if multiple threads share a database instance
(as they should for connection pooling).
This doesn't mean we are passing a forged mutable pointer to the driver
via the FFI, the locking on the inner database ensures exclusive access
to the `FFI_AdbcDatabase`.
```rust
/// Initialize the given connection using the loaded driver.
fn connection_init(
&self,
mut connection: ffi::FFI_AdbcConnection,
) -> Result<ffi::FFI_AdbcConnection> {
let driver = self.ffi_driver();
let mut database = self.inner.database.lock().unwrap();
// ConnectionInit
let mut error = ffi::FFI_AdbcError::with_driver(driver);
let method = driver_method!(driver, ConnectionInit);
let status = unsafe { method(&mut connection, &mut *database, &mut
error) };
check_status(status, error)?;
Ok(connection)
}
```
**This might be considered a breaking change.**
Closes #2790
---
rust/core/src/driver_manager.rs | 25 ++++--------
rust/core/src/lib.rs | 4 +-
rust/core/tests/common/mod.rs | 2 +-
rust/core/tests/driver_manager_sqlite.rs | 50 ++++++++++++------------
rust/driver/datafusion/README.md | 2 +-
rust/driver/datafusion/src/lib.rs | 4 +-
rust/driver/datafusion/tests/test_datafusion.rs | 2 +-
rust/driver/dummy/src/lib.rs | 4 +-
rust/driver/dummy/tests/driver_exporter_dummy.rs | 6 +--
rust/driver/snowflake/src/connection/builder.rs | 2 +-
rust/driver/snowflake/src/database.rs | 8 ++--
rust/driver/snowflake/tests/driver.rs | 2 +-
12 files changed, 50 insertions(+), 61 deletions(-)
diff --git a/rust/core/src/driver_manager.rs b/rust/core/src/driver_manager.rs
index 506c26559..2463554d3 100644
--- a/rust/core/src/driver_manager.rs
+++ b/rust/core/src/driver_manager.rs
@@ -56,7 +56,7 @@
//! # fn main() -> Result<(), Box<dyn std::error::Error>> {
//! let opts = [(OptionDatabase::Uri, ":memory:".into())];
//! let mut driver =
ManagedDriver::load_dynamic_from_name("adbc_driver_sqlite", None,
AdbcVersion::V100)?;
-//! let mut database = driver.new_database_with_opts(opts)?;
+//! let database = driver.new_database_with_opts(opts)?;
//! let mut connection = database.new_connection()?;
//! let mut statement = connection.new_statement()?;
//!
@@ -251,9 +251,7 @@ impl ManagedDriver {
}
/// Returns a new database using the loaded driver.
- ///
- /// This uses `&mut self` to prevent a deadlock.
- fn database_new(&mut self) -> Result<ffi::FFI_AdbcDatabase> {
+ fn database_new(&self) -> Result<ffi::FFI_AdbcDatabase> {
let driver = self.inner_ffi_driver();
let mut database = ffi::FFI_AdbcDatabase::default();
@@ -267,12 +265,7 @@ impl ManagedDriver {
}
/// Initialize the given database using the loaded driver.
- ///
- /// This uses `&mut self` to prevent a deadlock.
- fn database_init(
- &mut self,
- mut database: ffi::FFI_AdbcDatabase,
- ) -> Result<ffi::FFI_AdbcDatabase> {
+ fn database_init(&self, mut database: ffi::FFI_AdbcDatabase) ->
Result<ffi::FFI_AdbcDatabase> {
let driver = self.inner_ffi_driver();
// DatabaseInit
@@ -473,9 +466,7 @@ impl ManagedDatabase {
}
/// Returns a new connection using the loaded driver.
- ///
- /// This uses `&mut self` to prevent a deadlock.
- fn connection_new(&mut self) -> Result<ffi::FFI_AdbcConnection> {
+ fn connection_new(&self) -> Result<ffi::FFI_AdbcConnection> {
let driver = self.ffi_driver();
let mut connection = ffi::FFI_AdbcConnection::default();
@@ -489,10 +480,8 @@ impl ManagedDatabase {
}
/// Initialize the given connection using the loaded driver.
- ///
- /// This uses `&mut self` to prevent a deadlock.
fn connection_init(
- &mut self,
+ &self,
mut connection: ffi::FFI_AdbcConnection,
) -> Result<ffi::FFI_AdbcConnection> {
let driver = self.ffi_driver();
@@ -577,7 +566,7 @@ impl Optionable for ManagedDatabase {
impl Database for ManagedDatabase {
type ConnectionType = ManagedConnection;
- fn new_connection(&mut self) -> Result<Self::ConnectionType> {
+ fn new_connection(&self) -> Result<Self::ConnectionType> {
// Construct a new connection.
let connection = self.connection_new()?;
// Initialize the connection.
@@ -592,7 +581,7 @@ impl Database for ManagedDatabase {
}
fn new_connection_with_opts(
- &mut self,
+ &self,
opts: impl IntoIterator<Item = (<Self::ConnectionType as
Optionable>::Option, OptionValue)>,
) -> Result<Self::ConnectionType> {
// Construct a new connection.
diff --git a/rust/core/src/lib.rs b/rust/core/src/lib.rs
index 8568c8b82..cf7acb352 100644
--- a/rust/core/src/lib.rs
+++ b/rust/core/src/lib.rs
@@ -121,11 +121,11 @@ pub trait Database: Optionable<Option = OptionDatabase> {
type ConnectionType: Connection;
/// Allocate and initialize a new connection without pre-init options.
- fn new_connection(&mut self) -> Result<Self::ConnectionType>;
+ fn new_connection(&self) -> Result<Self::ConnectionType>;
/// Allocate and initialize a new connection with pre-init options.
fn new_connection_with_opts(
- &mut self,
+ &self,
opts: impl IntoIterator<Item = (options::OptionConnection,
OptionValue)>,
) -> Result<Self::ConnectionType>;
}
diff --git a/rust/core/tests/common/mod.rs b/rust/core/tests/common/mod.rs
index b55a6ddd3..7e06e5bfe 100644
--- a/rust/core/tests/common/mod.rs
+++ b/rust/core/tests/common/mod.rs
@@ -95,7 +95,7 @@ pub fn test_driver(driver: &mut ManagedDriver, uri: &str) {
assert!(driver.new_database_with_opts(opts).is_err());
}
-pub fn test_database(database: &mut ManagedDatabase) {
+pub fn test_database(database: &ManagedDatabase) {
assert!(database.new_connection().is_ok());
let opts = [(OptionConnection::AutoCommit, "true".into())];
diff --git a/rust/core/tests/driver_manager_sqlite.rs
b/rust/core/tests/driver_manager_sqlite.rs
index e6053b8dd..20075f836 100644
--- a/rust/core/tests/driver_manager_sqlite.rs
+++ b/rust/core/tests/driver_manager_sqlite.rs
@@ -46,8 +46,8 @@ fn test_driver() {
#[test]
fn test_database() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
- common::test_database(&mut database);
+ let database = get_database(&mut driver);
+ common::test_database(&database);
}
#[test]
@@ -79,7 +79,7 @@ fn test_database_get_option() {
#[test]
fn test_connection() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
common::test_connection(&mut connection);
}
@@ -87,7 +87,7 @@ fn test_connection() {
#[test]
fn test_connection_get_option() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let connection = database.new_connection().unwrap();
let error = connection
@@ -114,7 +114,7 @@ fn test_connection_get_option() {
#[test]
fn test_connection_cancel() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let error = connection.cancel().unwrap_err();
@@ -124,7 +124,7 @@ fn test_connection_cancel() {
#[test]
fn test_connection_commit_rollback() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
common::test_connection_commit_rollback(&mut connection);
}
@@ -132,7 +132,7 @@ fn test_connection_commit_rollback() {
#[test]
fn test_connection_read_partition() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let connection = database.new_connection().unwrap();
common::test_connection_read_partition(&connection);
}
@@ -140,7 +140,7 @@ fn test_connection_read_partition() {
#[test]
fn test_connection_get_table_types() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let connection = database.new_connection().unwrap();
common::test_connection_get_table_types(&connection, &["table", "view"]);
}
@@ -148,7 +148,7 @@ fn test_connection_get_table_types() {
#[test]
fn test_connection_get_info() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let connection = database.new_connection().unwrap();
common::test_connection_get_info(&connection, 5);
}
@@ -156,7 +156,7 @@ fn test_connection_get_info() {
#[test]
fn test_connection_get_objects() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let connection = database.new_connection().unwrap();
common::test_connection_get_objects(&connection, 1, 1);
}
@@ -164,7 +164,7 @@ fn test_connection_get_objects() {
#[test]
fn test_connection_get_table_schema() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
common::test_connection_get_table_schema(&mut connection);
}
@@ -172,7 +172,7 @@ fn test_connection_get_table_schema() {
#[test]
fn test_connection_get_statistic_names() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let connection = database.new_connection().unwrap();
assert!(connection.get_statistic_names().is_err());
}
@@ -180,7 +180,7 @@ fn test_connection_get_statistic_names() {
#[test]
fn test_connection_get_statistics() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let connection = database.new_connection().unwrap();
assert!(connection.get_statistics(None, None, None, false).is_err());
}
@@ -188,7 +188,7 @@ fn test_connection_get_statistics() {
#[test]
fn test_statement() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
common::test_statement(&mut statement);
@@ -197,7 +197,7 @@ fn test_statement() {
#[test]
fn test_statement_prepare() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
common::test_statement_prepare(&mut statement);
@@ -206,7 +206,7 @@ fn test_statement_prepare() {
#[test]
fn test_statement_set_substrait_plan() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
common::test_statement_set_substrait_plan(&mut statement);
@@ -215,7 +215,7 @@ fn test_statement_set_substrait_plan() {
#[test]
fn test_statement_get_parameter_schema() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
@@ -233,7 +233,7 @@ fn test_statement_get_parameter_schema() {
#[test]
fn test_statement_execute() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
common::test_statement_execute(&mut statement);
@@ -242,7 +242,7 @@ fn test_statement_execute() {
#[test]
fn test_statement_execute_update() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
common::test_statement_execute_update(&mut connection);
}
@@ -250,7 +250,7 @@ fn test_statement_execute_update() {
#[test]
fn test_statement_execute_schema() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
@@ -261,7 +261,7 @@ fn test_statement_execute_schema() {
#[test]
fn test_statement_execute_partitions() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
common::test_statement_execute_partitions(&mut statement);
@@ -270,7 +270,7 @@ fn test_statement_execute_partitions() {
#[test]
fn test_statement_cancel() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
@@ -281,7 +281,7 @@ fn test_statement_cancel() {
#[test]
fn test_statement_bind() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
common::test_statement_bind(&mut statement);
@@ -290,7 +290,7 @@ fn test_statement_bind() {
#[test]
fn test_statement_bind_stream() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
let mut statement = connection.new_statement().unwrap();
common::test_statement_bind_stream(&mut statement);
@@ -299,7 +299,7 @@ fn test_statement_bind_stream() {
#[test]
fn test_ingestion_roundtrip() {
let mut driver = get_driver();
- let mut database = get_database(&mut driver);
+ let database = get_database(&mut driver);
let mut connection = database.new_connection().unwrap();
common::test_ingestion_roundtrip(&mut connection);
}
diff --git a/rust/driver/datafusion/README.md b/rust/driver/datafusion/README.md
index 4aa18f202..2bbb5093b 100644
--- a/rust/driver/datafusion/README.md
+++ b/rust/driver/datafusion/README.md
@@ -36,7 +36,7 @@ fn main() {
)
.unwrap();
- let mut database = driver.new_database().unwrap();
+ let database = driver.new_database().unwrap();
let mut connection = database.new_connection().unwrap();
diff --git a/rust/driver/datafusion/src/lib.rs
b/rust/driver/datafusion/src/lib.rs
index 1e4dd7ac4..a462f4105 100644
--- a/rust/driver/datafusion/src/lib.rs
+++ b/rust/driver/datafusion/src/lib.rs
@@ -183,7 +183,7 @@ impl Optionable for DataFusionDatabase {
impl Database for DataFusionDatabase {
type ConnectionType = DataFusionConnection;
- fn new_connection(&mut self) -> Result<Self::ConnectionType> {
+ fn new_connection(&self) -> Result<Self::ConnectionType> {
let ctx = SessionContext::new();
let runtime = tokio::runtime::Builder::new_multi_thread()
@@ -198,7 +198,7 @@ impl Database for DataFusionDatabase {
}
fn new_connection_with_opts(
- &mut self,
+ &self,
opts: impl IntoIterator<
Item = (
adbc_core::options::OptionConnection,
diff --git a/rust/driver/datafusion/tests/test_datafusion.rs
b/rust/driver/datafusion/tests/test_datafusion.rs
index 9337861d5..2e9a15e7e 100644
--- a/rust/driver/datafusion/tests/test_datafusion.rs
+++ b/rust/driver/datafusion/tests/test_datafusion.rs
@@ -34,7 +34,7 @@ fn get_connection() -> ManagedConnection {
)
.unwrap();
- let mut database = driver.new_database().unwrap();
+ let database = driver.new_database().unwrap();
database.new_connection().unwrap()
}
diff --git a/rust/driver/dummy/src/lib.rs b/rust/driver/dummy/src/lib.rs
index a841fe0e3..9c4826474 100644
--- a/rust/driver/dummy/src/lib.rs
+++ b/rust/driver/dummy/src/lib.rs
@@ -230,12 +230,12 @@ impl Optionable for DummyDatabase {
impl Database for DummyDatabase {
type ConnectionType = DummyConnection;
- fn new_connection(&mut self) -> Result<Self::ConnectionType> {
+ fn new_connection(&self) -> Result<Self::ConnectionType> {
Ok(Self::ConnectionType::default())
}
fn new_connection_with_opts(
- &mut self,
+ &self,
opts: impl IntoIterator<Item = (<Self::ConnectionType as
Optionable>::Option, OptionValue)>,
) -> Result<Self::ConnectionType> {
let mut connection = Self::ConnectionType::default();
diff --git a/rust/driver/dummy/tests/driver_exporter_dummy.rs
b/rust/driver/dummy/tests/driver_exporter_dummy.rs
index a9601cbde..3f7eddb0a 100644
--- a/rust/driver/dummy/tests/driver_exporter_dummy.rs
+++ b/rust/driver/dummy/tests/driver_exporter_dummy.rs
@@ -73,7 +73,7 @@ fn get_exported() -> (
AdbcVersion::V110,
)
.unwrap();
- let mut database = driver.new_database().unwrap();
+ let database = driver.new_database().unwrap();
let mut connection = database.new_connection().unwrap();
let statement = connection.new_statement().unwrap();
(driver, database, connection, statement)
@@ -81,7 +81,7 @@ fn get_exported() -> (
fn get_native() -> (DummyDriver, DummyDatabase, DummyConnection,
DummyStatement) {
let mut driver = DummyDriver {};
- let mut database = driver.new_database().unwrap();
+ let database = driver.new_database().unwrap();
let mut connection = database.new_connection().unwrap();
let statement = connection.new_statement().unwrap();
(driver, database, connection, statement)
@@ -197,7 +197,7 @@ fn test_database_options() {
#[test]
fn test_connection_options() {
- let (_, mut database, _, _) = get_exported();
+ let (_, database, _, _) = get_exported();
// Pre-init options
let options = [
diff --git a/rust/driver/snowflake/src/connection/builder.rs
b/rust/driver/snowflake/src/connection/builder.rs
index d1b8422db..fbc2b8cfb 100644
--- a/rust/driver/snowflake/src/connection/builder.rs
+++ b/rust/driver/snowflake/src/connection/builder.rs
@@ -93,7 +93,7 @@ impl Builder {
impl Builder {
/// Attempt to initialize a [`Connection`] using the values provided to
/// this builder using the provided [`Database`].
- pub fn build(self, database: &mut Database) -> Result<Connection> {
+ pub fn build(self, database: &Database) -> Result<Connection> {
database.new_connection_with_opts(self)
}
}
diff --git a/rust/driver/snowflake/src/database.rs
b/rust/driver/snowflake/src/database.rs
index 006c20802..492933962 100644
--- a/rust/driver/snowflake/src/database.rs
+++ b/rust/driver/snowflake/src/database.rs
@@ -43,7 +43,7 @@ pub use builder::*;
pub struct Database(pub(crate) ManagedDatabase);
impl Database {
- fn get_info(&mut self, info_code: InfoCode) -> Result<Arc<dyn Array>> {
+ fn get_info(&self, info_code: InfoCode) -> Result<Arc<dyn Array>> {
self.new_connection()?
.get_info(Some(HashSet::from_iter([info_code])))?
.next()
@@ -115,7 +115,7 @@ impl Database {
}
/// Returns the [`AdbcVersion`] reported by the driver.
- pub fn adbc_version(&mut self) -> Result<AdbcVersion> {
+ pub fn adbc_version(&self) -> Result<AdbcVersion> {
self.new_connection()?
.get_info(Some(HashSet::from_iter([InfoCode::DriverAdbcVersion])))?
.next()
@@ -168,12 +168,12 @@ impl Optionable for Database {
impl adbc_core::Database for Database {
type ConnectionType = Connection;
- fn new_connection(&mut self) -> Result<Self::ConnectionType> {
+ fn new_connection(&self) -> Result<Self::ConnectionType> {
self.0.new_connection().map(Connection)
}
fn new_connection_with_opts(
- &mut self,
+ &self,
opts: impl IntoIterator<Item = (OptionConnection, OptionValue)>,
) -> Result<Self::ConnectionType> {
self.0.new_connection_with_opts(opts).map(Connection)
diff --git a/rust/driver/snowflake/tests/driver.rs
b/rust/driver/snowflake/tests/driver.rs
index 300bc3225..a5eaec756 100644
--- a/rust/driver/snowflake/tests/driver.rs
+++ b/rust/driver/snowflake/tests/driver.rs
@@ -56,7 +56,7 @@ mod tests {
LazyLock::new(|| database::Builder::from_env()?.build(&mut
DRIVER.deref().clone()?));
static CONNECTION: LazyLock<Result<Connection>> =
- LazyLock::new(|| connection::Builder::from_env()?.build(&mut
DATABASE.deref().clone()?));
+ LazyLock::new(||
connection::Builder::from_env()?.build(&DATABASE.deref().clone()?));
fn with_database(func: impl FnOnce(Database) -> Result<()>) -> Result<()> {
DATABASE.deref().clone().and_then(func)
