[FEAT] sql image_decode
#2757
[FEAT] sql image_decode
#2757
Conversation
CodSpeed Performance ReportMerging #2757 will degrade performances by 16.54%Comparing Summary
Benchmarks breakdown
|
| /// ```sql | ||
| /// SELECT concat('hello', 'world'); | ||
| /// ``` | ||
| /// this will be converted to: [lit("hello"), lit("world")] |
There was a problem hiding this comment.
Do we need to differentiate between function args that take as. input expressions vs functions that mandate that we can only take only take in scalar inputs?
There was a problem hiding this comment.
I'm not sure I fully understand the question.
are you asking if function args can accept exprs as inputs as well?
such as some_function(column1, arg0 := column2)?
There was a problem hiding this comment.
I think I know what Jay is saying. For example, round takes in one argument decimals that can only be a literal integer value. In those cases, I believe it is already structured so that the to_expr methods in the SQL modules include logic to make those checks before creating an expression.
There was a problem hiding this comment.
Or another example is the discussion we had a few months ago regarding seed?
| /// ```sql | ||
| /// SELECT concat('hello', 'world'); | ||
| /// ``` | ||
| /// this will be converted to: [lit("hello"), lit("world")] |
There was a problem hiding this comment.
I think I know what Jay is saying. For example, round takes in one argument decimals that can only be a literal integer value. In those cases, I believe it is already structured so that the to_expr methods in the SQL modules include logic to make those checks before creating an expression.
| match arg { | ||
| FunctionArg::Named { | ||
| name, | ||
| arg, | ||
| operator: FunctionArgOperator::Assignment, | ||
| } => match name.value.as_ref() { | ||
| "mode" => { | ||
| let arg = planner.try_unwrap_function_arg_expr(arg)?; | ||
| mode = Some(match arg.as_ref() { | ||
| Expr::Literal(LiteralValue::Utf8(s)) => s.parse()?, | ||
| _ => unsupported_sql_err!("Expected mode to be a string"), | ||
| }); | ||
| } | ||
| "on_error" => { | ||
| let arg = planner.try_unwrap_function_arg_expr(arg)?; | ||
|
|
||
| raise_on_error = match arg.as_ref() { | ||
| Expr::Literal(LiteralValue::Utf8(s)) => match s.as_ref() { | ||
| "raise" => true, | ||
| "null" => false, | ||
| _ => unsupported_sql_err!( | ||
| "Expected on_error to be 'raise' or 'null'" | ||
| ), | ||
| }, | ||
| _ => unsupported_sql_err!("Expected raise_on_error to be a boolean"), | ||
| }; | ||
| } | ||
| name => unsupported_sql_err!("Unexpected argument: '{name}'"), | ||
| }, | ||
|
|
||
| other => { | ||
| unsupported_sql_err!("Invalid arguments for image_decode: '{}'", other) | ||
| } |
There was a problem hiding this comment.
I don't have a great idea of how to do this at the moment, but I feel like there is a decent amount of boilerplate that will be shared across different functions to find and validate arguments, and we can maybe provide some common functions for simplifying that. Not blocking but a TODO for the future.
There was a problem hiding this comment.
yeah I agree, I feel like this could be solved a lot cleaner through serde, procedural macros or something else...
| match inputs { | ||
| [input] => { | ||
| let input = planner.plan_function_arg(input)?; | ||
| Ok(decode(input, None)) | ||
| } | ||
| [input, args @ ..] => { | ||
| let input = planner.plan_function_arg(input)?; | ||
| let args = ImageDecode::from_sql(args, planner)?; | ||
| Ok(decode(input, Some(args))) | ||
| } | ||
| _ => unsupported_sql_err!("Invalid arguments for image_decode: '{inputs:?}'"), | ||
| } |
There was a problem hiding this comment.
Also maybe a little messy to do part of the argument validation here and part in ImageDecode::from_sql
There was a problem hiding this comment.
yeah i think this is our usage of literals for some values, then exprs for other values surfacing. If we passed everything in as exprs, we could leave it up to ImageDecode to handle everything, but now we need to split it earlier to separate the exprs from the literals
adds sql
image_decodefunction.moves
image_decodeout ofdaft-dsland intodaft-functions.some small changes to
SQLFunctionso that it can work withScalarUDF