Allow different types of query variables (@@var) rather than just string
#1943
Conversation
github-actions
bot
added
datafusion
| @@ -399,7 +399,7 @@ impl fmt::Debug for Expr { | |||
| match self { | |||
| Expr::Alias(expr, alias) => write!(f, "{:?} AS {}", expr, alias), | |||
| Expr::Column(c) => write!(f, "{}", c), | |||
| Expr::ScalarVariable(var_names) => write!(f, "{}", var_names.join(".")), | |||
| Expr::ScalarVariable(_, var_names) => write!(f, "{}", var_names.join(".")), | |||
There was a problem hiding this comment.
It'll be better to print the data type, such as
Expr::ScalarVariable(data_type, var_names) => write!(f, "{}, data type: {}", var_names.join("."), data_type),
There was a problem hiding this comment.
I avoided this because it looks like the Debug implementation for Expr tries to print SQL-compatible output, but, for example, "@v0, data type: DataType::Utf8" wouldn't be SQL-like
There was a problem hiding this comment.
I avoided this because it looks like the Debug implementation for Expr tries to print SQL-compatible output, but, for example, "@v0, data type: DataType::Utf8" wouldn't be SQL-like
There was a problem hiding this comment.
it looks like the Debug implementation for Expr tries to print SQL-compatible output
It's a good point, makes sense to me.
|
BTW, ballista indirectly uses |
I thought about that but it looks like the existing tests for the user variables already cover it? |
Are you referring to here?
|
Could you elaborate more on what kinds of issues you are expecting? The crux of this change is that, previously, the types of all |
Sorry for the confusion. I went through the original code and the changes made to this ticket. I found the changes will eventually generate I would appreciate it if you could give an example usage about the ticket to help me understand and learn ❤️, thanks @maxburke |
|
Thank you @maxburke . I agree with @xudong963 that all this PR really needs are some tests and it would be good to go 👍 Maybe you can adapt / update the ones in https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/execution/context.rs#L1286-L1313 ? |
|
Test added / extended! |
alamb
changed the title
@@var) rather than just string
| @@ -1300,14 +1317,15 @@ mod tests { | |||
| ctx.register_table("dual", provider)?; | |||
|
|
|||
| let results = | |||
| plan_and_collect(&mut ctx, "SELECT @@version, @name FROM dual").await?; | |||
| plan_and_collect(&mut ctx, "SELECT @@version, @name, @integer + 1 FROM dual") | |||
Closes #1942