jonahgao commented on code in PR #15183:
URL: https://github.com/apache/datafusion/pull/15183#discussion_r1998943593
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -217,13 +217,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
// it shouldn't have ordering requirement as function argument
// required ordering should be defined in OVER clause.
let is_function_window = over.is_some();
- let sql_parser_span = name.0[0].span;
+ let sql_parser_span = name.0[0].span();
let name = if name.0.len() > 1 {
// DF doesn't handle compound identifiers
// (e.g. "foo.bar") for function names yet
name.to_string()
} else {
- crate::utils::normalize_ident(name.0[0].clone())
+
crate::utils::normalize_ident(name.0[0].as_ident().unwrap().clone())
Review Comment:
Considering that more `ObjectNamePart` enums will be coming, it's better to
return an error instead of unwrap.
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -216,8 +216,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
pub(super) fn sql_case_identifier_to_expr(
Review Comment:
Not related to this PR, why is it called `case_identifier`🤔
I'm not sure what the connection is between `case` and `identifier`.
##########
datafusion/sql/src/expr/identifier.rs:
##########
@@ -231,13 +230,22 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
} else {
None
};
- let when_expr = conditions
+ let when_then_expr = conditions
Review Comment:
👍
##########
datafusion/sql/src/planner.rs:
##########
@@ -844,11 +851,11 @@ pub fn object_name_to_qualifier(
.iter()
.rev()
.zip(columns)
- .map(|(ident, column_name)| {
+ .map(|(object_name_part, column_name)| {
format!(
r#"{} = '{}'"#,
column_name,
- normalizer.normalize(ident.clone())
+
normalizer.normalize(object_name_part.as_ident().unwrap().clone())
Review Comment:
Avoiding unwrap is preferable.
##########
datafusion/sql/src/relation/join.rs:
##########
@@ -136,7 +136,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
)
} else {
let id = object_names.swap_remove(0);
- Ok(self.ident_normalizer.normalize(id))
+
Ok(self.ident_normalizer.normalize(id.as_ident().unwrap().clone()))
Review Comment:
Same as above
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -292,7 +290,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
}
SQLExpr::TypedString { data_type, value } =>
Ok(Expr::Cast(Cast::new(
- Box::new(lit(value)),
+ Box::new(lit(value.into_string().unwrap())),
Review Comment:
This unwrap appears to be safe by design.
##########
datafusion/sql/src/planner.rs:
##########
@@ -560,11 +558,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
SQLDataType::SmallInt(_) | SQLDataType::Int2(_) =>
Ok(DataType::Int16),
SQLDataType::Int(_) | SQLDataType::Integer(_) |
SQLDataType::Int4(_) => Ok(DataType::Int32),
SQLDataType::BigInt(_) | SQLDataType::Int8(_) =>
Ok(DataType::Int64),
- SQLDataType::UnsignedTinyInt(_) => Ok(DataType::UInt8),
- SQLDataType::UnsignedSmallInt(_) | SQLDataType::UnsignedInt2(_) =>
Ok(DataType::UInt16),
- SQLDataType::UnsignedInt(_) | SQLDataType::UnsignedInteger(_) |
SQLDataType::UnsignedInt4(_) => {
- Ok(DataType::UInt32)
- }
+ SQLDataType::TinyIntUnsigned(_) => Ok(DataType::UInt8),
+ SQLDataType::SmallIntUnsigned(_) | SQLDataType::Int2Unsigned(_) =>
Ok(DataType::UInt16),
+ SQLDataType::IntUnsigned(_) | SQLDataType::IntegerUnsigned(_) |
SQLDataType::Int4Unsigned(_) => {
+ Ok(DataType::UInt32)
Review Comment:
rustfmt fails on this function. We need to fix it.
```sh
$ rustfmt --config error_on_line_overflow=true,error_on_unformatted=true
datafusion/sql/src/planner.rs
error[internal]: line formatted, but exceeded maximum width (maximum: 90
(see `max_width` option), found: 104)
--> /Users/jonah/Work/datafusion/datafusion/sql/src/planner.rs:559:559:91
|
559 | SQLDataType::Int(_) | SQLDataType::Integer(_) |
SQLDataType::Int4(_) => Ok(DataType::Int32),
|
^^^^^^^^^^^^^^
|
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
