Skip to content

Commit 90a70ff

Browse files
KKouldKKould
committed
chore: modify the column allocation method of auto increment expr and add auto increment check of add column
1 parent 6ef3e20 commit 90a70ff

File tree

6 files changed

+74
-26
lines changed

6 files changed

+74
-26
lines changed

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,12 @@ impl Interpreter for AddTableColumnInterpreter {
111111
&self.plan.field.name, &self.plan.table
112112
)));
113113
}
114+
if self.plan.is_autoincrement && num_rows > 0 {
115+
return Err(ErrorCode::AlterTableError(format!(
116+
"Cannot add column '{}' with `AUTOINCREMENT` to non-empty table '{}'",
117+
&self.plan.field.name, &self.plan.table
118+
)));
119+
}
114120
if field.default_expr().is_some() {
115121
let _ = DefaultExprBinder::try_new(self.ctx.clone())?.get_scalar(&field)?;
116122
}

โ€Ž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
@@ -186,6 +186,7 @@ async fn test_fuse_table_optimize_alter_table() -> Result<()> {
186186
option: AddColumnOption::End,
187187
is_deterministic: true,
188188
is_nextval: false,
189+
is_autoincrement: false,
189190
};
190191
let interpreter = AddTableColumnInterpreter::try_create(ctx.clone(), add_table_column_plan)?;
191192
let _ = interpreter.execute(ctx.clone()).await?;

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

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ use databend_common_expression::infer_schema_type;
7474
use databend_common_expression::infer_table_schema;
7575
use databend_common_expression::types::DataType;
7676
use databend_common_expression::AutoIncrementExpr;
77-
use databend_common_expression::ColumnId;
7877
use databend_common_expression::ComputedExpr;
7978
use databend_common_expression::DataField;
8079
use databend_common_expression::DataSchemaRefExt;
@@ -1093,7 +1092,7 @@ impl Binder {
10931092
.get_table(&catalog, &database, &table)
10941093
.await?
10951094
.schema();
1096-
let (field, comment, is_deterministic, is_nextval) =
1095+
let (field, comment, is_deterministic, is_nextval, is_autoincrement) =
10971096
self.analyze_add_column(column, schema).await?;
10981097
let option = match ast_option {
10991098
AstAddColumnOption::First => AddColumnOption::First,
@@ -1112,6 +1111,7 @@ impl Binder {
11121111
option,
11131112
is_deterministic,
11141113
is_nextval,
1114+
is_autoincrement,
11151115
})))
11161116
}
11171117
AlterTableAction::AddConstraint { constraint } => {
@@ -1182,7 +1182,7 @@ impl Binder {
11821182
.await?
11831183
.schema();
11841184
for column in column_def_vec {
1185-
let (field, comment, _, _) =
1185+
let (field, comment, _, _, _) =
11861186
self.analyze_add_column(column, schema.clone()).await?;
11871187
field_and_comment.push((field, comment));
11881188
}
@@ -1659,12 +1659,13 @@ impl Binder {
16591659
&self,
16601660
column: &ColumnDefinition,
16611661
table_schema: TableSchemaRef,
1662-
) -> Result<(TableField, String, bool, bool)> {
1662+
) -> Result<(TableField, String, bool, bool, bool)> {
16631663
let name = normalize_identifier(&column.name, &self.name_resolution_ctx).name;
16641664
let not_null = self.is_column_not_null();
16651665
let data_type = resolve_type_name(&column.data_type, not_null)?;
16661666
let mut is_deterministic = true;
16671667
let mut is_nextval = false;
1668+
let mut is_autoincrement = false;
16681669
let mut field = TableField::new(&name, data_type);
16691670

16701671
if let Some(expr) = &column.expr {
@@ -1710,16 +1711,23 @@ impl Binder {
17101711
));
17111712
}
17121713
field.auto_increment_expr = Some(AutoIncrementExpr {
1713-
column_id: table_schema.fields().len() as ColumnId,
1714+
column_id: table_schema.next_column_id(),
17141715
start: *start,
17151716
step: *step,
17161717
is_ordered: *is_ordered,
17171718
});
1719+
is_autoincrement = true;
17181720
}
17191721
}
17201722
}
17211723
let comment = column.comment.clone().unwrap_or_default();
1722-
Ok((field, comment, is_deterministic, is_nextval))
1724+
Ok((
1725+
field,
1726+
comment,
1727+
is_deterministic,
1728+
is_nextval,
1729+
is_autoincrement,
1730+
))
17231731
}
17241732

