From 6fb6b327fe4c935b4398be67423c46fa7490d6d8 Mon Sep 17 00:00:00 2001 From: broccoliSpicy Date: Sun, 26 Mar 2023 00:48:01 +0800 Subject: [PATCH 1/9] fix issue# 7691: Incorrect cast when specifying columns --- e2e_test/batch/basic/dml.slt.part | 24 ++ src/frontend/src/binder/insert.rs | 358 +++++++++++++++++----------- src/frontend/src/binder/set_expr.rs | 2 +- src/frontend/src/binder/values.rs | 43 ++-- 4 files changed, 260 insertions(+), 167 deletions(-) diff --git a/e2e_test/batch/basic/dml.slt.part b/e2e_test/batch/basic/dml.slt.part index 1b4ad5e459c2..b5dd642d17cd 100644 --- a/e2e_test/batch/basic/dml.slt.part +++ b/e2e_test/batch/basic/dml.slt.part @@ -1,6 +1,30 @@ statement ok SET RW_IMPLICIT_FLUSH TO true; +statement ok +create table t1 (v1 real, v2 int, v3 varchar); + + +# Insert + +statement ok +insert into t1 (v2, v1, v3) values (1, 2, 'a'), (3, 4, 'b'); + +query RI rowsort +select * from t1; +---- +2 1 a +4 3 b + +statement ok +insert into t1 (v2, v1) values (1, 2), (3, 4); + +statement ok +insert into t1 values (1, 2), (3, 4); + +statement ok +drop table t1; + statement ok create table t (v1 real, v2 int); diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index 46b6c969cb05..e5c480007741 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::HashSet; +use std::collections::HashMap; use itertools::Itertools; use risingwave_common::catalog::{Schema, TableVersionId}; @@ -108,156 +108,236 @@ impl Binder { .collect_vec(); let row_id_index = table_catalog.row_id_index; - let expected_types: Vec = columns_to_insert - .iter() - .map(|c| c.data_type().clone()) - .collect(); + let (returning_list, fields) = self.bind_returning_list(returning_items)?; + let is_returning = !returning_list.is_empty(); - // When the column types of `source` query do not match `expected_types`, casting is - // needed. - // - // In PG, when the `source` is a `VALUES` without order / limit / offset, special treatment - // is given and it is NOT equivalent to assignment cast over potential implicit cast inside. - // For example, the following is valid: - // ``` - // create table t (v1 time); - // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); - // ``` - // But the followings are not: - // ``` - // values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); - // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05') limit 1; - // ``` - // Because `timestamp` can cast to `time` in assignment context, but no casting between them - // is allowed implicitly. - // - // In this case, assignment cast should be used directly in `VALUES`, suppressing its - // internal implicit cast. - // In other cases, the `source` query is handled on its own and assignment cast is done - // afterwards. - let (source, cast_exprs, nulls_inserted) = match source { - Query { - with: None, - body: SetExpr::Values(values), - order_by: order, - limit: None, - offset: None, - fetch: None, - } if order.is_empty() => { - let (values, nulls_inserted) = - self.bind_values(values, Some(expected_types.clone()))?; - let body = BoundSetExpr::Values(values.into()); - ( - BoundQuery { - body, - order: vec![], - limit: None, - offset: None, - with_ties: false, - extra_order_exprs: vec![], - }, - vec![], - nulls_inserted, - ) + // create table t (a int, b real, c varchar) + // case 1: insert into t (target_columns) values (values) + // case 2: insert into t values (values) + let has_target_col = columns.len() > 0; + if has_target_col { + let mut target_table_col_indices: Vec = vec![]; + let mut cols_to_insert_name_idx_map: HashMap = HashMap::new(); + for (col_idx, col) in columns_to_insert.iter().enumerate() { + cols_to_insert_name_idx_map.insert(col.name().to_string(), col_idx); } - query => { - let bound = self.bind_query(query)?; - let actual_types = bound.data_types(); - let cast_exprs = match expected_types == actual_types { - true => vec![], - false => Self::cast_on_insert( - &expected_types, - actual_types - .into_iter() - .enumerate() - .map(|(i, t)| InputRef::new(i, t).into()) - .collect(), - )?, - }; - (bound, cast_exprs, false) + for target_col in &columns { + let target_col_name= &target_col.real_value(); + match cols_to_insert_name_idx_map.get_mut(target_col_name) { + Some(value_ref) => { + if *value_ref == usize::MAX { + return Err(RwError::from(ErrorCode::BindError( + "Column specified more than once".to_string(), + ))); + } + target_table_col_indices.push(*value_ref); + *value_ref = usize::MAX; + } + None => { + // Invalid column name found + return Err(RwError::from(ErrorCode::BindError(format!( + "Column {} not found in table {}", + target_col_name, table_name + )))); + } + } } - }; - let mut target_table_col_indices: Vec = vec![]; - 'outer: for query_column in &columns { - let column_name = query_column.real_value(); - for (col_idx, table_column) in columns_to_insert.iter().enumerate() { - if column_name == table_column.name() { - target_table_col_indices.push(col_idx); - continue 'outer; + // columns that are in the target table but not in the provided target columns + if target_table_col_indices.len() != columns_to_insert.len() { + for col in &columns_to_insert { + if let Some(col_to_insert_idx) = cols_to_insert_name_idx_map.get(col.name()) { + if *col_to_insert_idx != usize::MAX { + target_table_col_indices.push(*col_to_insert_idx); + } + } else { + unreachable!(); + } } } - // Invalid column name found - return Err(RwError::from(ErrorCode::BindError(format!( - "Column {} not found in table {}", - column_name, table_name - )))); - } - // create table t1 (v1 int, v2 int); insert into t1 (v2) values (5); - // We added the null values above. Above is equivalent to - // insert into t1 values (NULL, 5); - let target_table_col_indices = if !target_table_col_indices.is_empty() && nulls_inserted { - let provided_insert_cols: HashSet = - target_table_col_indices.iter().cloned().collect(); + let expected_types: Vec = target_table_col_indices + .iter() + .map(|idx| columns_to_insert[*idx].data_type().clone()).collect(); - let mut result: Vec = target_table_col_indices.clone(); - for i in 0..columns_to_insert.len() { - if !provided_insert_cols.contains(&i) { - result.push(i); - } - } - result - } else { - target_table_col_indices - }; + // When the column types of `source` query do not match `expected_types`, casting is + // needed. + // + // In PG, when the `source` is a `VALUES` without order / limit / offset, special treatment + // is given and it is NOT equivalent to assignment cast over potential implicit cast inside. + // For example, the following is valid: + // ``` + // create table t (v1 time); + // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); + // ``` + // But the followings are not: + // ``` + // values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); + // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05') limit 1; + // ``` + // Because `timestamp` can cast to `time` in assignment context, but no casting between them + // is allowed implicitly. + // + // In this case, assignment cast should be used directly in `VALUES`, suppressing its + // internal implicit cast. + // In other cases, the `source` query is handled on its own and assignment cast is done + // afterwards. + let (source, cast_exprs) = match source { + Query { + with: None, + body: SetExpr::Values(values), + order_by: order, + limit: None, + offset: None, + fetch: None, + } if order.is_empty() => { + assert!(!values.0.is_empty()); + let err_msg = match columns.len().cmp(&values.0[0].len()) { + std::cmp::Ordering::Equal => None, + // e.g. create table t (a int, b real) + // insert into t (v1, v2) values (7) + std::cmp::Ordering::Greater => Some("INSERT has more target columns than values"), + // e.g. create table t (a int, b real) + // insert into t (v1) values (7, 13) + std::cmp::Ordering::Less => Some("INSERT has less target columns than values"), + }; - let (returning_list, fields) = self.bind_returning_list(returning_items)?; - let is_returning = !returning_list.is_empty(); - // validate that query has a value for each target column, if target columns are used - // create table t1 (v1 int, v2 int); - // insert into t1 (v1, v2, v2) values (5, 6); // ...more target columns than values - // insert into t1 (v1) values (5, 6); // ...less target columns than values - let err_msg = match target_table_col_indices.len().cmp(&expected_types.len()) { - std::cmp::Ordering::Equal => None, - std::cmp::Ordering::Greater => Some("INSERT has more target columns than values"), - std::cmp::Ordering::Less => Some("INSERT has less target columns than values"), - }; + if let Some(msg) = err_msg { + return Err(RwError::from(ErrorCode::BindError( + msg.to_string(), + ))); + } + + let values = self.bind_values(values, Some(expected_types.clone()))?; + let body = BoundSetExpr::Values(values.into()); + ( + BoundQuery { + body, + order: vec![], + limit: None, + offset: None, + with_ties: false, + extra_order_exprs: vec![], + }, + vec![], + ) + }, + query => { + let bound = self.bind_query(query)?; + let actual_types = bound.data_types(); + let cast_exprs = match expected_types == actual_types { + true => vec![], + false => Self::cast_on_insert( + &expected_types, + actual_types + .into_iter() + .enumerate() + .map(|(i, t)| InputRef::new(i, t).into()) + .collect(), + )?, + }; + (bound, cast_exprs) + }, + }; + let insert = BoundInsert { + table_id, + table_version_id, + table_name, + owner, + row_id_index, + column_indices: target_table_col_indices, + source, + cast_exprs, + returning_list, + returning_schema: if is_returning { + Some(Schema { fields }) + } else { + None + }, + }; + return Ok(insert) + } else { + let expected_types :Vec = columns_to_insert + .iter() + .map(|c| c.data_type().clone()) + .collect(); + let (source, cast_exprs) = match source { + Query { + with: None, + body: SetExpr::Values(values), + order_by: order, + limit: None, + offset: None, + fetch: None, + } if order.is_empty() => { + assert!(!values.0.is_empty()); + let err_msg = match columns_to_insert.len().cmp(&values.0[0].len()) { + std::cmp::Ordering::Equal => None, + // e.g. create table t (a int, b real) + // insert into t values (7) + // this kind of usage is fine, null values will be provided implicitly. + std::cmp::Ordering::Greater => None, + // e.g. create table t (a int, b real) + // insert into t values (7, 13, 17) + std::cmp::Ordering::Less => Some("INSERT has more expressions than target columns"), + }; - if let Some(msg) = err_msg && !target_table_col_indices.is_empty() { - return Err(RwError::from(ErrorCode::BindError( - msg.to_string(), - ))); - } + if let Some(msg) = err_msg { + return Err(RwError::from(ErrorCode::BindError( + msg.to_string(), + ))); + } + + let values = self.bind_values(values, Some(expected_types.clone()))?; + let body = BoundSetExpr::Values(values.into()); + ( + BoundQuery { + body, + order: vec![], + limit: None, + offset: None, + with_ties: false, + extra_order_exprs: vec![], + }, + vec![], - // Check if column was used multiple times in query e.g. - // insert into t1 (v1, v1) values (1, 5); - let mut uniq_cols = target_table_col_indices.clone(); - uniq_cols.sort_unstable(); - uniq_cols.dedup(); - if target_table_col_indices.len() != uniq_cols.len() { - return Err(RwError::from(ErrorCode::BindError( - "Column specified more than once".to_string(), - ))); + ) + }, + query => { + let bound = self.bind_query(query)?; + let actual_types = bound.data_types(); + let cast_exprs = match expected_types == actual_types { + true => vec![], + false => Self::cast_on_insert( + &expected_types, + actual_types + .into_iter() + .enumerate() + .map(|(i, t)| InputRef::new(i, t).into()) + .collect(), + )?, + }; + (bound, cast_exprs) + } + }; + let insert = BoundInsert { + table_id, + table_version_id, + table_name, + owner, + row_id_index, + column_indices: (0..columns_to_insert.len()).collect::>(), + source, + cast_exprs, + returning_list, + returning_schema: if is_returning { + Some(Schema { fields }) + } else { + None + }, + }; + return Ok(insert) } - - let insert = BoundInsert { - table_id, - table_version_id, - table_name, - owner, - row_id_index, - column_indices: target_table_col_indices, - source, - cast_exprs, - returning_list, - returning_schema: if is_returning { - Some(Schema { fields }) - } else { - None - }, - }; - Ok(insert) } /// Cast a list of `exprs` to corresponding `expected_types` IN ASSIGNMENT CONTEXT. Make sure diff --git a/src/frontend/src/binder/set_expr.rs b/src/frontend/src/binder/set_expr.rs index dd1d646ee3da..5696e90a0b7d 100644 --- a/src/frontend/src/binder/set_expr.rs +++ b/src/frontend/src/binder/set_expr.rs @@ -114,7 +114,7 @@ impl Binder { pub(super) fn bind_set_expr(&mut self, set_expr: SetExpr) -> Result { match set_expr { SetExpr::Select(s) => Ok(BoundSetExpr::Select(Box::new(self.bind_select(*s)?))), - SetExpr::Values(v) => Ok(BoundSetExpr::Values(Box::new(self.bind_values(v, None)?.0))), + SetExpr::Values(v) => Ok(BoundSetExpr::Values(Box::new(self.bind_values(v, None)?))), SetExpr::Query(q) => Ok(BoundSetExpr::Query(Box::new(self.bind_query(*q)?))), SetExpr::SetOperation { op, diff --git a/src/frontend/src/binder/values.rs b/src/frontend/src/binder/values.rs index 44ac158270d8..01a7fa0d501e 100644 --- a/src/frontend/src/binder/values.rs +++ b/src/frontend/src/binder/values.rs @@ -84,12 +84,11 @@ fn values_column_name(values_id: usize, col_id: usize) -> String { impl Binder { /// Bind [`Values`] with given `expected_types`. If no types are expected, a compatible type for /// all rows will be used. - /// Returns true if null values were inserted pub(super) fn bind_values( &mut self, values: Values, expected_types: Option>, - ) -> Result<(BoundValues, bool)> { + ) -> Result { assert!(!values.0.is_empty()); self.context.clause = Some(Clause::Values); @@ -102,33 +101,24 @@ impl Binder { // Adding Null values in case user did not specify all columns. E.g. // create table t1 (v1 int, v2 int); insert into t1 (v2) values (5); - let vec_len = bound[0].len(); - let nulls_to_insert = if let Some(expected_types) = &expected_types && expected_types.len() > vec_len { - let nulls_to_insert = expected_types.len() - vec_len; + let mut num_columns = bound[0].len(); + if bound.iter().any(|row| row.len() != num_columns) { + return Err( + ErrorCode::BindError("VALUES lists must all be the same length".into()).into(), + ); + } + if let Some(expected_types) = &expected_types && expected_types.len() > num_columns { + let nulls_to_insert = expected_types.len() - num_columns; for row in &mut bound { - if vec_len != row.len() { - return Err(ErrorCode::BindError( - "VALUES lists must all be the same length".into(), - ) - .into()); - } for i in 0..nulls_to_insert { - let t = expected_types[vec_len + i].clone(); + let t = expected_types[num_columns + i].clone(); row.push(ExprImpl::literal_null(t)); } } - nulls_to_insert - } else { - 0 - }; - - // only check for this condition again if we did not insert any nulls - let num_columns = bound[0].len(); - if nulls_to_insert == 0 && bound.iter().any(|row| row.len() != num_columns) { - return Err( - ErrorCode::BindError("VALUES lists must all be the same length".into()).into(), - ); + num_columns = expected_types.len(); } +/* + */ // Calculate column types. let types = match expected_types { @@ -173,13 +163,12 @@ impl Binder { ) .into()); } - Ok((bound_values, nulls_to_insert > 0)) + Ok(bound_values) } } #[cfg(test)] mod tests { - use risingwave_common::util::iter_util::zip_eq_fast; use risingwave_sqlparser::ast::{Expr, Value}; @@ -207,8 +196,8 @@ mod tests { .collect(), ); - assert_eq!(res.0.schema, schema); - for vec in res.0.rows { + assert_eq!(res.schema, schema); + for vec in res.rows { for (expr, ty) in zip_eq_fast(vec, schema.data_types()) { assert_eq!(expr.return_type(), ty); } From 584c0eaeb7fe83a289dd8e2696fe119086fa1f55 Mon Sep 17 00:00:00 2001 From: broccoliSpicy Date: Sun, 26 Mar 2023 14:17:50 +0800 Subject: [PATCH 2/9] fix lint --- .../planner_test/tests/testdata/insert.yaml | 2 +- src/frontend/src/binder/insert.rs | 28 ++++++++----------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/frontend/planner_test/tests/testdata/insert.yaml b/src/frontend/planner_test/tests/testdata/insert.yaml index 423f8e0e97f7..866601f9b980 100644 --- a/src/frontend/planner_test/tests/testdata/insert.yaml +++ b/src/frontend/planner_test/tests/testdata/insert.yaml @@ -108,7 +108,7 @@ sql: | create table t (v1 int, v2 int); insert into t (v1, v2, v2) values (5, 6); - binder_error: 'Bind error: INSERT has more target columns than values' + binder_error: 'Bind error: Column specified more than once' - name: Not enough target columns sql: | create table t (v1 int, v2 int); diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index e29106429b35..15aae498a651 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -89,7 +89,7 @@ impl Binder { pub(super) fn bind_insert( &mut self, name: ObjectName, - columns: Vec, + target_col_idents: Vec, source: Query, returning_items: Vec, ) -> Result { @@ -103,8 +103,8 @@ impl Binder { let columns_to_insert = table_catalog.columns_to_insert().cloned().collect_vec(); let generated_column_names: HashSet<_> = table_catalog.generated_column_names().collect(); - for query_col in &columns { - let query_col_name = query_col.real_value(); + for target_col_ident in &target_col_idents { + let query_col_name = target_col_ident.real_value(); if generated_column_names.contains(query_col_name.as_str()) { return Err(RwError::from(ErrorCode::BindError(format!( "cannot insert a non-DEFAULT value into column \"{0}\". Column \"{0}\" is a generated column.", @@ -133,18 +133,14 @@ impl Binder { let (returning_list, fields) = self.bind_returning_list(returning_items)?; let is_returning = !returning_list.is_empty(); - // create table t (a int, b real, c varchar) - // case 1: insert into t (target_columns) values (values) - // case 2: insert into t values (values) - let has_target_col = columns.len() > 0; - if has_target_col { + if !target_col_idents.is_empty() { let mut target_table_col_indices: Vec = vec![]; let mut cols_to_insert_name_idx_map: HashMap = HashMap::new(); for (col_idx, col) in columns_to_insert.iter().enumerate() { cols_to_insert_name_idx_map.insert(col.name().to_string(), col_idx); } - for target_col in &columns { - let target_col_name = &target_col.real_value(); + for target_col_ident in &target_col_idents { + let target_col_name = &target_col_ident.real_value(); match cols_to_insert_name_idx_map.get_mut(target_col_name) { Some(value_ref) => { if *value_ref == usize::MAX { @@ -153,7 +149,7 @@ impl Binder { ))); } target_table_col_indices.push(*value_ref); - *value_ref = usize::MAX; + *value_ref = usize::MAX; // mark this column name, for duplicate detection } None => { // Invalid column name found @@ -215,7 +211,7 @@ impl Binder { fetch: None, } if order.is_empty() => { assert!(!values.0.is_empty()); - let err_msg = match columns.len().cmp(&values.0[0].len()) { + let err_msg = match target_col_idents.len().cmp(&values.0[0].len()) { std::cmp::Ordering::Equal => None, // e.g. create table t (a int, b real) // insert into t (v1, v2) values (7) @@ -233,7 +229,7 @@ impl Binder { return Err(RwError::from(ErrorCode::BindError(msg.to_string()))); } - let values = self.bind_values(values, Some(expected_types.clone()))?; + let values = self.bind_values(values, Some(expected_types))?; let body = BoundSetExpr::Values(values.into()); ( BoundQuery { @@ -280,7 +276,7 @@ impl Binder { None }, }; - return Ok(insert); + Ok(insert) } else { let expected_types: Vec = columns_to_insert .iter() @@ -313,7 +309,7 @@ impl Binder { return Err(RwError::from(ErrorCode::BindError(msg.to_string()))); } - let values = self.bind_values(values, Some(expected_types.clone()))?; + let values = self.bind_values(values, Some(expected_types))?; let body = BoundSetExpr::Values(values.into()); ( BoundQuery { @@ -360,7 +356,7 @@ impl Binder { None }, }; - return Ok(insert); + Ok(insert) } } From 886b4226ec23b8bba63bee5593eee599b0669148 Mon Sep 17 00:00:00 2001 From: broccoliSpicy Date: Tue, 4 Apr 2023 21:36:37 +0800 Subject: [PATCH 3/9] condense some code --- src/frontend/src/binder/insert.rs | 60 ++++++++++++------------------- 1 file changed, 23 insertions(+), 37 deletions(-) diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index 15aae498a651..a85d45567823 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -132,9 +132,11 @@ impl Binder { let (returning_list, fields) = self.bind_returning_list(returning_items)?; let is_returning = !returning_list.is_empty(); + let mut target_table_col_indices: Vec = vec![]; + let bound_query; + let cast_exprs; if !target_col_idents.is_empty() { - let mut target_table_col_indices: Vec = vec![]; let mut cols_to_insert_name_idx_map: HashMap = HashMap::new(); for (col_idx, col) in columns_to_insert.iter().enumerate() { cols_to_insert_name_idx_map.insert(col.name().to_string(), col_idx); @@ -201,7 +203,7 @@ impl Binder { // internal implicit cast. // In other cases, the `source` query is handled on its own and assignment cast is done // afterwards. - let (source, cast_exprs) = match source { + (bound_query, cast_exprs) = match source { Query { with: None, body: SetExpr::Values(values), @@ -260,29 +262,13 @@ impl Binder { (bound, cast_exprs) } }; - let insert = BoundInsert { - table_id, - table_version_id, - table_name, - owner, - row_id_index, - column_indices: target_table_col_indices, - source, - cast_exprs, - returning_list, - returning_schema: if is_returning { - Some(Schema { fields }) - } else { - None - }, - }; - Ok(insert) } else { let expected_types: Vec = columns_to_insert .iter() .map(|c| c.data_type().clone()) .collect(); - let (source, cast_exprs) = match source { + target_table_col_indices = (0..columns_to_insert.len()).collect(); + (bound_query, cast_exprs) = match source { Query { with: None, body: SetExpr::Values(values), @@ -340,24 +326,24 @@ impl Binder { (bound, cast_exprs) } }; - let insert = BoundInsert { - table_id, - table_version_id, - table_name, - owner, - row_id_index, - column_indices: (0..columns_to_insert.len()).collect::>(), - source, - cast_exprs, - returning_list, - returning_schema: if is_returning { - Some(Schema { fields }) - } else { - None - }, - }; - Ok(insert) } + let insert = BoundInsert { + table_id, + table_version_id, + table_name, + owner, + row_id_index, + column_indices: target_table_col_indices, + source: bound_query, + cast_exprs, + returning_list, + returning_schema: if is_returning { + Some(Schema { fields }) + } else { + None + }, + }; + Ok(insert) } /// Cast a list of `exprs` to corresponding `expected_types` IN ASSIGNMENT CONTEXT. Make sure From a736a9bdc4d7435ed9795c4c8328041b85f3c7d5 Mon Sep 17 00:00:00 2001 From: broccoliSpicy <93440049+broccoliSpicy@users.noreply.github.com> Date: Tue, 4 Apr 2023 21:39:42 +0800 Subject: [PATCH 4/9] Update src/frontend/src/binder/values.rs Co-authored-by: xxchan --- src/frontend/src/binder/values.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/frontend/src/binder/values.rs b/src/frontend/src/binder/values.rs index 00e85f336bb1..1cd164dae997 100644 --- a/src/frontend/src/binder/values.rs +++ b/src/frontend/src/binder/values.rs @@ -84,6 +84,7 @@ fn values_column_name(values_id: usize, col_id: usize) -> String { impl Binder { /// Bind [`Values`] with given `expected_types`. If no types are expected, a compatible type for /// all rows will be used. + /// If values are shorter than expected, `NULL`s will be filled. pub(super) fn bind_values( &mut self, values: Values, From 41ef25d7ec0314cea3a0c053307d5cd19b349f06 Mon Sep 17 00:00:00 2001 From: xxchan Date: Tue, 4 Apr 2023 16:19:02 +0200 Subject: [PATCH 5/9] extract logic for simple values --- src/frontend/src/binder/insert.rs | 60 +++++++------------------------ src/frontend/src/binder/query.rs | 13 +++++++ src/sqlparser/src/ast/query.rs | 17 +++++++++ 3 files changed, 42 insertions(+), 48 deletions(-) diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index e702bd6d1543..7dcffb29c301 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -203,15 +203,8 @@ impl Binder { // internal implicit cast. // In other cases, the `source` query is handled on its own and assignment cast is done // afterwards. - (bound_query, cast_exprs) = match source { - Query { - with: None, - body: SetExpr::Values(values), - order_by: order, - limit: None, - offset: None, - fetch: None, - } if order.is_empty() => { + (bound_query, cast_exprs) = match source.as_simple_values() { + Some(values) => { assert!(!values.0.is_empty()); let err_msg = match target_col_idents.len().cmp(&values.0[0].len()) { std::cmp::Ordering::Equal => None, @@ -231,22 +224,11 @@ impl Binder { return Err(RwError::from(ErrorCode::BindError(msg.to_string()))); } - let values = self.bind_values(values, Some(expected_types))?; - let body = BoundSetExpr::Values(values.into()); - ( - BoundQuery { - body, - order: vec![], - limit: None, - offset: None, - with_ties: false, - extra_order_exprs: vec![], - }, - vec![], - ) + let values = self.bind_values(values.clone(), Some(expected_types))?; + (BoundQuery::with_values(values), vec![]) } - query => { - let bound = self.bind_query(query)?; + None => { + let bound = self.bind_query(source)?; let actual_types = bound.data_types(); let cast_exprs = match expected_types == actual_types { true => vec![], @@ -268,15 +250,8 @@ impl Binder { .map(|c| c.data_type().clone()) .collect(); target_table_col_indices = (0..columns_to_insert.len()).collect(); - (bound_query, cast_exprs) = match source { - Query { - with: None, - body: SetExpr::Values(values), - order_by: order, - limit: None, - offset: None, - fetch: None, - } if order.is_empty() => { + (bound_query, cast_exprs) = match source.as_simple_values() { + Some(values) => { assert!(!values.0.is_empty()); let err_msg = match columns_to_insert.len().cmp(&values.0[0].len()) { std::cmp::Ordering::Equal => None, @@ -295,22 +270,11 @@ impl Binder { return Err(RwError::from(ErrorCode::BindError(msg.to_string()))); } - let values = self.bind_values(values, Some(expected_types))?; - let body = BoundSetExpr::Values(values.into()); - ( - BoundQuery { - body, - order: vec![], - limit: None, - offset: None, - with_ties: false, - extra_order_exprs: vec![], - }, - vec![], - ) + let values = self.bind_values(values.clone(), Some(expected_types))?; + (BoundQuery::with_values(values), vec![]) } - query => { - let bound = self.bind_query(query)?; + None => { + let bound = self.bind_query(source)?; let actual_types = bound.data_types(); let cast_exprs = match expected_types == actual_types { true => vec![], diff --git a/src/frontend/src/binder/query.rs b/src/frontend/src/binder/query.rs index 3cac14efd1cd..9381ecdd7422 100644 --- a/src/frontend/src/binder/query.rs +++ b/src/frontend/src/binder/query.rs @@ -22,6 +22,7 @@ use risingwave_common::util::sort_util::{ColumnOrder, OrderType}; use risingwave_sqlparser::ast::{Cte, Expr, Fetch, OrderByExpr, Query, Value, With}; use super::statement::RewriteExprsRecursive; +use super::BoundValues; use crate::binder::{Binder, BoundSetExpr}; use crate::expr::{CorrelatedId, Depth, ExprImpl, ExprRewriter}; @@ -95,6 +96,18 @@ impl BoundQuery { self.body .collect_correlated_indices_by_depth_and_assign_id(depth, correlated_id) } + + /// Simple `VALUES` without other clauses. + pub fn with_values(values: BoundValues) -> Self { + BoundQuery { + body: BoundSetExpr::Values(values.into()), + order: vec![], + limit: None, + offset: None, + with_ties: false, + extra_order_exprs: vec![], + } + } } impl RewriteExprsRecursive for BoundQuery { diff --git a/src/sqlparser/src/ast/query.rs b/src/sqlparser/src/ast/query.rs index 31b7074b6e67..c6224434cff6 100644 --- a/src/sqlparser/src/ast/query.rs +++ b/src/sqlparser/src/ast/query.rs @@ -43,6 +43,23 @@ pub struct Query { pub fetch: Option, } +impl Query { + /// Simple `VALUES` without other clauses. + pub fn as_simple_values(&self) -> Option<&Values> { + match &self { + Query { + with: None, + body: SetExpr::Values(values), + order_by, + limit: None, + offset: None, + fetch: None, + } if order_by.is_empty() => Some(values), + _ => None, + } + } +} + impl fmt::Display for Query { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { if let Some(ref with) = self.with { From b07fd7aa63ca9d7f451b969614ee1851fbd2357e Mon Sep 17 00:00:00 2001 From: broccoliSpicy Date: Tue, 4 Apr 2023 23:40:11 +0800 Subject: [PATCH 6/9] fix lint --- src/frontend/src/binder/insert.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index 7dcffb29c301..41104fe51136 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -19,10 +19,10 @@ use risingwave_common::catalog::{Schema, TableVersionId}; use risingwave_common::error::{ErrorCode, Result, RwError}; use risingwave_common::types::DataType; use risingwave_common::util::iter_util::ZipEqFast; -use risingwave_sqlparser::ast::{Ident, ObjectName, Query, SelectItem, SetExpr}; +use risingwave_sqlparser::ast::{Ident, ObjectName, Query, SelectItem}; use super::statement::RewriteExprsRecursive; -use super::{BoundQuery, BoundSetExpr}; +use super::BoundQuery; use crate::binder::Binder; use crate::catalog::TableId; use crate::expr::{ExprImpl, InputRef}; From 1135f06a72aa5c97baba9798854098b76b319e78 Mon Sep 17 00:00:00 2001 From: xxchan Date: Tue, 4 Apr 2023 17:05:15 +0200 Subject: [PATCH 7/9] rename some varibles --- src/frontend/src/binder/insert.rs | 51 ++++++++++++++++--------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index 41104fe51136..f2d324c06c60 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -89,7 +89,7 @@ impl Binder { pub(super) fn bind_insert( &mut self, name: ObjectName, - target_col_idents: Vec, + cols_to_insert_by_user: Vec, source: Query, returning_items: Vec, ) -> Result { @@ -100,11 +100,11 @@ impl Binder { let table_id = table_catalog.id; let owner = table_catalog.owner; let table_version_id = table_catalog.version_id().expect("table must be versioned"); - let columns_to_insert = table_catalog.columns_to_insert().cloned().collect_vec(); + let cols_to_insert_in_table = table_catalog.columns_to_insert().cloned().collect_vec(); let generated_column_names: HashSet<_> = table_catalog.generated_column_names().collect(); - for target_col_ident in &target_col_idents { - let query_col_name = target_col_ident.real_value(); + for col in &cols_to_insert_by_user { + let query_col_name = col.real_value(); if generated_column_names.contains(query_col_name.as_str()) { return Err(RwError::from(ErrorCode::BindError(format!( "cannot insert a non-DEFAULT value into column \"{0}\". Column \"{0}\" is a generated column.", @@ -132,43 +132,44 @@ impl Binder { let (returning_list, fields) = self.bind_returning_list(returning_items)?; let is_returning = !returning_list.is_empty(); - let mut target_table_col_indices: Vec = vec![]; + let mut col_indices_to_insert: Vec = vec![]; let bound_query; let cast_exprs; - if !target_col_idents.is_empty() { - let mut cols_to_insert_name_idx_map: HashMap = HashMap::new(); - for (col_idx, col) in columns_to_insert.iter().enumerate() { - cols_to_insert_name_idx_map.insert(col.name().to_string(), col_idx); + if !cols_to_insert_by_user.is_empty() { + let mut col_name_to_idx: HashMap = HashMap::new(); + for (col_idx, col) in cols_to_insert_in_table.iter().enumerate() { + col_name_to_idx.insert(col.name().to_string(), col_idx); } - for target_col_ident in &target_col_idents { - let target_col_name = &target_col_ident.real_value(); - match cols_to_insert_name_idx_map.get_mut(target_col_name) { + + for col_name in &cols_to_insert_by_user { + let col_name = &col_name.real_value(); + match col_name_to_idx.get_mut(col_name) { Some(value_ref) => { if *value_ref == usize::MAX { return Err(RwError::from(ErrorCode::BindError( "Column specified more than once".to_string(), ))); } - target_table_col_indices.push(*value_ref); + col_indices_to_insert.push(*value_ref); *value_ref = usize::MAX; // mark this column name, for duplicate detection } None => { // Invalid column name found return Err(RwError::from(ErrorCode::BindError(format!( "Column {} not found in table {}", - target_col_name, table_name + col_name, table_name )))); } } } // columns that are in the target table but not in the provided target columns - if target_table_col_indices.len() != columns_to_insert.len() { - for col in &columns_to_insert { - if let Some(col_to_insert_idx) = cols_to_insert_name_idx_map.get(col.name()) { + if col_indices_to_insert.len() != cols_to_insert_in_table.len() { + for col in &cols_to_insert_in_table { + if let Some(col_to_insert_idx) = col_name_to_idx.get(col.name()) { if *col_to_insert_idx != usize::MAX { - target_table_col_indices.push(*col_to_insert_idx); + col_indices_to_insert.push(*col_to_insert_idx); } } else { unreachable!(); @@ -176,9 +177,9 @@ impl Binder { } } - let expected_types: Vec = target_table_col_indices + let expected_types: Vec = col_indices_to_insert .iter() - .map(|idx| columns_to_insert[*idx].data_type().clone()) + .map(|idx| cols_to_insert_in_table[*idx].data_type().clone()) .collect(); // When the column types of `source` query do not match `expected_types`, casting is @@ -206,7 +207,7 @@ impl Binder { (bound_query, cast_exprs) = match source.as_simple_values() { Some(values) => { assert!(!values.0.is_empty()); - let err_msg = match target_col_idents.len().cmp(&values.0[0].len()) { + let err_msg = match cols_to_insert_by_user.len().cmp(&values.0[0].len()) { std::cmp::Ordering::Equal => None, // e.g. create table t (a int, b real) // insert into t (v1, v2) values (7) @@ -245,15 +246,15 @@ impl Binder { } }; } else { - let expected_types: Vec = columns_to_insert + let expected_types: Vec = cols_to_insert_in_table .iter() .map(|c| c.data_type().clone()) .collect(); - target_table_col_indices = (0..columns_to_insert.len()).collect(); + col_indices_to_insert = (0..cols_to_insert_in_table.len()).collect(); (bound_query, cast_exprs) = match source.as_simple_values() { Some(values) => { assert!(!values.0.is_empty()); - let err_msg = match columns_to_insert.len().cmp(&values.0[0].len()) { + let err_msg = match cols_to_insert_in_table.len().cmp(&values.0[0].len()) { std::cmp::Ordering::Equal => None, // e.g. create table t (a int, b real) // insert into t values (7) @@ -297,7 +298,7 @@ impl Binder { table_name, owner, row_id_index, - column_indices: target_table_col_indices, + column_indices: col_indices_to_insert, source: bound_query, cast_exprs, returning_list, From 6e0d9a053b3d1c34e5899c0709316d097ee24946 Mon Sep 17 00:00:00 2001 From: xxchan Date: Tue, 4 Apr 2023 18:29:55 +0200 Subject: [PATCH 8/9] refactor --- .../planner_test/tests/testdata/insert.yaml | 6 +- src/frontend/src/binder/insert.rs | 296 +++++++++--------- 2 files changed, 150 insertions(+), 152 deletions(-) diff --git a/src/frontend/planner_test/tests/testdata/insert.yaml b/src/frontend/planner_test/tests/testdata/insert.yaml index 866601f9b980..9cacf3bf43ca 100644 --- a/src/frontend/planner_test/tests/testdata/insert.yaml +++ b/src/frontend/planner_test/tests/testdata/insert.yaml @@ -107,13 +107,13 @@ - name: To many target columns sql: | create table t (v1 int, v2 int); - insert into t (v1, v2, v2) values (5, 6); - binder_error: 'Bind error: Column specified more than once' + insert into t (v1, v2) values (5); + binder_error: 'Bind error: INSERT has more target columns than expressions' - name: Not enough target columns sql: | create table t (v1 int, v2 int); insert into t (v1) values (5, 6); - binder_error: 'Bind error: INSERT has less target columns than values' + binder_error: 'Bind error: INSERT has more expressions than target columns' - name: insert literal null sql: | create table t(v1 int); diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index f2d324c06c60..1fdf2aba0d2e 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -15,7 +15,7 @@ use std::collections::{HashMap, HashSet}; use itertools::Itertools; -use risingwave_common::catalog::{Schema, TableVersionId}; +use risingwave_common::catalog::{ColumnCatalog, Schema, TableVersionId}; use risingwave_common::error::{ErrorCode, Result, RwError}; use risingwave_common::types::DataType; use risingwave_common::util::iter_util::ZipEqFast; @@ -132,166 +132,104 @@ impl Binder { let (returning_list, fields) = self.bind_returning_list(returning_items)?; let is_returning = !returning_list.is_empty(); - let mut col_indices_to_insert: Vec = vec![]; + + let col_indices_to_insert = get_col_indices_to_insert( + &cols_to_insert_in_table, + &cols_to_insert_by_user, + &table_name, + )?; + let expected_types: Vec = col_indices_to_insert + .iter() + .map(|idx| cols_to_insert_in_table[*idx].data_type().clone()) + .collect(); + + // When the column types of `source` query do not match `expected_types`, + // casting is needed. + // + // In PG, when the `source` is a `VALUES` without order / limit / offset, + // special treatment is given and it is NOT equivalent to + // assignment cast over potential implicit cast inside. For + // example, the following is valid: + // + // ``` + // create table t (v1 time); + // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); + // ``` + // + // But the followings are not: + // + // ``` + // values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); + // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05') + // limit 1; + // ``` + // + // Because `timestamp` can cast to `time` in assignment context, but no casting + // between them is allowed implicitly. + // + // In this case, assignment cast should be used directly in `VALUES`, + // suppressing its internal implicit cast. + // In other cases, the `source` query is handled on its own and assignment cast + // is done afterwards. let bound_query; let cast_exprs; - if !cols_to_insert_by_user.is_empty() { - let mut col_name_to_idx: HashMap = HashMap::new(); - for (col_idx, col) in cols_to_insert_in_table.iter().enumerate() { - col_name_to_idx.insert(col.name().to_string(), col_idx); + match source.as_simple_values() { + None => { + bound_query = self.bind_query(source)?; + let actual_types = bound_query.data_types(); + cast_exprs = match expected_types == actual_types { + true => vec![], + false => Self::cast_on_insert( + &expected_types, + actual_types + .into_iter() + .enumerate() + .map(|(i, t)| InputRef::new(i, t).into()) + .collect(), + )?, + }; } - - for col_name in &cols_to_insert_by_user { - let col_name = &col_name.real_value(); - match col_name_to_idx.get_mut(col_name) { - Some(value_ref) => { - if *value_ref == usize::MAX { - return Err(RwError::from(ErrorCode::BindError( - "Column specified more than once".to_string(), - ))); + Some(values) => { + assert!(!values.0.is_empty()); + let num_value_cols = values.0[0].len(); + let has_user_specified_columns = !cols_to_insert_by_user.is_empty(); + let num_target_cols = if has_user_specified_columns { + cols_to_insert_by_user.len() + } else { + cols_to_insert_in_table.len() + }; + let err_msg = match num_target_cols.cmp(&num_value_cols) { + std::cmp::Ordering::Equal => None, + std::cmp::Ordering::Greater => { + if has_user_specified_columns { + // e.g. insert into t (v1, v2) values (7) + Some("INSERT has more target columns than expressions") + } else { + // e.g. create table t (a int, b real) + // insert into t values (7) + // this kind of usage is fine, null values will be provided + // implicitly. + None } - col_indices_to_insert.push(*value_ref); - *value_ref = usize::MAX; // mark this column name, for duplicate detection } - None => { - // Invalid column name found - return Err(RwError::from(ErrorCode::BindError(format!( - "Column {} not found in table {}", - col_name, table_name - )))); - } - } - } - - // columns that are in the target table but not in the provided target columns - if col_indices_to_insert.len() != cols_to_insert_in_table.len() { - for col in &cols_to_insert_in_table { - if let Some(col_to_insert_idx) = col_name_to_idx.get(col.name()) { - if *col_to_insert_idx != usize::MAX { - col_indices_to_insert.push(*col_to_insert_idx); - } - } else { - unreachable!(); - } - } - } - - let expected_types: Vec = col_indices_to_insert - .iter() - .map(|idx| cols_to_insert_in_table[*idx].data_type().clone()) - .collect(); - - // When the column types of `source` query do not match `expected_types`, casting is - // needed. - // - // In PG, when the `source` is a `VALUES` without order / limit / offset, special - // treatment is given and it is NOT equivalent to assignment cast over - // potential implicit cast inside. For example, the following is valid: - // ``` - // create table t (v1 time); - // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); - // ``` - // But the followings are not: - // ``` - // values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); - // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05') limit 1; - // ``` - // Because `timestamp` can cast to `time` in assignment context, but no casting between - // them is allowed implicitly. - // - // In this case, assignment cast should be used directly in `VALUES`, suppressing its - // internal implicit cast. - // In other cases, the `source` query is handled on its own and assignment cast is done - // afterwards. - (bound_query, cast_exprs) = match source.as_simple_values() { - Some(values) => { - assert!(!values.0.is_empty()); - let err_msg = match cols_to_insert_by_user.len().cmp(&values.0[0].len()) { - std::cmp::Ordering::Equal => None, - // e.g. create table t (a int, b real) - // insert into t (v1, v2) values (7) - std::cmp::Ordering::Greater => { - Some("INSERT has more target columns than values") - } + std::cmp::Ordering::Less => { // e.g. create table t (a int, b real) // insert into t (v1) values (7, 13) - std::cmp::Ordering::Less => { - Some("INSERT has less target columns than values") - } - }; - - if let Some(msg) = err_msg { - return Err(RwError::from(ErrorCode::BindError(msg.to_string()))); + // or insert into t values (7, 13, 17) + Some("INSERT has more expressions than target columns") } - - let values = self.bind_values(values.clone(), Some(expected_types))?; - (BoundQuery::with_values(values), vec![]) - } - None => { - let bound = self.bind_query(source)?; - let actual_types = bound.data_types(); - let cast_exprs = match expected_types == actual_types { - true => vec![], - false => Self::cast_on_insert( - &expected_types, - actual_types - .into_iter() - .enumerate() - .map(|(i, t)| InputRef::new(i, t).into()) - .collect(), - )?, - }; - (bound, cast_exprs) + }; + if let Some(msg) = err_msg { + return Err(RwError::from(ErrorCode::BindError(msg.to_string()))); } - }; - } else { - let expected_types: Vec = cols_to_insert_in_table - .iter() - .map(|c| c.data_type().clone()) - .collect(); - col_indices_to_insert = (0..cols_to_insert_in_table.len()).collect(); - (bound_query, cast_exprs) = match source.as_simple_values() { - Some(values) => { - assert!(!values.0.is_empty()); - let err_msg = match cols_to_insert_in_table.len().cmp(&values.0[0].len()) { - std::cmp::Ordering::Equal => None, - // e.g. create table t (a int, b real) - // insert into t values (7) - // this kind of usage is fine, null values will be provided implicitly. - std::cmp::Ordering::Greater => None, - // e.g. create table t (a int, b real) - // insert into t values (7, 13, 17) - std::cmp::Ordering::Less => { - Some("INSERT has more expressions than target columns") - } - }; - if let Some(msg) = err_msg { - return Err(RwError::from(ErrorCode::BindError(msg.to_string()))); - } - - let values = self.bind_values(values.clone(), Some(expected_types))?; - (BoundQuery::with_values(values), vec![]) - } - None => { - let bound = self.bind_query(source)?; - let actual_types = bound.data_types(); - let cast_exprs = match expected_types == actual_types { - true => vec![], - false => Self::cast_on_insert( - &expected_types, - actual_types - .into_iter() - .enumerate() - .map(|(i, t)| InputRef::new(i, t).into()) - .collect(), - )?, - }; - (bound, cast_exprs) - } - }; + let values = self.bind_values(values.clone(), Some(expected_types))?; + bound_query = BoundQuery::with_values(values); + cast_exprs = vec![]; + } } + let insert = BoundInsert { table_id, table_version_id, @@ -331,3 +269,63 @@ impl Binder { Err(ErrorCode::BindError(msg.into()).into()) } } + +/// Returned indices have the same length as `cols_to_insert_in_table`. +/// The first elements have the same order as `cols_to_insert_by_user`. +/// The rest are what's not specified by the user. +/// +/// Also checks there are no duplicate nor unknown columns provided by the user. +fn get_col_indices_to_insert( + cols_to_insert_in_table: &[ColumnCatalog], + cols_to_insert_by_user: &[Ident], + table_name: &str, +) -> Result> { + if cols_to_insert_by_user.is_empty() { + return Ok((0..cols_to_insert_in_table.len()).collect()); + } + + let mut col_indices_to_insert: Vec = Vec::new(); + + let mut col_name_to_idx: HashMap = HashMap::new(); + for (col_idx, col) in cols_to_insert_in_table.iter().enumerate() { + col_name_to_idx.insert(col.name().to_string(), col_idx); + } + + for col_name in cols_to_insert_by_user { + let col_name = &col_name.real_value(); + match col_name_to_idx.get_mut(col_name) { + Some(value_ref) => { + if *value_ref == usize::MAX { + return Err(RwError::from(ErrorCode::BindError( + "Column specified more than once".to_string(), + ))); + } + col_indices_to_insert.push(*value_ref); + *value_ref = usize::MAX; // mark this column name, for duplicate + // detection + } + None => { + // Invalid column name found + return Err(RwError::from(ErrorCode::BindError(format!( + "Column {} not found in table {}", + col_name, table_name + )))); + } + } + } + + // columns that are in the target table but not in the provided target columns + if col_indices_to_insert.len() != cols_to_insert_in_table.len() { + for col in cols_to_insert_in_table { + if let Some(col_to_insert_idx) = col_name_to_idx.get(col.name()) { + if *col_to_insert_idx != usize::MAX { + col_indices_to_insert.push(*col_to_insert_idx); + } + } else { + unreachable!(); + } + } + } + + Ok(col_indices_to_insert) +} From 6f955f6982793a94f8cc3c155acbd3008def333e Mon Sep 17 00:00:00 2001 From: xxchan Date: Tue, 4 Apr 2023 18:32:12 +0200 Subject: [PATCH 9/9] revert some comment alignment changes --- src/frontend/src/binder/insert.rs | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/frontend/src/binder/insert.rs b/src/frontend/src/binder/insert.rs index 1fdf2aba0d2e..0c38d61e13ff 100644 --- a/src/frontend/src/binder/insert.rs +++ b/src/frontend/src/binder/insert.rs @@ -146,10 +146,9 @@ impl Binder { // When the column types of `source` query do not match `expected_types`, // casting is needed. // - // In PG, when the `source` is a `VALUES` without order / limit / offset, - // special treatment is given and it is NOT equivalent to - // assignment cast over potential implicit cast inside. For - // example, the following is valid: + // In PG, when the `source` is a `VALUES` without order / limit / offset, special treatment + // is given and it is NOT equivalent to assignment cast over potential implicit cast inside. + // For example, the following is valid: // // ``` // create table t (v1 time); @@ -160,17 +159,16 @@ impl Binder { // // ``` // values (timestamp '2020-01-01 01:02:03'), (time '03:04:05'); - // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05') - // limit 1; + // insert into t values (timestamp '2020-01-01 01:02:03'), (time '03:04:05') limit 1; // ``` // - // Because `timestamp` can cast to `time` in assignment context, but no casting - // between them is allowed implicitly. + // Because `timestamp` can cast to `time` in assignment context, but no casting between them + // is allowed implicitly. // - // In this case, assignment cast should be used directly in `VALUES`, - // suppressing its internal implicit cast. - // In other cases, the `source` query is handled on its own and assignment cast - // is done afterwards. + // In this case, assignment cast should be used directly in `VALUES`, suppressing its + // internal implicit cast. + // In other cases, the `source` query is handled on its own and assignment cast is done + // afterwards. let bound_query; let cast_exprs;