feat(error): highlight bind error location (#863)

Sqlparser introduced [span](https://docs.rs/sqlparser/latest/sqlparser/index.html#source-spans) since v0.53. This PR utilizes span to improve error reporting. ``` > create table t(a int); > select b from t; bind error: invalid column "b" LINE 1: select b from t; ^ ``` --------- Signed-off-by: Runji Wang <[email protected]>
risinglightdb · Dec 25, 2024 · c41cd65 · c41cd65
1 parent 10e1804
commit c41cd65
Show file tree

Hide file tree

Showing 16 changed files with 499 additions and 251 deletions.
diff --git a/src/binder/copy.rs b/src/binder/copy.rs
@@ -87,14 +87,18 @@ impl Binder {
                 } => {
                     let (table, is_system, is_view) = self.bind_table_id(&table_name)?;
                     if is_system {
-                        return Err(BindError::CopyTo("system table".into()));
+                        return Err(
+                            ErrorKind::CopyTo("system table".into()).with_spanned(&table_name)
+                        );
                     } else if is_view {
-                        return Err(BindError::CopyTo("view".into()));
+                        return Err(ErrorKind::CopyTo("view".into()).with_spanned(&table_name));
                     }
                     let cols = self.bind_table_columns(&table_name, &columns)?;
                     (table, cols)
                 }
-                CopySource::Query(_) => return Err(BindError::CopyTo("query".into())),
+                CopySource::Query(query) => {
+                    return Err(ErrorKind::CopyTo("query".into()).with_spanned(&*query));
+                }
             };
             let types = self.type_(cols)?;
             let types = self.egraph.add(Node::Type(types));

diff --git a/src/binder/create_function.rs b/src/binder/create_function.rs
@@ -58,26 +58,28 @@ impl Binder {
         }: crate::parser::CreateFunction,
     ) -> Result {
         let Ok((schema_name, function_name)) = split_name(&name) else {
-            return Err(BindError::BindFunctionError(
+            return Err(ErrorKind::BindFunctionError(
                 "failed to parse the input function name".to_string(),
-            ));
+            )
+            .with_spanned(&name));
         };
 
         let schema_name = schema_name.to_string();
         let name = function_name.to_string();
 
         let Some(return_type) = return_type else {
-            return Err(BindError::BindFunctionError(
+            return Err(ErrorKind::BindFunctionError(
                 "`return type` must be specified".to_string(),
-            ));
+            )
+            .into());
         };
         let return_type = crate::types::DataType::from(&return_type);
 
         // TODO: language check (e.g., currently only support sql)
         let Some(language) = language else {
-            return Err(BindError::BindFunctionError(
-                "`language` must be specified".to_string(),
-            ));
+            return Err(
+                ErrorKind::BindFunctionError("`language` must be specified".to_string()).into(),
+            );
         };
         let language = language.to_string();
 
@@ -88,17 +90,22 @@ impl Binder {
             | Some(CreateFunctionBody::AsAfterOptions(expr)) => match expr {
                 Expr::Value(Value::SingleQuotedString(s)) => s,
                 Expr::Value(Value::DollarQuotedString(s)) => s.value,
-                _ => return Err(BindError::BindFunctionError("expected string".into())),
+                _ => {
+                    return Err(
+                        ErrorKind::BindFunctionError("expected string".into()).with_spanned(&expr)
+                    )
+                }
             },
             Some(CreateFunctionBody::Return(return_expr)) => {
                 // Note: this is a current work around, and we are assuming return sql udf
                 // will NOT involve complex syntax, so just reuse the logic for select definition
                 format!("select {}", &return_expr.to_string())
             }
             None => {
-                return Err(BindError::BindFunctionError(
+                return Err(ErrorKind::BindFunctionError(
                     "AS or RETURN must be specified".to_string(),
-                ));
+                )
+                .into());
             }
         };
 

diff --git a/src/binder/create_table.rs b/src/binder/create_table.rs
@@ -62,16 +62,18 @@ impl Binder {
         let schema = self
             .catalog
             .get_schema_by_name(schema_name)
-            .ok_or_else(|| BindError::InvalidSchema(schema_name.into()))?;
+            .ok_or_else(|| ErrorKind::InvalidSchema(schema_name.into()).with_spanned(&name))?;
         if schema.get_table_by_name(table_name).is_some() {
-            return Err(BindError::TableExists(table_name.into()));
+            return Err(ErrorKind::TableExists(table_name.into()).with_spanned(&name));
         }
 
         // check duplicated column names
         let mut set = HashSet::new();
         for col in &columns {
             if !set.insert(col.name.value.to_lowercase()) {
-                return Err(BindError::ColumnExists(col.name.value.to_lowercase()));
+                return Err(
+                    ErrorKind::ColumnExists(col.name.value.to_lowercase()).with_spanned(col)
+                );
             }
         }
 
@@ -80,18 +82,20 @@ impl Binder {
 
         if ordered_pk_ids.len() > 1 {
             // multi primary key should be declared by "primary key(c1, c2...)" syntax
-            return Err(BindError::NotSupportedTSQL);
+            return Err(ErrorKind::NotSupportedTSQL.into());
         }
 
         let pks_name_from_constraints = Binder::pks_name_from_constraints(&constraints);
         if has_pk_from_column && !pks_name_from_constraints.is_empty() {
             // can't get primary key both from "primary key(c1, c2...)" syntax and
             // column's option
-            return Err(BindError::NotSupportedTSQL);
+            return Err(ErrorKind::NotSupportedTSQL.into());
         } else if !has_pk_from_column {
-            for name in &pks_name_from_constraints {
-                if !set.contains(name) {
-                    return Err(BindError::InvalidColumn(name.clone()));
+            for name in pks_name_from_constraints {
+                if !set.contains(&name.value.to_lowercase()) {
+                    return Err(
+                        ErrorKind::InvalidColumn(name.value.to_lowercase()).with_span(name.span)
+                    );
                 }
             }
             // We have used `pks_name_from_constraints` to get the primary keys' name sorted by
@@ -102,7 +106,7 @@ impl Binder {
                 .map(|name| {
                     columns
                         .iter()
-                        .position(|c| c.name.value.eq_ignore_ascii_case(name))
+                        .position(|c| c.name.value.eq_ignore_ascii_case(&name.value))
                         .unwrap() as ColumnId
                 })
                 .collect();
@@ -153,20 +157,15 @@ impl Binder {
     }
 
     /// get the primary keys' name sorted by declaration order in "primary key(c1, c2..)" syntax.
-    fn pks_name_from_constraints(constraints: &[TableConstraint]) -> Vec<String> {
+    fn pks_name_from_constraints(constraints: &[TableConstraint]) -> &[Ident] {
         for constraint in constraints {
             match constraint {
-                TableConstraint::PrimaryKey { columns, .. } => {
-                    return columns
-                        .iter()
-                        .map(|ident| ident.value.to_lowercase())
-                        .collect()
-                }
+                TableConstraint::PrimaryKey { columns, .. } => return columns,
                 _ => continue,
             }
         }
         // no primary key
-        vec![]
+        &[]
     }
 }
 

diff --git a/src/binder/create_view.rs b/src/binder/create_view.rs
@@ -16,16 +16,18 @@ impl Binder {
         let schema = self
             .catalog
             .get_schema_by_name(schema_name)
-            .ok_or_else(|| BindError::InvalidSchema(schema_name.into()))?;
+            .ok_or_else(|| ErrorKind::InvalidSchema(schema_name.into()).with_spanned(&name))?;
         if schema.get_table_by_name(table_name).is_some() {
-            return Err(BindError::TableExists(table_name.into()));
+            return Err(ErrorKind::TableExists(table_name.into()).with_spanned(&name));
         }
 
         // check duplicated column names
         let mut set = HashSet::new();
         for col in &columns {
             if !set.insert(col.name.value.to_lowercase()) {
-                return Err(BindError::ColumnExists(col.name.value.to_lowercase()));
+                return Err(
+                    ErrorKind::ColumnExists(col.name.value.to_lowercase()).with_spanned(col)
+                );
             }
         }
 
@@ -35,7 +37,7 @@ impl Binder {
 
         // TODO: support inferring column names from query
         if columns.len() != output_types.len() {
-            return Err(BindError::ViewAliasesMismatch);
+            return Err(ErrorKind::ViewAliasesMismatch.with_spanned(&name));
         }
 
         let columns: Vec<ColumnCatalog> = columns

diff --git a/src/binder/delete.rs b/src/binder/delete.rs
@@ -4,19 +4,21 @@ use super::*;
 
 impl Binder {
     pub(super) fn bind_delete(&mut self, delete: Delete) -> Result {
-        let from = match delete.from {
+        let from = match &delete.from {
             FromTable::WithFromKeyword(t) => t,
             FromTable::WithoutKeyword(t) => t,
         };
         if from.len() != 1 || !from[0].joins.is_empty() {
-            return Err(BindError::Todo(format!("delete from {from:?}")));
+            return Err(ErrorKind::Todo(format!("delete from {from:?}")).with_spanned(&delete.from));
         }
         let TableFactor::Table { name, alias, .. } = &from[0].relation else {
-            return Err(BindError::Todo(format!("delete from {from:?}")));
+            return Err(
+                ErrorKind::Todo(format!("delete from {from:?}")).with_spanned(&from[0].relation)
+            );
         };
         let (table_id, is_system, is_view) = self.bind_table_id(name)?;
         if is_system || is_view {
-            return Err(BindError::CanNotDelete);
+            return Err(ErrorKind::CanNotDelete.with_spanned(name));
         }
         let scan = self.bind_table_def(name, alias.clone(), true)?;
         let cond = self.bind_where(delete.selection)?;

diff --git a/src/binder/drop.rs b/src/binder/drop.rs
@@ -11,10 +11,10 @@ impl Binder {
         cascade: bool,
     ) -> Result {
         if !matches!(object_type, ObjectType::Table | ObjectType::View) {
-            return Err(BindError::Todo(format!("drop {object_type:?}")));
+            return Err(ErrorKind::Todo(format!("drop {object_type:?}")).into());
         }
         if cascade {
-            return Err(BindError::Todo("cascade drop".into()));
+            return Err(ErrorKind::Todo("cascade drop".into()).into());
         }
         let mut table_ids = Vec::with_capacity(names.len());
         for name in names {
@@ -24,7 +24,8 @@ impl Binder {
             if if_exists && result.is_none() {
                 continue;
             }
-            let table_id = result.ok_or_else(|| BindError::InvalidTable(table_name.into()))?;
+            let table_id = result
+                .ok_or_else(|| ErrorKind::InvalidTable(table_name.into()).with_spanned(&name))?;
             let id = self.egraph.add(Node::Table(table_id));
             table_ids.push(id);
         }