Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement trait based API for defining AggregateUDF #8710

Closed
alamb opened this issue Jan 1, 2024 · 1 comment · Fixed by #8733
Closed

Implement trait based API for defining AggregateUDF #8710

alamb opened this issue Jan 1, 2024 · 1 comment · Fixed by #8733
Labels
enhancement New feature or request

Comments

@alamb
Copy link
Contributor

alamb commented Jan 1, 2024

Similarly to #8568

Is your feature request related to a problem or challenge?

The current way a user implements a AggregateUDF is awkward and very hard to extend in backwards compatible ways:

They must wade through several Arc<dyn<...> typedefs to figure out how to provide the type signature and implementation

impl AggregateUDF {

/// Create a new AggregateUDF 
pub fn new(
    name: &str,
    signature: &Signature,
    return_type: &Arc<dyn Fn(&[DataType]) -> Result<Arc<DataType>, DataFusionError> + Send + Sync>,
    accumulator: &Arc<dyn Fn(&DataType) -> Result<Box<dyn Accumulator>, DataFusionError> + Send + Sync>,
    state_type: &Arc<dyn Fn(&DataType) -> Result<Arc<Vec<DataType>>, DataFusionError> + Send + Sync>
) -> AggregateUDF {
...
}

Describe the solution you'd like

Follow the pattern in #8578

  1. Create a AggregateUDFImpl trait, and AggregateUDF::new_from_impl that creates an AggregateUDF from the impl
  2. Add an example in datafusion-examples/examples/advanced_udaf.rs of using this API

I am not sure why this API is implemented like it is (other than it was consistent with ScalarUDF). As a user I would expect to be able to use a trait object like this

like

struct MyUDF { 
..
}

impl AggregateUDFImpl for MyUDF {
  fn name(&self) -> &str, 
  fn return_type(&self) -> &DataType, 
...
}

Describe alternatives you've considered

No response

Additional context

@alamb alamb added the enhancement New feature or request label Jan 1, 2024
@alamb alamb changed the title Implement trait based API for defining AggregateUDF Implement trait based API for defining AggregateUDF Jan 1, 2024
@guojidan
Copy link
Contributor

guojidan commented Jan 3, 2024

I will trying implement this : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants