Make it easier to register configuration extension options

[alamb]

Also, inspired by simplification you've done in SessionContext, maybe we should add companion object to FunctionFactory so it can register configuration as well. At the moment its a bit of work to register configuration:

https://github.com/milenkovicm/torchfusion/blob/634c2a4e39b81e1969671db8faea50b96bc43c7f/src/lib.rs#L101

let mut session_config = SessionConfig::new().with_information_schema(true);

session_config
      .options_mut()
      .extensions
      // register torch factory configuration
      .insert(TorchConfig::default());

it would be much better if config could be registered when ctx.with_function_factory(...) called ... but that could be a follow up PR

Originally posted by @milenkovicm in #9482 (comment)

[alamb]

@milenkovicm Is there a reason you don't want to put the configuration directly on the FunctionFactory (e.g. on the CustomFunctionFactory in #9482)?

If not, perhaps we could add an API like SessionConfig.with_extension_options:

let mut session_config = SessionConfig::new()
  .with_information_schema(true)
  // register torch factory configuration
  .with_extension_options(TorchConfig::default())

[milenkovicm]

@alamb, having it as the part of FunctionFactory would be ideal, imho. Something like:

#[async_trait]
pub trait FunctionFactory: Sync + Send {

    type Config : Default + datafusion_common::config::ExtensionOptions + datafusion_common::config::ConfigExtension;
    
    async fn create(
        &self,
        state: &SessionConfig,
        statement: CreateFunction,
    ) -> Result<RegisterFunction>;
}

it requires a bit of mental gymnastic to make ti right with generics (at least in my head 😂)

wdyt ?

[milenkovicm]

ConfigExtension is not object safe, which makes my initial idea a bit harder to implement

[milenkovicm]

// does not work
//
// the trait `ConfigExtension` cannot be made into an object
// `ConfigExtension` cannot be made into an object
// for a trait to be "object safe" it needs to allow building a vtable to 
// allow the call to be resolvable dynamically; for more information visit
//
fn configuration(&self) -> Option<&dyn ConfigExtension> {
    todo!()
}

//
// does work but I don't like it 
// 
// user can  return whatever they want, registration process 
// should try to register extension ... 
// this would involve creating documentation to 
// explain what we want, which makes bad API IMHO 
fn configuration(&self) -> Option<dyn Any> {
    None
}

// we can let user do it on init
// user should know what they are doing 
//
// i dont find this api particullary useful at this point
fn init(&self, state: &mut SessionState) {
    state.options().extensions.insert(extension)
}

/// config extension to register with this function factory 
fn config_extension<T:ConfigExtension>() -> Option<T> where Self: Sized {
    None
}

pub fn with_function_factory(
    mut self,
    function_factory: Arc<dyn FunctionFactory>,
) -> Self {
    // the size for values of type `dyn FunctionFactory` cannot be known at compilation time
    // the trait `Sized` is not implemented for `dyn FunctionFactory
    let c = <dyn FunctionFactory>::config_extension();

    self.function_factory = Some(function_factory);
    self
}

// it would force user to define `ConfigExtension`
pub fn with_function_factory<T: FunctionFactory + 'static, C : ConfigExtension>(
    mut self,
    function_factory: Arc<T>,
) -> Self {
    let c : Option<C> = T::config_extension();
    self.function_factory = Some(function_factory);
    
    self
}

[alamb]

@alamb, having it as the part of FunctionFactory would be ideal, imho. Something like:

What about something like this (no changes to FunctionFactory, but literally put it on the struct directly)

/// Configuration is stored specifically for this function factory
#[derive(Debug, Default)]
struct CustomFunctionFactory {
  config: CustomConfigObject
}

What I am missing is the usecase for dynamically passing the configuration. It would make sense if there was DDL like

CREATE FUNCTION FACTORY ...

Where you had to connect the configuration but didn't know it beforehand. But if you have to make a CustomFunctionFactory instance anyway, why not just give it the config directly 🤔

[milenkovicm]

Use case is not to register function factory it's more to configure it once it's registered, something like:

ctx.sql("SET torchfusion.device = cpu").await?.show().await?;

It can be done now, but there are few steps which should be done. IMHO,it makes more sense to keep it like that than make FunctionFactory more complicated.

[alamb]

Use case is not to register function factory it's more to configure it once it's registered, something like:

Ah, that makes sense

So in that case what do you think about this

let mut session_config = SessionConfig::new()
  .with_information_schema(true)
  // register torch factory configuration
  .with_extension_options(TorchConfig::default())

That just does the gymnastics like

self.state.write()
      .session_config
      .options_mut()
      .extensions
      // register torch factory configuration
      .insert(TorchConfig::default());

[milenkovicm]

That would definitely work better than any of previous changes I've done to function factory.

I'd propose to change nothing at the moment, leave interfaces as they are if this is not common use case. Would it make sense to add it from your perspective? Is configuration extension commonly used?

[alamb]

Would it make sense to add it from your perspective?

I think so

Is configuration extension commonly used?

I am not sure

[milenkovicm]

Then let's do it like that and close this issue. Could you please push PR?

Thanks a lot @alamb

[milenkovicm]

Hey @alamb , I thought you pushed

SessionConfig.with_extension_options(...)

but I don't see it in the code, Am I looking at the wrong place? I can push change if you agree.
Thanks, regards

[alamb]

but I don't see it in the code, Am I looking at the wrong place? I can push change if you agree.

I don't think I did (maybe I got confused above). It still seems like a good change to me to make it eaiser to configure SessionContext

A PR would be most appreciated

[milenkovicm]

Apologies for confusion @alamb. Will push PR later