17251733
#[async_backtrace::framed]
@@ -1728,11 +1736,12 @@ impl Binder {
17281736
columns: &[ColumnDefinition],
17291737
) -> Result<(TableSchemaRef, Vec<String>)> {
17301738
let mut has_computed = false;
1739+
let mut has_autoincrement = false;
17311740
let mut fields = Vec::with_capacity(columns.len());
17321741
let mut fields_comments = Vec::with_capacity(columns.len());
17331742
let not_null = self.is_column_not_null();
17341743
let mut default_expr_binder = DefaultExprBinder::try_new(self.ctx.clone())?;
1735-
for (i, column) in columns.iter().enumerate() {
1744+
for column in columns.iter() {
17361745
let name = normalize_identifier(&column.name, &self.name_resolution_ctx).name;
17371746
let schema_data_type = resolve_type_name(&column.data_type, not_null)?;
17381747
fields_comments.push(column.comment.clone().unwrap_or_default());
@@ -1757,8 +1766,9 @@ impl Binder {
17571766
"AUTO INCREMENT only supports Decimal or Numeric (e.g. INT32) types",
17581767
));
17591768
}
1769+
has_autoincrement = true;
17601770
field.auto_increment_expr = Some(AutoIncrementExpr {
1761-
column_id: i as ColumnId,
1771+
column_id: 0,
17621772
start: *start,
17631773
step: *step,
17641774
is_ordered: *is_ordered,
@@ -1769,6 +1779,18 @@ impl Binder {
17691779
}
17701780
fields.push(field);
17711781
}
1782+
// update auto increment expr column id
1783+
if has_autoincrement {
1784+
let table_schema = TableSchema::new(fields.clone());
1785+
1786+
for (i, table_field) in table_schema.fields().iter().enumerate() {
1787+
let Some(auto_increment_expr) = fields[i].auto_increment_expr.as_mut() else {
1788+
continue;
1789+
};
1790+
1791+
auto_increment_expr.column_id = table_field.column_id;
1792+
}
1793+
}
17721794

17731795
let fields = if has_computed {
17741796
let mut source_fields = Vec::with_capacity(fields.len());

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,7 @@ pub struct AddTableColumnPlan {
315315
pub option: AddColumnOption,
316316
pub is_deterministic: bool,
317317
pub is_nextval: bool,
318+
pub is_autoincrement: bool,
318319
}
319320

320321
impl AddTableColumnPlan {

โ€Žtests/suites/0_stateless/02_ddl/02_0001_autoincrement.resultโ€Ž

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,29 +55,35 @@ Error: APIError: QueryFailed: [1065]error:
5555
>>>>
5656
alter table test_autoincrement add column c7 int autoincrement;
5757

58+
Error: APIError: QueryFailed: [1132]Cannot add column 'c7' with `AUTOINCREMENT` to non-empty table 'test_autoincrement'
59+
<<<<
5860
>>>>
59-
insert into test_autoincrement (c1) values(0);
61+
alter table test_autoincrement drop column c3;
6062

61-
1
6263
>>>> select * from test_autoincrement order by c2;
63-
0 0 0 1 1 0.00 NULL
64-
0 1 1 3 3 1.00 NULL
65-
0 2 2 5 5 2.00 NULL
66-
0 3 3 7 7 3.00 0
64+
0 0 1 1 0.00
65+
0 1 3 3 1.00
66+
0 2 5 5 2.00
6767
<<<<
6868
>>>>
69-
alter table test_autoincrement drop column c2;
69+
create or replace table test_empty_autoincrement (c1 int);
7070

71-
>>>> select * from test_autoincrement order by c2;
72-
Error: APIError: QueryFailed: [1065]error:
73-
--> SQL:1:43
74-
|
75-
1 | select * from test_autoincrement order by c2
76-
| ^^ column c2 doesn't exist
71+
>>>>
72+
alter table test_empty_autoincrement add column c2 int autoincrement;
73+
74+
>>>>
75+
insert into test_empty_autoincrement (c1) values(0);
7776

77+
1
78+
>>>>
79+
insert into test_empty_autoincrement (c1) values(0);
7880

81+
1
82+
>>>> select * from test_empty_autoincrement order by c2;
83+
0 0
84+
0 1
7985
<<<<
8086
>>>>
81-
drop table test_autoincrement;
87+
drop table test_empty_autoincrement;
8288

8389
>>>> drop database test

โ€Žtests/suites/0_stateless/02_ddl/02_0001_autoincrement.shโ€Ž

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,19 +41,31 @@ alter table test_autoincrement add column c7 int autoincrement;
4141
"""
4242

4343
stmt """
44-
insert into test_autoincrement (c1) values(0);
44+
alter table test_autoincrement drop column c3;
4545
"""
4646

4747
query "select * from test_autoincrement order by c2;"
4848

49+
# test add column success on empty table
4950
stmt """
50-
alter table test_autoincrement drop column c2;
51+
create or replace table test_empty_autoincrement (c1 int);
5152
"""
5253

53-
query "select * from test_autoincrement order by c2;"
54+
stmt """
55+
alter table test_empty_autoincrement add column c2 int autoincrement;
56+
"""
57+
58+
stmt """
59+
insert into test_empty_autoincrement (c1) values(0);
60+
"""
61+
stmt """
62+
insert into test_empty_autoincrement (c1) values(0);
63+
"""
64+
65+
query "select * from test_empty_autoincrement order by c2;"
5466

5567
stmt """
56-
drop table test_autoincrement;
68+
drop table test_empty_autoincrement;
5769
"""
5870

5971
stmt "drop database test"

0 commit comments

Comments
ย (0)