Add in FFI integration test for config extensions

Author: timsaucer

datafusion/ffi/src/config/mod.rs

@@ -19,7 +19,7 @@ pub mod extension_options;
 
 use abi_stable::StableAbi;
 use abi_stable::std_types::{RHashMap, RString};
-use datafusion_common::config::{ConfigOptions, ExtensionOptions};
+use datafusion_common::config::{ConfigOptions, ExtensionOptions, TableOptions};
 use datafusion_common::{DataFusionError, Result};
 
 use crate::config::extension_options::FFI_ExtensionOptions;
@@ -73,3 +73,53 @@ impl TryFrom<FFI_ConfigOptions> for ConfigOptions {
         Ok(options)
     }
 }
+
+// TODO(tsaucer) add text about how extension options will require user to convert to concrete type.
+#[repr(C)]
+#[derive(Debug, Clone, StableAbi)]
+pub struct FFI_TableOptions {
+    base_options: RHashMap<RString, RString>,
+
+    extensions: FFI_ExtensionOptions,
+}
+
+impl From<&TableOptions> for FFI_TableOptions {
+    fn from(options: &TableOptions) -> Self {
+        let base_options: RHashMap<RString, RString> = options
+            .entries()
+            .into_iter()
+            .filter_map(|entry| entry.value.map(|value| (entry.key, value)))
+            .map(|(key, value)| (key.into(), value.into()))
+            .collect();
+
+        let mut extensions = FFI_ExtensionOptions::default();
+        for (extension_name, extension) in options.extensions.iter() {
+            for entry in extension.entries().iter() {
+                if let Some(value) = entry.value.as_ref() {
+                    extensions
+                        .set(format!("{extension_name}.{}", entry.key).as_str(), value)
+                        .expect("FFI_ExtensionOptions set should always return Ok");
+                }
+            }
+        }
+
+        Self {
+            base_options,
+            extensions,
+        }
+    }
+}
+
+impl TryFrom<FFI_TableOptions> for TableOptions {
+    type Error = DataFusionError;
+    fn try_from(ffi_options: FFI_TableOptions) -> Result<Self, Self::Error> {
+        let mut options = TableOptions::default();
+        options.extensions.insert(ffi_options.extensions);
+
+        for kv_tuple in ffi_options.base_options.iter() {
+            options.set(kv_tuple.0.as_str(), kv_tuple.1.as_str())?;
+        }
+
+        Ok(options)
+    }
+}

datafusion/ffi/src/tests/config.rs

@@ -0,0 +1,51 @@
+// 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.
+
+use datafusion_common::config::ConfigExtension;
+use datafusion_common::extensions_options;
+
+use crate::config::extension_options::FFI_ExtensionOptions;
+
+extensions_options! {
+   pub struct ExternalConfig {
+       /// Should "foo" be replaced by "bar"?
+       pub is_enabled: bool, default = true
+
+       /// Some value to be extracted
+       pub base_number: usize, default = 1000
+   }
+}
+
+impl PartialEq for ExternalConfig {
+    fn eq(&self, other: &Self) -> bool {
+        self.base_number == other.base_number && self.is_enabled == other.is_enabled
+    }
+}
+impl Eq for ExternalConfig {}
+
+impl ConfigExtension for ExternalConfig {
+    const PREFIX: &'static str = "external_config";
+}
+
+pub(crate) extern "C" fn create_extension_options() -> FFI_ExtensionOptions {
+    let mut extensions = FFI_ExtensionOptions::default();
+    extensions
+        .add_config(&ExternalConfig::default())
+        .expect("add_config should be infallible for ExternalConfig");
+
+    extensions
+}

datafusion/ffi/src/tests/mod.rs

@@ -38,6 +38,7 @@ use super::table_provider::FFI_TableProvider;
 use super::udf::FFI_ScalarUDF;
 use crate::catalog_provider::FFI_CatalogProvider;
 use crate::catalog_provider_list::FFI_CatalogProviderList;
+use crate::config::extension_options::FFI_ExtensionOptions;
 use crate::proto::logical_extension_codec::FFI_LogicalExtensionCodec;
 use crate::tests::catalog::create_catalog_provider_list;
 use crate::udaf::FFI_AggregateUDF;
@@ -46,6 +47,7 @@ use crate::udwf::FFI_WindowUDF;
 
 mod async_provider;
 pub mod catalog;
+pub mod config;
 mod sync_provider;
 mod udf_udaf_udwf;
 pub mod utils;
@@ -87,6 +89,9 @@ pub struct ForeignLibraryModule {
 
     pub create_rank_udwf: extern "C" fn() -> FFI_WindowUDF,
 
+    /// Create extension options, for either ConfigOptions or TableOptions
+    pub create_extension_options: extern "C" fn() -> FFI_ExtensionOptions,
+
     pub version: extern "C" fn() -> u64,
 }
 
@@ -141,6 +146,7 @@ pub fn get_foreign_library_module() -> ForeignLibraryModuleRef {
         create_sum_udaf: create_ffi_sum_func,
         create_stddev_udaf: create_ffi_stddev_func,
         create_rank_udwf: create_ffi_rank_func,
+        create_extension_options: config::create_extension_options,
         version: super::version,
     }
     .leak_into_prefix()

datafusion/ffi/tests/ffi_config.rs

@@ -0,0 +1,73 @@
+// 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.
+
+/// Add an additional module here for convenience to scope this to only
+/// when the feature integration-tests is built
+#[cfg(feature = "integration-tests")]
+mod tests {
+    use datafusion::error::{DataFusionError, Result};
+    use datafusion_common::config::{ConfigExtension, ConfigOptions};
+    use datafusion_ffi::config::extension_options::FFI_ExtensionOptions;
+    use datafusion_ffi::tests::config::ExternalConfig;
+    use datafusion_ffi::tests::utils::get_module;
+
+    #[test]
+    fn test_ffi_extension_options() -> Result<()> {
+        let module = get_module()?;
+
+        let extension_options =
+            module
+                .create_extension_options()
+                .ok_or(DataFusionError::NotImplemented(
+                    "External test library failed to implement create_extension_options"
+                        .to_string(),
+                ))?();
+
+        println!("{:?} {}", extension_options, FFI_ExtensionOptions::PREFIX);
+
+        let mut config = ConfigOptions::new();
+        config.extensions.insert(extension_options);
+
+        fn extract_config(config: &ConfigOptions) -> ExternalConfig {
+            // For our use case of this test, we do not need to check
+            // using .get::<ExternalConfig>() but it is left here as an
+            // example to users of this crate.
+            config
+                .extensions
+                .get::<ExternalConfig>()
+                .map(|v| v.to_owned())
+                .or_else(|| {
+                    config
+                        .extensions
+                        .get::<FFI_ExtensionOptions>()
+                        .and_then(|ext| ext.to_extension().ok())
+                })
+                .expect("Should be able to get ExternalConfig")
+        }
+
+        // Verify default values are as expected
+        let returned_config = extract_config(&config);
+
+        assert_eq!(returned_config, ExternalConfig::default());
+
+        config.set("external_config.is_enabled", "false")?;
+        let returned_config = extract_config(&config);
+        assert!(!returned_config.is_enabled);
+
+        Ok(())
+    }
+}