jayzhan211 commented on PR #11168:
URL: https://github.com/apache/datafusion/pull/11168#issuecomment-2197850460
I think it is a great idea overall. There are only two things that I think
we could improve on
1. Avoid clone like what you have mentioned, I don't have a good solution
for now too.
2. Extend it for more kinds of ops like FieldAccess `plan_field_access`
Given the current design, if we want to support `plan_field_access` we need
to add the function in trait, and doesn't look flexible enough if there are
many kinds of custom planning
```rust
for planner in self.planners.iter() {
if let Some(expr) =
planner.plan_binary_op(op.clone(), left.clone(),
right.clone(), schema)?
{
return Ok(expr);
}
if let Some(expr) =
planner.plan_indices(expr....)?
{
return Ok(expr);
}
...
}
```
We could define a more general function and enum that includes all the
possible ast node. I'm not sure if this is general enough since these are two
things that I know are rewritten to function in datafusion right now.
```rust
// Node from sqlparser that are possible for rewrite
pub enum AstNode {
Operator(sqlparser::ast::BinaryOperator),
Subscript(sqlparser::ast::Subscript), // For FieldAccess rewrite
}
/// This trait allows users to customize the behavior of the SQL planner
pub trait UserDefinedPlanner {
fn plan(&self, ast_node: AstNode, exprs: Vec<Expr>, _schema: &DFSchema)
-> Result<Option<Expr>> {
Ok(None)
}
}
```
--
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]
