martin-g commented on code in PR #20222:
URL: https://github.com/apache/datafusion/pull/20222#discussion_r3006200202
##########
datafusion/catalog/src/table.rs:
##########
@@ -507,10 +507,30 @@ pub trait TableProviderFactory: Debug + Sync + Send {
) -> Result<Arc<dyn TableProvider>>;
}
+/// Describes arguments provided to the table function call.
+pub struct TableFunctionArgs<'a> {
+ /// Call arguments.
+ pub args: &'a [Expr],
Review Comment:
```suggestion
pub exprs: &'a [Expr],
```
This would be clearer, I think.
##########
datafusion/catalog/src/table.rs:
##########
@@ -507,10 +507,30 @@ pub trait TableProviderFactory: Debug + Sync + Send {
) -> Result<Arc<dyn TableProvider>>;
}
+/// Describes arguments provided to the table function call.
+pub struct TableFunctionArgs<'a> {
Review Comment:
The new struct is `pub` and all its fields are `pub`. This allows the users
to construct it without a constructor method.
Consider adding a constructor method, e.g. `new(...)` and hiding the fields
+ using getter methods.
This will make it easier to extend the struct with more fields in the
future, if needed.
##########
datafusion/ffi/src/udtf.rs:
##########
@@ -215,13 +273,32 @@ impl From<FFI_TableFunction> for Arc<dyn
TableFunctionImpl> {
}
impl TableFunctionImpl for ForeignTableFunction {
- fn call(&self, args: &[Expr]) -> Result<Arc<dyn TableProvider>> {
+ fn call_with_args(&self, args: TableFunctionArgs) -> Result<Arc<dyn
TableProvider>> {
+ let session =
+ FFI_SessionRef::new(args.session, None,
self.0.logical_codec.clone());
Review Comment:
```suggestion
FFI_SessionRef::new(args.session, self.0.runtime(),
self.0.logical_codec.clone());
```
What is the reason to not pass the runtime here ?
##########
datafusion/catalog/src/table.rs:
##########
@@ -507,10 +507,30 @@ pub trait TableProviderFactory: Debug + Sync + Send {
) -> Result<Arc<dyn TableProvider>>;
}
+/// Describes arguments provided to the table function call.
+pub struct TableFunctionArgs<'a> {
+ /// Call arguments.
+ pub args: &'a [Expr],
+ /// Session within which the function is called.
+ pub session: &'a dyn Session,
+}
+
/// A trait for table function implementations
pub trait TableFunctionImpl: Debug + Sync + Send + Any {
/// Create a table provider
- fn call(&self, args: &[Expr]) -> Result<Arc<dyn TableProvider>>;
+ #[deprecated(
+ since = "53.0.0",
+ note = "Implement `TableFunctionImpl::call_with_args` instead"
+ )]
+ fn call(&self, _args: &[Expr]) -> Result<Arc<dyn TableProvider>> {
+ internal_err!("unimplemented")
Review Comment:
```suggestion
internal_err!("TableFunctionImpl::call is not implemented. Implement
TableFunctionImpl::call_with_args instead.")
```
##########
datafusion/catalog/src/table.rs:
##########
@@ -507,10 +507,30 @@ pub trait TableProviderFactory: Debug + Sync + Send {
) -> Result<Arc<dyn TableProvider>>;
}
+/// Describes arguments provided to the table function call.
+pub struct TableFunctionArgs<'a> {
+ /// Call arguments.
+ pub args: &'a [Expr],
+ /// Session within which the function is called.
+ pub session: &'a dyn Session,
Review Comment:
Are the expressions depending on the Session ?
I think you should use a second lifetime here
```suggestion
pub session: &'b dyn Session,
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
