Skip to content

Review panics in the sql crate #3397

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.

Sign up for GitHub

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

Merged
alamb merged 2 commits into from
Sep 10, 2022
Merged

Review panics in the sql crate #3397

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5230d69
review panics
HaoYang670 Sep 8, 2022
35d1c29
fmt and clippy
HaoYang670 Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions datafusion/sql/src/planner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,10 @@ pub struct SqlToRel<'a, S: ContextProvider> {

fn plan_key(key: SQLExpr) -> Result<ScalarValue> {
let scalar = match key {
SQLExpr::Value(Value::Number(s, _)) => {
ScalarValue::Int64(Some(s.parse().unwrap()))
}
SQLExpr::Value(Value::Number(s, _)) => ScalarValue::Int64(Some(
s.parse()
.map_err(|_| ParserError(format!("Cannot parse {} as i64.", s)))?,
)),
SQLExpr::Value(Value::SingleQuotedString(s) | Value::DoubleQuotedString(s)) => {
ScalarValue::Utf8(Some(s))
}
Expand All @@ -114,9 +115,7 @@ fn plan_key(key: SQLExpr) -> Result<ScalarValue> {

fn plan_indexed(expr: Expr, mut keys: Vec<SQLExpr>) -> Result<Expr> {
let key = keys.pop().ok_or_else(|| {
DataFusionError::SQL(ParserError(
"Internal error: Missing index key expression".to_string(),
))
ParserError("Internal error: Missing index key expression".to_string())
})?;

let expr = if !keys.is_empty() {
Expand Down Expand Up @@ -254,12 +253,18 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
// nor do we support multiple object names
} => match object_type {
ObjectType::Table => Ok(LogicalPlan::DropTable(DropTable {
name: names.get(0).unwrap().to_string(),
name: names
.get(0)
.ok_or_else(|| ParserError("Missing table name.".to_string()))?
.to_string(),
if_exists,
schema: DFSchemaRef::new(DFSchema::empty()),
})),
ObjectType::View => Ok(LogicalPlan::DropView(DropView {
name: names.get(0).unwrap().to_string(),
name: names
.get(0)
.ok_or_else(|| ParserError("Missing table name.".to_string()))?
.to_string(),
if_exists,
schema: DFSchemaRef::new(DFSchema::empty()),
})),
Expand Down Expand Up @@ -308,7 +313,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let query = "SELECT * FROM information_schema.tables;";
let mut rewrite = DFParser::parse_sql(query)?;
assert_eq!(rewrite.len(), 1);
self.statement_to_plan(rewrite.pop_front().unwrap())
self.statement_to_plan(rewrite.pop_front().unwrap()) // length of rewrite is 1
}
} else {
Err(DataFusionError::Plan(
Expand Down Expand Up @@ -564,7 +569,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let mut joins = t.joins.into_iter();
let mut left = self.parse_relation_join(
left,
joins.next().unwrap(),
joins.next().unwrap(), // length of joins > 0
ctes,
outer_query_schema,
)?;
Expand Down Expand Up @@ -865,7 +870,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&& can_hash(
left_schema
.field_from_column(l)
.unwrap()
.unwrap() // the result must be OK
.data_type(),
)
{
Expand All @@ -875,7 +880,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
&& can_hash(
left_schema
.field_from_column(r)
.unwrap()
.unwrap() // the result must be OK
.data_type(),
)
{
Expand Down Expand Up @@ -2388,7 +2393,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

let mut rewrite = DFParser::parse_sql(&query)?;
assert_eq!(rewrite.len(), 1);
self.statement_to_plan(rewrite.pop_front().unwrap())
self.statement_to_plan(rewrite.pop_front().unwrap()) // length of rewrite is 1
}

fn show_create_table_to_plan(
Expand Down Expand Up @@ -2426,7 +2431,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

let mut rewrite = DFParser::parse_sql(&query)?;
assert_eq!(rewrite.len(), 1);
self.statement_to_plan(rewrite.pop_front().unwrap())
self.statement_to_plan(rewrite.pop_front().unwrap()) // length of rewrite is 1
}

/// Return true if there is a table provider available for "schema.table"
Expand Down Expand Up @@ -2678,9 +2683,13 @@ pub fn convert_data_type(sql_type: &SQLDataType) -> Result<DataType> {

// Parse number in sql string, convert to Expr::Literal
fn parse_sql_number(n: &str) -> Result<Expr> {
match n.parse::<i64>() {
Ok(n) => Ok(lit(n)),
Err(_) => Ok(lit(n.parse::<f64>().unwrap())),
match (n.parse::<i64>(), n.parse::<f64>()) {
(Ok(n), _) => Ok(lit(n)),
(Err(_), Ok(n)) => Ok(lit(n)),
(Err(_), Err(_)) => Err(DataFusionError::from(ParserError(format!(
"Cannot parse {} as i64 or f64",
n
)))),
}
Comment thread
andygrove marked this conversation as resolved.
Comment on lines +2686 to 2693

@andygrove andygrove Sep 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be simpler to do this in two steps?

Suggested change
match (n.parse::<i64>(), n.parse::<f64>()) {
(Ok(n), _) => Ok(lit(n)),
(Err(_), Ok(n)) => Ok(lit(n)),
(Err(_), Err(_)) => Err(DataFusionError::from(ParserError(format!(
"Cannot parse {} as i64 or f64",
n
)))),
}
match n.parse::<i64>() {
Ok(n) => Ok(lit(n)),
_ => match n.parse::<f64>() {
Ok(n) => Ok(lit(n)),
_ => Err(DataFusionError::from(ParserError(format!("Cannot parse {} as i64 or f64", n)))),
}
}

@alamb alamb Sep 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an alternate proposal of (yet) another way to do it: #3432

}

Expand Down