-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: enhance-CLI-query-header-for-cast-expressions-with-literals #15736
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
base: main
Are you sure you want to change the base?
feat: enhance-CLI-query-header-for-cast-expressions-with-literals #15736
Conversation
datafusion/sql/src/parser.rs
Outdated
DataType::Float(_) => "FLOAT", | ||
DataType::Double(_) => "DOUBLE", | ||
DataType::Text => "TEXT", | ||
// TODO: need to support more DataType, probably use an Extension Trait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support a full list of enum elements for DataType
here, this DataType
comes from an external library. This probably means everything we update this API, we need to make accommodation as well.
Or we can look into using strum
crate to auto derive ToString
for all DataType
, we are currently using it for the datafusion proto
crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, supported data type result would look like this:
select cast('1' as int);
+------------------+
| cast('1' as int) |
+------------------+
| 1 |
+------------------+
unsupported data type result would look like this:
select cast(1 as string);
+-------------------------+
| cast(1 as string(none)) |
+-------------------------+
| 1 |
+-------------------------+
datafusion/sql/src/parser.rs
Outdated
Expr::Value(sqlparser::ast::ValueWithSpan { value: Value::Number(n, _), .. }) => { | ||
n.clone() | ||
}, | ||
// TODO: need to take care of other types of values, like TimeStamp, etc... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently only matching specific case for select cast('1' as int)
, we need to support other types as well if we want to go with this idea. I'm thinking about a way to generalize the Value
extraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For enum Value
, there are 22 types to support, I can write them into a function if we want to go ahead with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole code modification will be turn into several functions for code readability and maintainability, I'm leaving for now just to validate the idea.
Which issue does this PR close?
Rationale for this change
Stated in #5221
What changes are included in this PR?
SQL parser modification
Are these changes tested?
Not yet
Are there any user-facing changes?
Yes