Skip to content

Commit 1cf3d94

Browse files
b41shTCeason
b41sh
and
TCeason
authored
fix(query): column default expr should not cause seq.nextval modify (#18694)
* fix(query): column default expr should not cause seq.nextval modify * do not generate sequence next value as default value * fix --------- Co-authored-by: TCeason <[email protected]>
1 parent 613a23a commit 1cf3d94

File tree

12 files changed

+290
-124
lines changed

12 files changed

+290
-124
lines changed

โ€Žsrc/query/service/src/interpreters/interpreter_table_add_column.rsโ€Ž

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,13 @@ impl Interpreter for AddTableColumnInterpreter {
104104
.check_enterprise_enabled(self.ctx.get_license_key(), ComputedColumn)?;
105105
}
106106

107+
let num_rows = table_info.meta.statistics.number_of_rows;
108+
if self.plan.is_nextval && num_rows > 0 {
109+
return Err(ErrorCode::AlterTableError(format!(
110+
"Cannot add column '{}' with `nextval` as default value to non-empty table '{}'",
111+
&self.plan.field.name, &self.plan.table
112+
)));
113+
}
107114
if field.default_expr().is_some() {
108115
let _ = DefaultExprBinder::try_new(self.ctx.clone())?.get_scalar(&field)?;
109116
}
@@ -117,9 +124,9 @@ impl Interpreter for AddTableColumnInterpreter {
117124
.meta
118125
.add_column(&field, &self.plan.comment, index)?;
119126

120-
// if the new column is a stored computed field,
127+
// if the new column is a stored computed field and table is non-empty,
121128
// need rebuild the table to generate stored computed column.
122-
if let Some(ComputedExpr::Stored(_)) = field.computed_expr {
129+
if num_rows > 0 && matches!(field.computed_expr, Some(ComputedExpr::Stored(_))) {
123130
let fuse_table = FuseTable::try_from_table(tbl.as_ref())?;
124131
let base_snapshot = fuse_table.read_table_snapshot().await?;
125132
let prev_snapshot_id = base_snapshot.snapshot_id().map(|(id, _)| id);
@@ -162,8 +169,9 @@ impl Interpreter for AddTableColumnInterpreter {
162169
)
163170
.await?;
164171

165-
// If the column is not deterministic, update to refresh the value with default expr.
166-
if !self.plan.is_deterministic {
172+
// If the column is not deterministic and table is non-empty,
173+
// update to refresh the value with default expr.
174+
if num_rows > 0 && !self.plan.is_deterministic {
167175
self.ctx
168176
.evict_table_from_cache(catalog_name, db_name, tbl_name)?;
169177
let query = format!(

โ€Žsrc/query/service/src/interpreters/interpreter_table_create.rsโ€Ž

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -429,9 +429,10 @@ impl CreateTableInterpreter {
429429
/// - Update cluster key of table meta.
430430
fn build_request(&self, statistics: Option<TableStatistics>) -> Result<CreateTableReq> {
431431
let fields = self.plan.schema.fields().clone();
432+
let mut default_expr_binder = DefaultExprBinder::try_new(self.ctx.clone())?;
432433
for field in fields.iter() {
433434
if field.default_expr().is_some() {
434-
let _ = DefaultExprBinder::try_new(self.ctx.clone())?.get_scalar(field)?;
435+
let _ = default_expr_binder.get_scalar(field)?;
435436
}
436437
is_valid_column(field.name())?;
437438
}

โ€Žsrc/query/service/tests/it/storages/fuse/operations/alter_table.rsโ€Ž

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,7 @@ async fn test_fuse_table_optimize_alter_table() -> Result<()> {
185185
comment: "".to_string(),
186186
option: AddColumnOption::End,
187187
is_deterministic: true,
188+
is_nextval: false,
188189
};
189190
let interpreter = AddTableColumnInterpreter::try_create(ctx.clone(), add_table_column_plan)?;
190191
let _ = interpreter.execute(ctx.clone()).await?;

โ€Žsrc/query/sql/src/planner/binder/ddl/table.rsโ€Ž

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1091,7 +1091,7 @@ impl Binder {
10911091
.get_table(&catalog, &database, &table)
10921092
.await?
10931093
.schema();
1094-
let (field, comment, is_deterministic) =
1094+
let (field, comment, is_deterministic, is_nextval) =
10951095
self.analyze_add_column(column, schema).await?;
10961096
let option = match ast_option {
10971097
AstAddColumnOption::First => AddColumnOption::First,
@@ -1109,6 +1109,7 @@ impl Binder {
11091109
comment,
11101110
option,
11111111
is_deterministic,
1112+
is_nextval,
11121113
})))
11131114
}
11141115
AlterTableAction::AddConstraint { constraint } => {
@@ -1179,7 +1180,7 @@ impl Binder {
11791180
.await?
11801181
.schema();
11811182
for column in column_def_vec {
1182-
let (field, comment, _) =
1183+
let (field, comment, _, _) =
11831184
self.analyze_add_column(column, schema.clone()).await?;
11841185
field_and_comment.push((field, comment));
11851186
}
@@ -1656,20 +1657,22 @@ impl Binder {
16561657
&self,
16571658
column: &ColumnDefinition,
16581659
table_schema: TableSchemaRef,
1659-
) -> Result<(TableField, String, bool)> {
1660+
) -> Result<(TableField, String, bool, bool)> {
16601661
let name = normalize_identifier(&column.name, &self.name_resolution_ctx).name;
16611662
let not_null = self.is_column_not_null();
16621663
let data_type = resolve_type_name(&column.data_type, not_null)?;
16631664
let mut is_deterministic = true;
1665+
let mut is_nextval = false;
16641666
let mut field = TableField::new(&name, data_type);
16651667
if let Some(expr) = &column.expr {
16661668
match expr {
16671669
ColumnExpr::Default(default_expr) => {
16681670
let mut default_expr_binder = DefaultExprBinder::try_new(self.ctx.clone())?;
1669-
let (expr, expr_is_deterministic) =
1671+
let (expr, expr_is_deterministic, expr_is_nextval) =
16701672
default_expr_binder.parse_default_expr_to_string(&field, default_expr)?;
16711673
field = field.with_default_expr(Some(expr));
16721674
is_deterministic = expr_is_deterministic;
1675+
is_nextval = expr_is_nextval;
16731676
}
16741677
ColumnExpr::Virtual(virtual_expr) => {
16751678
let expr = parse_computed_expr_to_string(
@@ -1693,7 +1696,7 @@ impl Binder {
16931696
}
16941697
}
16951698
let comment = column.comment.clone().unwrap_or_default();
1696-
Ok((field, comment, is_deterministic))
1699+
Ok((field, comment, is_deterministic, is_nextval))
16971700
}
16981701

16991702
#[async_backtrace::framed]
@@ -1714,7 +1717,7 @@ impl Binder {
17141717
if let Some(expr) = &column.expr {
17151718
match expr {
17161719
ColumnExpr::Default(default_expr) => {
1717-
let (expr, _) = default_expr_binder
1720+
let (expr, _, _) = default_expr_binder
17181721
.parse_default_expr_to_string(&field, default_expr)?;
17191722
field = field.with_default_expr(Some(expr));
17201723
}

โ€Žsrc/query/sql/src/planner/binder/default_expr.rsโ€Ž

Lines changed: 62 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,12 @@ use databend_common_expression::TableField;
3636
use databend_common_functions::BUILTIN_FUNCTIONS;
3737
use parking_lot::RwLock;
3838

39-
use crate::binder::expr_values::ExprValuesRewriter;
4039
use crate::binder::wrap_cast;
4140
use crate::binder::AsyncFunctionDesc;
4241
use crate::planner::binder::BindContext;
4342
use crate::planner::semantic::NameResolutionContext;
4443
use crate::planner::semantic::TypeChecker;
44+
use crate::plans::walk_expr_mut;
4545
use crate::plans::AsyncFunctionArgument;
4646
use crate::plans::AsyncFunctionCall;
4747
use crate::plans::ConstantExpr;
@@ -58,7 +58,7 @@ pub struct DefaultExprBinder {
5858
name_resolution_ctx: NameResolutionContext,
5959
metadata: MetadataRef,
6060

61-
rewriter: ExprValuesRewriter,
61+
rewriter: DefaultValueRewriter,
6262
dummy_block: DataBlock,
6363
func_ctx: FunctionContext,
6464
}
@@ -87,7 +87,7 @@ impl DefaultExprBinder {
8787

8888
let dummy_block = DataBlock::new(vec![], 1);
8989
let func_ctx = ctx.get_function_context()?;
90-
let rewriter = ExprValuesRewriter::new(ctx.clone());
90+
let rewriter = DefaultValueRewriter::new();
9191

9292
Ok(DefaultExprBinder {
9393
bind_context,
@@ -109,16 +109,36 @@ impl DefaultExprBinder {
109109
&mut self,
110110
field: &TableField,
111111
ast: &AExpr,
112-
) -> Result<(String, bool)> {
112+
) -> Result<(String, bool, bool)> {
113113
let data_field: DataField = field.into();
114-
let scalar_expr = self.bind(ast, data_field.data_type())?;
115-
let is_nextval = get_nextval(&scalar_expr).is_some();
116-
if !scalar_expr.evaluable() && !is_nextval {
114+
let (scalar_expr, data_type) = self.bind(ast)?;
115+
let (evaluable, is_nextval) = scalar_expr.default_value_evaluable();
116+
// The nextval can not work with other expressions.
117+
if !evaluable || (is_nextval && !matches!(scalar_expr, ScalarExpr::AsyncFunctionCall(_))) {
117118
return Err(ErrorCode::SemanticError(format!(
118119
"default value expression `{:#}` is invalid",
119120
ast
120121
)));
121122
}
123+
let dest_type = data_field.data_type();
124+
// The data type of nextval must be number or decimal.
125+
if is_nextval
126+
&& !matches!(
127+
dest_type.remove_nullable(),
128+
DataType::Number(_) | DataType::Decimal(_)
129+
)
130+
{
131+
return Err(ErrorCode::SemanticError(format!(
132+
"default data type does not match data type for column {}",
133+
data_field.name()
134+
)));
135+
}
136+
137+
let scalar_expr = if data_type != *dest_type {
138+
wrap_cast(&scalar_expr, dest_type)
139+
} else {
140+
scalar_expr
141+
};
122142
let expr = scalar_expr.as_expr()?;
123143
let (expr, is_deterministic) = if is_nextval {
124144
(expr, false)
@@ -129,7 +149,7 @@ impl DefaultExprBinder {
129149
} else {
130150
(expr, false)
131151
};
132-
Ok((expr.sql_display(), is_deterministic))
152+
Ok((expr.sql_display(), is_deterministic, is_nextval))
133153
}
134154

135155
pub fn parse(&mut self, default_expr: &str) -> Result<AExpr> {
@@ -138,7 +158,7 @@ impl DefaultExprBinder {
138158
Ok(ast)
139159
}
140160

141-
pub fn bind(&mut self, ast: &AExpr, dest_type: &DataType) -> Result<ScalarExpr> {
161+
pub fn bind(&mut self, ast: &AExpr) -> Result<(ScalarExpr, DataType)> {
142162
let mut type_checker = TypeChecker::try_create(
143163
&mut self.bind_context,
144164
self.ctx.clone(),
@@ -147,11 +167,8 @@ impl DefaultExprBinder {
147167
&[],
148168
true,
149169
)?;
150-
let (mut scalar, data_type) = *type_checker.resolve(ast)?;
151-
if &data_type != dest_type {
152-
scalar = wrap_cast(&scalar, dest_type);
153-
}
154-
Ok(scalar)
170+
let (scalar_expr, data_type) = *type_checker.resolve(ast)?;
171+
Ok((scalar_expr, data_type))
155172
}
156173

157174
pub fn parse_and_bind(&mut self, field: &DataField) -> Result<ScalarExpr> {
@@ -165,7 +182,13 @@ impl DefaultExprBinder {
165182
e
166183
)
167184
})?;
168-
self.bind(&ast, field.data_type())
185+
let (scalar_expr, data_type) = self.bind(&ast)?;
186+
let dest_type = field.data_type();
187+
if data_type != *dest_type {
188+
Ok(wrap_cast(&scalar_expr, dest_type))
189+
} else {
190+
Ok(scalar_expr)
191+
}
169192
} else {
170193
Ok(ScalarExpr::ConstantExpr(ConstantExpr {
171194
span: None,
@@ -281,3 +304,27 @@ impl DefaultExprBinder {
281304
}
282305
}
283306
}
307+
308+
struct DefaultValueRewriter {}
309+
310+
impl DefaultValueRewriter {
311+
pub fn new() -> Self {
312+
Self {}
313+
}
314+
}
315+
316+
impl<'a> VisitorMut<'a> for DefaultValueRewriter {
317+
fn visit(&mut self, expr: &'a mut ScalarExpr) -> Result<()> {
318+
if let ScalarExpr::AsyncFunctionCall(async_func) = &expr {
319+
if async_func.func_name == "nextval" {
320+
// Don't generate a new sequence next value,
321+
// because the default value is not used for new inserted values.
322+
*expr = ScalarExpr::ConstantExpr(ConstantExpr {
323+
span: async_func.span,
324+
value: Scalar::default_value(&async_func.return_type),
325+
});
326+
}
327+
}
328+
walk_expr_mut(self, expr)
329+
}
330+
}

โ€Žsrc/query/sql/src/planner/binder/expr_values.rsโ€Ž

Lines changed: 52 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@ use databend_common_expression::types::UInt8Type;
2222
use databend_common_expression::BlockEntry;
2323
use databend_common_expression::DataBlock;
2424
use databend_common_expression::DataSchemaRef;
25+
use databend_common_expression::FunctionKind;
2526
use databend_common_expression::Scalar;
27+
use databend_common_functions::BUILTIN_FUNCTIONS;
2628
use databend_common_pipeline_transforms::processors::Transform;
2729
use databend_common_users::Object;
2830

@@ -63,41 +65,57 @@ impl ExprValuesRewriter {
6365

6466
impl<'a> VisitorMut<'a> for ExprValuesRewriter {
6567
fn visit(&mut self, expr: &'a mut ScalarExpr) -> Result<()> {
66-
if let ScalarExpr::AsyncFunctionCall(async_func) = &expr {
67-
let tenant = self.ctx.get_tenant();
68-
let catalog = self.ctx.get_default_catalog()?;
69-
let visibility_checker = if self
70-
.ctx
71-
.get_settings()
72-
.get_enable_experimental_sequence_privilege_check()?
73-
{
74-
let ctx = self.ctx.clone();
75-
Some(databend_common_base::runtime::block_on(async move {
76-
ctx.get_visibility_checker(false, Object::Sequence).await
77-
})?)
78-
} else {
79-
None
80-
};
81-
let value = databend_common_base::runtime::block_on(async move {
82-
async_func
83-
.generate(tenant.clone(), catalog.clone(), visibility_checker)
84-
.await
85-
})?;
86-
87-
*expr = ScalarExpr::ConstantExpr(ConstantExpr {
88-
span: async_func.span,
89-
value,
90-
});
91-
}
92-
93-
if matches!(
94-
expr,
68+
match &expr {
69+
ScalarExpr::AsyncFunctionCall(async_func) => {
70+
let tenant = self.ctx.get_tenant();
71+
let catalog = self.ctx.get_default_catalog()?;
72+
let visibility_checker = if self
73+
.ctx
74+
.get_settings()
75+
.get_enable_experimental_sequence_privilege_check()?
76+
{
77+
let ctx = self.ctx.clone();
78+
Some(databend_common_base::runtime::block_on(async move {
79+
ctx.get_visibility_checker(false, Object::Sequence).await
80+
})?)
81+
} else {
82+
None
83+
};
84+
let value = databend_common_base::runtime::block_on(async move {
85+
async_func
86+
.generate(tenant.clone(), catalog.clone(), visibility_checker)
87+
.await
88+
})?;
89+
90+
*expr = ScalarExpr::ConstantExpr(ConstantExpr {
91+
span: async_func.span,
92+
value,
93+
});
94+
}
95+
ScalarExpr::FunctionCall(func) => {
96+
if BUILTIN_FUNCTIONS
97+
.get_property(&func.func_name)
98+
.map(|property| property.kind == FunctionKind::SRF)
99+
.unwrap_or(false)
100+
{
101+
self.scalars.push(expr.clone());
102+
return Ok(());
103+
}
104+
}
95105
ScalarExpr::WindowFunction(_)
96-
| ScalarExpr::AggregateFunction(_)
97-
| ScalarExpr::UDFCall(_)
98-
) {
99-
self.scalars.push(expr.clone());
100-
return Ok(());
106+
| ScalarExpr::AggregateFunction(_)
107+
| ScalarExpr::SubqueryExpr(_)
108+
| ScalarExpr::UDFCall(_)
109+
| ScalarExpr::UDAFCall(_) => {
110+
self.scalars.push(expr.clone());
111+
return Ok(());
112+
}
113+
ScalarExpr::BoundColumnRef(_)
114+
| ScalarExpr::ConstantExpr(_)
115+
| ScalarExpr::TypedConstantExpr(_, _)
116+
| ScalarExpr::LambdaFunction(_)
117+
| ScalarExpr::CastExpr(_) => {}
118+
ScalarExpr::UDFLambdaCall(_) => {}
101119
}
102120

103121
walk_expr_mut(self, expr)

โ€Žsrc/query/sql/src/planner/plans/ddl/table.rsโ€Ž

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,7 @@ pub struct AddTableColumnPlan {
314314
pub comment: String,
315315
pub option: AddColumnOption,
316316
pub is_deterministic: bool,
317+
pub is_nextval: bool,
317318
}
318319

319320
impl AddTableColumnPlan {

0 commit comments

Comments
ย (0)