diff --git a/javascript/src/client.rs b/javascript/src/client.rs index 6324131688..dee9af31a1 100644 --- a/javascript/src/client.rs +++ b/javascript/src/client.rs @@ -230,7 +230,7 @@ impl AdbcConnectionCore { .map_err(|e| ClientError::Other(e.to_string()))?; let codes: Option> = info_codes.map(|v| { v.into_iter() - .filter_map(|code| InfoCode::try_from(code).ok()) + .map(InfoCode::from) .collect::>() }); let reader = conn.get_info(codes)?; diff --git a/rust/core/src/options.rs b/rust/core/src/options.rs index 99fdb59078..dd0e015757 100644 --- a/rust/core/src/options.rs +++ b/rust/core/src/options.rs @@ -223,6 +223,19 @@ pub enum InfoCode { /// /// ADBC API revision 1.1.0 DriverAdbcVersion, + /// Any other info code than the ones listed above. The value is the raw code. + /// + /// Codes `[0, 10_000)` are reserved for ADBC usage (of which `[500, 1_000)` + /// is reserved for "XDBC" information since ADBC API revision 1.1.0), while + /// codes `10_000` and higher are driver/vendor-specific. Drivers ignore + /// requests for codes they don't recognize (the row is omitted from the + /// result). + /// + /// Note: to keep equality and hashing consistent, this variant should never + /// be constructed with the value of a code that has a dedicated variant; + /// use [InfoCode::from] to convert from a raw `u32`, which only produces + /// `Other` for values without one. + Other(u32), } impl From<&InfoCode> for u32 { @@ -243,34 +256,30 @@ impl From<&InfoCode> for u32 { InfoCode::DriverVersion => constants::ADBC_INFO_DRIVER_VERSION, InfoCode::DriverArrowVersion => constants::ADBC_INFO_DRIVER_ARROW_VERSION, InfoCode::DriverAdbcVersion => constants::ADBC_INFO_DRIVER_ADBC_VERSION, + InfoCode::Other(v) => *v, } } } -impl TryFrom for InfoCode { - type Error = Error; - - fn try_from(value: u32) -> Result { +impl From for InfoCode { + fn from(value: u32) -> Self { match value { - constants::ADBC_INFO_VENDOR_NAME => Ok(InfoCode::VendorName), - constants::ADBC_INFO_VENDOR_VERSION => Ok(InfoCode::VendorVersion), - constants::ADBC_INFO_VENDOR_ARROW_VERSION => Ok(InfoCode::VendorArrowVersion), - constants::ADBC_INFO_VENDOR_SQL => Ok(InfoCode::VendorSql), - constants::ADBC_INFO_VENDOR_SUBSTRAIT => Ok(InfoCode::VendorSubstrait), + constants::ADBC_INFO_VENDOR_NAME => InfoCode::VendorName, + constants::ADBC_INFO_VENDOR_VERSION => InfoCode::VendorVersion, + constants::ADBC_INFO_VENDOR_ARROW_VERSION => InfoCode::VendorArrowVersion, + constants::ADBC_INFO_VENDOR_SQL => InfoCode::VendorSql, + constants::ADBC_INFO_VENDOR_SUBSTRAIT => InfoCode::VendorSubstrait, constants::ADBC_INFO_VENDOR_SUBSTRAIT_MIN_VERSION => { - Ok(InfoCode::VendorSubstraitMinVersion) + InfoCode::VendorSubstraitMinVersion } constants::ADBC_INFO_VENDOR_SUBSTRAIT_MAX_VERSION => { - Ok(InfoCode::VendorSubstraitMaxVersion) + InfoCode::VendorSubstraitMaxVersion } - constants::ADBC_INFO_DRIVER_NAME => Ok(InfoCode::DriverName), - constants::ADBC_INFO_DRIVER_VERSION => Ok(InfoCode::DriverVersion), - constants::ADBC_INFO_DRIVER_ARROW_VERSION => Ok(InfoCode::DriverArrowVersion), - constants::ADBC_INFO_DRIVER_ADBC_VERSION => Ok(InfoCode::DriverAdbcVersion), - v => Err(Error::with_message_and_status( - format!("Unknown info code: {v}"), - Status::InvalidData, - )), + constants::ADBC_INFO_DRIVER_NAME => InfoCode::DriverName, + constants::ADBC_INFO_DRIVER_VERSION => InfoCode::DriverVersion, + constants::ADBC_INFO_DRIVER_ARROW_VERSION => InfoCode::DriverArrowVersion, + constants::ADBC_INFO_DRIVER_ADBC_VERSION => InfoCode::DriverAdbcVersion, + v => InfoCode::Other(v), } } } @@ -741,3 +750,20 @@ impl std::fmt::Display for Statistics { write!(f, "{}", self.as_ref()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn info_code_u32_roundtrip() { + for code in (0..20_000_u32).chain([u32::MAX]) { + assert_eq!(u32::from(&InfoCode::from(code)), code); + } + assert_eq!( + InfoCode::from(constants::ADBC_INFO_VENDOR_NAME), + InfoCode::VendorName + ); + assert_eq!(InfoCode::from(10_042), InfoCode::Other(10_042)); + } +} diff --git a/rust/driver/dummy/src/lib.rs b/rust/driver/dummy/src/lib.rs index 7879873ca9..8c3b92e762 100644 --- a/rust/driver/dummy/src/lib.rs +++ b/rust/driver/dummy/src/lib.rs @@ -37,6 +37,8 @@ use adbc_core::{ schemas, }; +pub const VENDOR_INFO_CODE: u32 = 10_042; + #[derive(Debug)] pub struct SingleBatchReader { batch: Option, @@ -312,9 +314,9 @@ impl Connection for DummyConnection { fn get_info( &self, - _codes: Option>, + codes: Option>, ) -> Result> { - let string_value_array = StringArray::from(vec!["MyVendorName"]); + let string_value_array = StringArray::from(vec!["MyVendorName", "MyVendorInfoValue"]); let bool_value_array = BooleanArray::from(vec![true]); let int64_value_array = Int64Array::from(vec![42]); let int32_bitmask_array = Int32Array::from(vec![1337]); @@ -355,20 +357,34 @@ impl Connection for DummyConnection { false, )?; - let name_array = UInt32Array::from(vec![ - Into::::into(&InfoCode::VendorName), - Into::::into(&InfoCode::VendorVersion), - Into::::into(&InfoCode::VendorArrowVersion), - Into::::into(&InfoCode::DriverName), - Into::::into(&InfoCode::DriverVersion), - Into::::into(&InfoCode::DriverArrowVersion), - ]); - - let type_id_buffer = [0_i8, 1, 2, 3, 4, 5] - .into_iter() + // Every info value this driver knows, as (code, union type id, offset into that type's + // child array). Includes a vendor-specific code to exercise `InfoCode::Other`. + let known_info: [(InfoCode, i8, i32); 7] = [ + (InfoCode::VendorName, 0, 0), + (InfoCode::VendorVersion, 1, 0), + (InfoCode::VendorArrowVersion, 2, 0), + (InfoCode::DriverName, 3, 0), + (InfoCode::DriverVersion, 4, 0), + (InfoCode::DriverArrowVersion, 5, 0), + (InfoCode::Other(VENDOR_INFO_CODE), 0, 1), + ]; + let rows: Vec<&(InfoCode, i8, i32)> = known_info + .iter() + .filter(|(code, _, _)| codes.as_ref().is_none_or(|codes| codes.contains(code))) + .collect(); + + let name_array = UInt32Array::from( + rows.iter() + .map(|(code, _, _)| code.into()) + .collect::>(), + ); + let type_id_buffer = rows + .iter() + .map(|(_, type_id, _)| *type_id) .collect::>(); - let value_offsets_buffer = [0_i32, 0, 0, 0, 0, 0] - .into_iter() + let value_offsets_buffer = rows + .iter() + .map(|(_, _, offset)| *offset) .collect::>(); let value_array = UnionArray::try_new( diff --git a/rust/driver/dummy/tests/driver_exporter_dummy.rs b/rust/driver/dummy/tests/driver_exporter_dummy.rs index b9bc989416..36e7657f92 100644 --- a/rust/driver/dummy/tests/driver_exporter_dummy.rs +++ b/rust/driver/dummy/tests/driver_exporter_dummy.rs @@ -19,11 +19,14 @@ /// directly using the Rust API (native) and through the exported driver via the /// driver manager (exported). That allows us to test that data correctly round-trip /// between C and Rust. +use std::collections::HashSet; use std::ffi::CString; use std::ops::Deref; use std::sync::Arc; -use arrow_array::ffi_stream::FFI_ArrowArrayStream; +use arrow_array::cast::AsArray; +use arrow_array::ffi_stream::{ArrowArrayStreamReader, FFI_ArrowArrayStream}; +use arrow_array::types::UInt32Type; use arrow_array::{Array, Float64Array, Int64Array, RecordBatch, RecordBatchReader, StringArray}; use arrow_schema::{DataType, Field, Schema}; use arrow_select::concat::concat_batches; @@ -365,7 +368,99 @@ fn test_connection_get_info() { .unwrap(), ); assert_eq!(exported_info.schema(), *schemas::GET_INFO_SCHEMA.deref()); + // The dummy driver recognizes `DriverName` but not `DriverAdbcVersion`, whose row is omitted. + assert_eq!(exported_info.num_rows(), 1); assert_eq!(exported_info, native_info); + + let vendor_codes: HashSet = [InfoCode::Other(adbc_dummy::VENDOR_INFO_CODE)].into(); + let exported_info = concat_reader( + exported_connection + .get_info(Some(vendor_codes.clone())) + .unwrap(), + ); + let native_info = concat_reader(native_connection.get_info(Some(vendor_codes)).unwrap()); + assert_eq!(exported_info.num_rows(), 1); + assert_eq!( + exported_info + .column(0) + .as_primitive::() + .value(0), + adbc_dummy::VENDOR_INFO_CODE + ); + assert_eq!(exported_info, native_info); +} + +// Driven at the C ABI (unlike the other tests) to exercise the exporter's raw `u32` code +// handling: codes without a dedicated `InfoCode` variant must reach the driver as +// `InfoCode::Other` (so it can answer vendor-specific codes), and per the ADBC spec the driver +// ignores requests for codes it doesn't recognize (the row is omitted) rather than failing the +// call. +#[test] +fn test_connection_get_info_ignores_unrecognized_codes() { + let driver = DummyDriver::ffi_driver(); + let mut error = FFI_AdbcError::default(); + let err = &mut error as *mut FFI_AdbcError; + + unsafe { + let mut database = FFI_AdbcDatabase::default(); + assert_eq!( + driver.DatabaseNew.unwrap()(&mut database, err), + ADBC_STATUS_OK + ); + assert_eq!( + driver.DatabaseInit.unwrap()(&mut database, err), + ADBC_STATUS_OK + ); + + let mut connection = FFI_AdbcConnection::default(); + assert_eq!( + driver.ConnectionNew.unwrap()(&mut connection, err), + ADBC_STATUS_OK + ); + assert_eq!( + driver.ConnectionInit.unwrap()(&mut connection, &mut database, err), + ADBC_STATUS_OK + ); + + let info_codes: [u32; 3] = [ + Into::::into(&InfoCode::DriverName), + adbc_dummy::VENDOR_INFO_CODE, + u32::MAX, + ]; + let mut stream = FFI_ArrowArrayStream::empty(); + assert_eq!( + driver.ConnectionGetInfo.unwrap()( + &mut connection, + info_codes.as_ptr(), + info_codes.len(), + &mut stream, + err, + ), + ADBC_STATUS_OK, + "an unrecognized info code must be ignored, not fail the call" + ); + + let info = concat_reader(ArrowArrayStreamReader::try_new(stream).unwrap()); + let returned_codes: HashSet = info + .column(0) + .as_primitive::() + .values() + .iter() + .copied() + .collect(); + assert_eq!( + returned_codes, + [ + Into::::into(&InfoCode::DriverName), + adbc_dummy::VENDOR_INFO_CODE + ] + .into(), + "the vendor-specific code must be answered and the unrecognized one omitted" + ); + + driver.ConnectionRelease.unwrap()(&mut connection, err); + driver.DatabaseRelease.unwrap()(&mut database, err); + } } #[test] diff --git a/rust/ffi/src/driver_exporter.rs b/rust/ffi/src/driver_exporter.rs index ca483c5a42..429f0caf31 100644 --- a/rust/ffi/src/driver_exporter.rs +++ b/rust/ffi/src/driver_exporter.rs @@ -1163,9 +1163,8 @@ extern "C" fn connection_get_info( None } else { let info_codes = unsafe { std::slice::from_raw_parts(info_codes, info_codes_length) }; - let info_codes: Result> = - info_codes.iter().map(|c| InfoCode::try_from(*c)).collect(); - let info_codes = check_err!(info_codes, error); + let info_codes: HashSet = + info_codes.iter().map(|c| InfoCode::from(*c)).collect(); Some(info_codes) };