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

Discussion: make it easier for specify SQL --> function translation #10534

Closed
alamb opened this issue May 15, 2024 · 7 comments
Closed

Discussion: make it easier for specify SQL --> function translation #10534

alamb opened this issue May 15, 2024 · 7 comments

Comments

@alamb
Copy link
Contributor

alamb commented May 15, 2024

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

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"
)
}

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)

@alamb
Copy link
Contributor Author

alamb commented May 15, 2024

@jayzhan211 asks #10374 (comment)

@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.

The array operator to function is syntax like array1 || array2 -> array_concat, which is in ArrayFunctionRewriter now, so I'm thinking about whether we should move this to the parser or not.

I personally think moving the translation of array1 || array --> array_concat to the parser is a better idea as it will be more efficient than trying to rewrite an expr after the fact

@jayzhan211
Copy link
Contributor

jayzhan211 commented 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 Expr given the registered function.

@alamb
Copy link
Contributor Author

alamb commented 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
Copy link
Contributor Author

alamb commented Jun 29, 2024

For anyone following along, @samuelcolvin @jayzhan211 and myself are discussing proposals for this API in

#11137 and #11168

@alamb
Copy link
Contributor Author

alamb commented Jun 30, 2024

#11180 -- looking very nice

@jayzhan211
Copy link
Contributor

To rewrite with sql planner

  • date_part
  • create_struct
  • create_named_struct
  • sql_overlay_to_expr
  • sql_position_to_expr
  • sql_substring_to_expr
  • sql_compound_identifier_to_expr

@alamb
Copy link
Contributor Author

alamb commented 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

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

No branches or pull requests

2 participants