felipecrv commented on code in PR #3381:
URL: https://github.com/apache/arrow-adbc/pull/3381#discussion_r2312597541
##########
rust/driver_manager/src/lib.rs:
##########
@@ -119,13 +119,14 @@ use arrow_array::{Array, RecordBatch, RecordBatchReader,
StructArray};
use toml::de::DeTable;
use adbc_core::{
- error::{Error, Result, Status},
+ constants,
+ error::{AdbcStatusCode, Error, Result, Status},
options::{self, AdbcVersion, InfoCode, OptionValue},
- LoadFlags, PartitionedResult, LOAD_FLAG_ALLOW_RELATIVE_PATHS,
LOAD_FLAG_SEARCH_ENV,
- LOAD_FLAG_SEARCH_SYSTEM, LOAD_FLAG_SEARCH_USER,
+ Connection, Database, Driver, LoadFlags, Optionable, PartitionedResult,
Statement,
+ LOAD_FLAG_ALLOW_RELATIVE_PATHS, LOAD_FLAG_SEARCH_ENV,
LOAD_FLAG_SEARCH_SYSTEM,
+ LOAD_FLAG_SEARCH_USER,
};
-use adbc_core::{ffi, ffi::driver_method, Optionable};
-use adbc_core::{Connection, Database, Driver, Statement};
+use adbc_ffi::driver_method;
Review Comment:
Is it possible to `use adbc_ffi as ffi` here so you don't need to rename
every ffi type mention in this file?
##########
rust/core/src/error.rs:
##########
@@ -153,3 +157,59 @@ impl From<std::ffi::IntoStringError> for Error {
error.into()
}
}
+
+impl TryFrom<AdbcStatusCode> for Status {
+ type Error = Error;
+
+ fn try_from(value: AdbcStatusCode) -> Result<Self> {
+ match value {
+ constants::ADBC_STATUS_OK => Ok(Status::Ok),
+ constants::ADBC_STATUS_UNKNOWN => Ok(Status::Unknown),
+ constants::ADBC_STATUS_NOT_IMPLEMENTED =>
Ok(Status::NotImplemented),
+ constants::ADBC_STATUS_NOT_FOUND => Ok(Status::NotFound),
+ constants::ADBC_STATUS_ALREADY_EXISTS => Ok(Status::AlreadyExists),
+ constants::ADBC_STATUS_INVALID_ARGUMENT =>
Ok(Status::InvalidArguments),
+ constants::ADBC_STATUS_INVALID_STATE => Ok(Status::InvalidState),
+ constants::ADBC_STATUS_INVALID_DATA => Ok(Status::InvalidData),
+ constants::ADBC_STATUS_INTEGRITY => Ok(Status::Integrity),
+ constants::ADBC_STATUS_INTERNAL => Ok(Status::Internal),
+ constants::ADBC_STATUS_IO => Ok(Status::IO),
+ constants::ADBC_STATUS_CANCELLED => Ok(Status::Cancelled),
+ constants::ADBC_STATUS_TIMEOUT => Ok(Status::Timeout),
+ constants::ADBC_STATUS_UNAUTHENTICATED =>
Ok(Status::Unauthenticated),
+ constants::ADBC_STATUS_UNAUTHORIZED => Ok(Status::Unauthorized),
+ v => Err(Error::with_message_and_status(
+ format!("Unknown status code: {v}"),
+ Status::InvalidData,
+ )),
+ }
+ }
+}
+
+impl From<Status> for AdbcStatusCode {
+ fn from(value: Status) -> Self {
+ match value {
+ Status::Ok => constants::ADBC_STATUS_OK,
+ Status::Unknown => constants::ADBC_STATUS_UNKNOWN,
+ Status::NotImplemented => constants::ADBC_STATUS_NOT_IMPLEMENTED,
+ Status::NotFound => constants::ADBC_STATUS_NOT_FOUND,
+ Status::AlreadyExists => constants::ADBC_STATUS_ALREADY_EXISTS,
+ Status::InvalidArguments =>
constants::ADBC_STATUS_INVALID_ARGUMENT,
+ Status::InvalidState => constants::ADBC_STATUS_INVALID_STATE,
+ Status::InvalidData => constants::ADBC_STATUS_INVALID_DATA,
+ Status::Integrity => constants::ADBC_STATUS_INTEGRITY,
+ Status::Internal => constants::ADBC_STATUS_INTERNAL,
+ Status::IO => constants::ADBC_STATUS_IO,
+ Status::Cancelled => constants::ADBC_STATUS_CANCELLED,
+ Status::Timeout => constants::ADBC_STATUS_TIMEOUT,
+ Status::Unauthenticated => constants::ADBC_STATUS_UNAUTHENTICATED,
+ Status::Unauthorized => constants::ADBC_STATUS_UNAUTHORIZED,
+ }
+ }
+}
+
+impl From<&Status> for AdbcStatusCode {
+ fn from(value: &Status) -> Self {
+ (*value).into()
+ }
+}
Review Comment:
Indeed.
##########
rust/core/src/error.rs:
##########
@@ -153,3 +157,59 @@ impl From<std::ffi::IntoStringError> for Error {
error.into()
}
}
+
+impl TryFrom<AdbcStatusCode> for Status {
+ type Error = Error;
+
+ fn try_from(value: AdbcStatusCode) -> Result<Self> {
+ match value {
+ constants::ADBC_STATUS_OK => Ok(Status::Ok),
+ constants::ADBC_STATUS_UNKNOWN => Ok(Status::Unknown),
+ constants::ADBC_STATUS_NOT_IMPLEMENTED =>
Ok(Status::NotImplemented),
+ constants::ADBC_STATUS_NOT_FOUND => Ok(Status::NotFound),
+ constants::ADBC_STATUS_ALREADY_EXISTS => Ok(Status::AlreadyExists),
+ constants::ADBC_STATUS_INVALID_ARGUMENT =>
Ok(Status::InvalidArguments),
+ constants::ADBC_STATUS_INVALID_STATE => Ok(Status::InvalidState),
+ constants::ADBC_STATUS_INVALID_DATA => Ok(Status::InvalidData),
+ constants::ADBC_STATUS_INTEGRITY => Ok(Status::Integrity),
+ constants::ADBC_STATUS_INTERNAL => Ok(Status::Internal),
+ constants::ADBC_STATUS_IO => Ok(Status::IO),
+ constants::ADBC_STATUS_CANCELLED => Ok(Status::Cancelled),
+ constants::ADBC_STATUS_TIMEOUT => Ok(Status::Timeout),
+ constants::ADBC_STATUS_UNAUTHENTICATED =>
Ok(Status::Unauthenticated),
+ constants::ADBC_STATUS_UNAUTHORIZED => Ok(Status::Unauthorized),
+ v => Err(Error::with_message_and_status(
+ format!("Unknown status code: {v}"),
+ Status::InvalidData,
+ )),
Review Comment:
Another option: `debug_assert!(false, ...)` and return `Status::Unknown`.
Let's us catch incomplete status enum coverage, but doesn't have to panic in
prod.
##########
rust/core/src/error.rs:
##########
@@ -153,3 +157,59 @@ impl From<std::ffi::IntoStringError> for Error {
error.into()
}
}
+
+impl TryFrom<AdbcStatusCode> for Status {
+ type Error = Error;
+
+ fn try_from(value: AdbcStatusCode) -> Result<Self> {
+ match value {
+ constants::ADBC_STATUS_OK => Ok(Status::Ok),
+ constants::ADBC_STATUS_UNKNOWN => Ok(Status::Unknown),
+ constants::ADBC_STATUS_NOT_IMPLEMENTED =>
Ok(Status::NotImplemented),
+ constants::ADBC_STATUS_NOT_FOUND => Ok(Status::NotFound),
+ constants::ADBC_STATUS_ALREADY_EXISTS => Ok(Status::AlreadyExists),
+ constants::ADBC_STATUS_INVALID_ARGUMENT =>
Ok(Status::InvalidArguments),
+ constants::ADBC_STATUS_INVALID_STATE => Ok(Status::InvalidState),
+ constants::ADBC_STATUS_INVALID_DATA => Ok(Status::InvalidData),
+ constants::ADBC_STATUS_INTEGRITY => Ok(Status::Integrity),
+ constants::ADBC_STATUS_INTERNAL => Ok(Status::Internal),
+ constants::ADBC_STATUS_IO => Ok(Status::IO),
+ constants::ADBC_STATUS_CANCELLED => Ok(Status::Cancelled),
+ constants::ADBC_STATUS_TIMEOUT => Ok(Status::Timeout),
+ constants::ADBC_STATUS_UNAUTHENTICATED =>
Ok(Status::Unauthenticated),
+ constants::ADBC_STATUS_UNAUTHORIZED => Ok(Status::Unauthorized),
+ v => Err(Error::with_message_and_status(
+ format!("Unknown status code: {v}"),
+ Status::InvalidData,
+ )),
Review Comment:
Nevermind... Just noticed this is moved code.
##########
rust/ffi/src/lib.rs:
##########
@@ -0,0 +1,49 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! ADBC: Arrow Database Connectivity
+//!
+//! ADBC is a set of APIs and libraries for
[Arrow](https://arrow.apache.org/)-native
+//! access to databases. Execute SQL and [Substrait](https://substrait.io/)
+//! queries, query database catalogs, and more, all using Arrow data to
+//! eliminate unnecessary data copies, speed up access, and make it more
+//! convenient to build analytical applications.
+//!
+//! Read more about ADBC at <https://arrow.apache.org/adbc/>
+//!
+//! This library currently provides:
+//! - Structs for C-compatible items as defined in
[`adbc.h`](https://github.com/apache/arrow-adbc/blob/main/c/include/arrow-adbc/adbc.h).
+//! - A driver exporter that takes an implementation of the abstract API and
+//! turns it into an object file that implements the C API.
+//!
+//! # Driver Exporter
+//!
+//! The driver exporter allows exposing native Rust drivers as C drivers to be
+//! used by other languages via their own driver manager. Once you have an
+//! implementation of [adbc_core::Driver], provided that it also implements
[Default], you
+//! can build it as an object file implementing the C API with the
+//! [export_driver] macro.
+
+pub mod driver_exporter;
+#[doc(hidden)]
+pub use driver_exporter::FFIDriver;
+pub mod methods;
+pub(crate) mod types;
+pub use types::{
+ FFI_AdbcConnection, FFI_AdbcDatabase, FFI_AdbcDriver,
FFI_AdbcDriverInitFunc, FFI_AdbcError,
+ FFI_AdbcErrorDetail, FFI_AdbcPartitions, FFI_AdbcStatement,
+};
Review Comment:
Please, let's not. It's good to have a string distinction between these
types.
The only sane way of using these types would be `use adbc_ffi:T as FFI_T` so
let's avoid the need for aliasing of imports by keeping them named like this
from the start.
##########
rust/core/src/error.rs:
##########
@@ -153,3 +157,59 @@ impl From<std::ffi::IntoStringError> for Error {
error.into()
}
}
+
+impl TryFrom<AdbcStatusCode> for Status {
+ type Error = Error;
+
+ fn try_from(value: AdbcStatusCode) -> Result<Self> {
+ match value {
+ constants::ADBC_STATUS_OK => Ok(Status::Ok),
+ constants::ADBC_STATUS_UNKNOWN => Ok(Status::Unknown),
+ constants::ADBC_STATUS_NOT_IMPLEMENTED =>
Ok(Status::NotImplemented),
+ constants::ADBC_STATUS_NOT_FOUND => Ok(Status::NotFound),
+ constants::ADBC_STATUS_ALREADY_EXISTS => Ok(Status::AlreadyExists),
+ constants::ADBC_STATUS_INVALID_ARGUMENT =>
Ok(Status::InvalidArguments),
+ constants::ADBC_STATUS_INVALID_STATE => Ok(Status::InvalidState),
+ constants::ADBC_STATUS_INVALID_DATA => Ok(Status::InvalidData),
+ constants::ADBC_STATUS_INTEGRITY => Ok(Status::Integrity),
+ constants::ADBC_STATUS_INTERNAL => Ok(Status::Internal),
+ constants::ADBC_STATUS_IO => Ok(Status::IO),
+ constants::ADBC_STATUS_CANCELLED => Ok(Status::Cancelled),
+ constants::ADBC_STATUS_TIMEOUT => Ok(Status::Timeout),
+ constants::ADBC_STATUS_UNAUTHENTICATED =>
Ok(Status::Unauthenticated),
+ constants::ADBC_STATUS_UNAUTHORIZED => Ok(Status::Unauthorized),
+ v => Err(Error::with_message_and_status(
+ format!("Unknown status code: {v}"),
+ Status::InvalidData,
+ )),
Review Comment:
Could we make this infallible by using `unreachable!()` here? `enum`
conversion is UB in C++ which is dangerous, but going from that to `Err` in
Rust is overkill IMO. An `unreachable!` would mean that drivers returning bad
status (broken drivers) simple trigger a panic. I don't like forcing operations
that use the FFI to be fallible on the most basic operation already (i.e.
converting a status code).
##########
rust/core/src/options.rs:
##########
@@ -37,7 +37,7 @@ pub enum OptionValue {
impl OptionValue {
/// Gets the data type of the option's value.
- pub(crate) fn get_type(&self) -> &str {
+ pub fn get_type(&self) -> &str {
Review Comment:
`type_name()`?
##########
rust/core/src/lib.rs:
##########
@@ -27,10 +27,6 @@
//!
//! This library currently provides:
//! - An abstract Rust API to be implemented by vendor-specific drivers.
-//! - A driver manager which implements this same API, but dynamically loads
-//! drivers internally and forwards calls appropriately using the [C
API](https://github.com/apache/arrow-adbc/blob/main/c/include/arrow-adbc/adbc.h).
-//! - A driver exporter that takes an implementation of the abstract API and
-//! turns it into an object file that implements the C API.
Review Comment:
Would it be more informative to rewrite this to:
> The `core` library currently provides the basic types shared by
vendor-specific drivers, the driver manager, and the driver exporter.
?
--
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
