Repository navigation
Discussion: make it easier for specify SQL --> function translation #10534
Description
I extracted this from a conversation between @jayzhan211 and myself
Basically the usecase is that the sql planner converts things like [1, 2, 3] to make_array(1, 2, 3) and the name make_array is hard coded
> @alamb Do you think we should also rewrite the array operator to function in parser step? It is currently in optimizer step. I think the downside of moving array rewrite in parser step is that if any user expects different array function with the same syntax, then they can't do it since we don't have "user-defined" parser mechanism now. But the benefit is that we can eliminate intermediate binary expression.
I agree that changing the parser to insert a call to get field access directly is a good idea (and would be consistent and allow us to remove Expr::GetFieldAccess
Have you thought about "user-defined" parser idea before, the way that user can define their own expression to get from the syntax? Is it useful in production? 🤔
One thing I have thought about is changing the hard coded lookup of function names from a pattern like this
datafusion/datafusion/sql/src/expr/value.rs
Lines 144 to 150 in fc34dac
To be something more structured
pub trait PlannerFunctions {
/// return the UDF to use for creating arrays ("make_array") by default:
fn make_array(&self) -> Result<ScalarUDF>;
...
// similar functions for other built in functions
}And then instead of
if let Some(udf) = self.context_provider.get_function_meta("make_array") {
Ok(Expr::ScalarFunction(ScalarFunction::new_udf(udf, values)))
} else {
not_impl_err!(
"array_expression featrue is disable, So should implement make_array UDF by yourself"
)
} The planner might look like
let udf = self.planner_functions.make_array()?;
Ok(Expr::ScalarFunction(ScalarFunction::new_udf(udf, values))) But I haven't had a usecase to do that myself yet
Originally posted by @alamb in #10374 (comment)
Activity
alamb commentedon May 15, 2024
@jayzhan211 asks #10374 (comment)
I personally think moving the translation of
array1 || array-->array_concatto the parser is a better idea as it will be more efficient than trying to rewrite an expr after the factjayzhan211 commentedon May 16, 2024
I rethought the issue in #10102, and I found it is strongly related to the idea of the user-defined parser mentioned here, that we can define the returned
Exprgiven the registered function.alamb commentedon May 16, 2024
In general, I think the idea of allowing users to customize the behavior of the sql planner is reasonable. However I am not entirely sure if we need to modify the planner itself, or if it would be a better approach for users to implement rewrite passes after the existing planner runs 🤔
alamb commentedon Jun 29, 2024
For anyone following along, @samuelcolvin @jayzhan211 and myself are discussing proposals for this API in
#11137 and #11168
alamb commentedon Jun 30, 2024
#11180 -- looking very nice
jayzhan211 commentedon Jul 2, 2024
To rewrite with sql planner
alamb commentedon Jul 2, 2024
I filed #11207 to track the work to move the remaining functions to the user defined extension planner so closing this one