Introduce TypePlanner for customizing type planning
#13294
Conversation
github-actions
bot
added
sql
datafusion/expr/src/planner.rs
Outdated
| /// Plan SQL type to DataFusion data type | ||
| /// | ||
| /// Returns None if not possible | ||
| fn plan_data_type(&self, _sql_type: &ast::DataType) -> Result<Option<DataType>> { |
datafusion/sql/src/planner.rs
Outdated
| @@ -401,6 +401,14 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| } | |||
|
|
|||
| pub(crate) fn convert_data_type(&self, sql_type: &SQLDataType) -> Result<DataType> { | |||
| // First check if any of the registered expr_planners can handle this type | |||
| for expr_planner in self.context_provider.get_expr_planners() { | |||
There was a problem hiding this comment.
It seems strange that the planner for individual expressions would handle query-wide data types (though I realize this is currently the only hook that is available into the sql planner)
What would you think about adding a DataTypePlanner trait / field to the planner explicitly for this conversion?
There was a problem hiding this comment.
An independent trait looks good to me. I'm thinking what's a good way to register the custom planner.
Maybe introduce a method in ContextProvder::get_data_type_planner?
pub trait ContextProvider {
...
fn get_data_type(&self) -> Option<Arc<dyn DataTypePlanner>>
None
}
}We can allow registering the data type planner in SessionState. WDYT?
There was a problem hiding this comment.
Yes I think that makes sense to me
goldmedal
changed the title
plan_data_type for ExprPlannerTypePlanner for customizing type planning
goldmedal
changed the title
TypePlanner for customizing type planningTypePlanner for customizing type planning
There was a problem hiding this comment.
Thank you @goldmedal -- this looks great
I apologize for the delay in reviewing. I have been away for a few days.
Never mind. Thanks @alamb for reviewing. |
Which issue does this PR close?
Closes #13292 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
A new trait for ExprPlanner