Skip to content

Commit 598b25d

Browse files
TCeasonTCeason
authored
fix(query): alter column comment can not change column type (#18181)
* fix(query): alter column comment can not change column type Support new syntax: In main: ```sql ALTER TABLE <TABLE_NAME> MODIFY <column name> <type> [DEFAULT <expr>] [, COLUMN? <column name> <type> [DEFAULT <expr>] <comment>]; ``` In pr: ``` ALTER TABLE <TABLE_NAME> MODIFY <column name> <type> [DEFAULT <expr>] [, COLUMN? <column name> <type> [DEFAULT <expr>]]; ALTER TABLE <TABLE_NAME> MODIFY <column name> <comment> [, COLUMN? <column_name> <comment>]; ``` * refactor: add ModifyColumnAction::Comment and do_set_comment
1 parent b1b9fc5 commit 598b25d

File tree

13 files changed

+221
-45
lines changed

13 files changed

+221
-45
lines changed

‎src/query/ast/src/ast/statements/table.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -980,6 +980,24 @@ impl Display for CreateDefinition {
980980
}
981981
}
982982

983+
#[derive(Debug, Clone, PartialEq, Drive, DriveMut)]
984+
pub struct ColumnComment {
985+
pub name: Identifier,
986+
pub comment: String,
987+
}
988+
989+
impl Display for ColumnComment {
990+
fn fmt(&self, f: &mut Formatter) -> std::fmt::Result {
991+
write!(
992+
f,
993+
"{} COMMENT {}",
994+
self.name,
995+
QuotedString(&self.comment, '\'')
996+
)?;
997+
Ok(())
998+
}
999+
}
1000+
9831001
#[derive(Debug, Clone, PartialEq, Drive, DriveMut)]
9841002
pub enum ModifyColumnAction {
9851003
// (column name id, masking policy name)
@@ -990,6 +1008,8 @@ pub enum ModifyColumnAction {
9901008
SetDataType(Vec<ColumnDefinition>),
9911009
// column name id
9921010
ConvertStoredComputedColumn(Identifier),
1011+
// (column name id, new comment)
1012+
Comment(Vec<ColumnComment>),
9931013
}
9941014

9951015
impl Display for ModifyColumnAction {
@@ -1007,6 +1027,7 @@ impl Display for ModifyColumnAction {
10071027
ModifyColumnAction::ConvertStoredComputedColumn(column) => {
10081028
write!(f, "{} DROP STORED", column)?
10091029
}
1030+
ModifyColumnAction::Comment(columns) => write_comma_separated_list(f, columns)?,
10101031
}
10111032

10121033
Ok(())

‎src/query/ast/src/parser/statement.rs‎

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3729,27 +3729,19 @@ pub fn modify_column_type(i: Input) -> IResult<ColumnDefinition> {
37293729
|(_, default_expr)| ColumnConstraint::DefaultExpr(Box::new(default_expr)),
37303730
),));
37313731

3732-
let comment = map(
3733-
rule! {
3734-
COMMENT ~ #literal_string
3735-
},
3736-
|(_, comment)| comment,
3737-
);
3738-
37393732
map_res(
37403733
rule! {
37413734
#ident
37423735
~ #type_name
37433736
~ ( #nullable | #expr )*
3744-
~ ( #comment )?
3745-
: "`<column name> <type> [DEFAULT <expr>] [COMMENT '<comment>']`"
3737+
: "`<column name> <type> [DEFAULT <expr>]`"
37463738
},
3747-
|(name, data_type, constraints, comment)| {
3739+
|(name, data_type, constraints)| {
37483740
let mut def = ColumnDefinition {
37493741
name,
37503742
data_type,
37513743
expr: None,
3752-
comment,
3744+
comment: None,
37533745
};
37543746
for constraint in constraints {
37553747
match constraint {
@@ -3777,6 +3769,23 @@ pub fn modify_column_type(i: Input) -> IResult<ColumnDefinition> {
37773769
)(i)
37783770
}
37793771

3772+
pub fn modify_column_comment(i: Input) -> IResult<ColumnComment> {
3773+
let comment = map(
3774+
rule! {
3775+
COMMENT ~ #literal_string
3776+
},
3777+
|(_, comment)| comment,
3778+
);
3779+
map_res(
3780+
rule! {
3781+
#ident
3782+
~ #comment
3783+
: "`<column name> COMMENT '<comment>'`"
3784+
},
3785+
|(name, comment)| Ok(ColumnComment { name, comment }),
3786+
)(i)
3787+
}
3788+
37803789
pub fn modify_column_action(i: Input) -> IResult<ModifyColumnAction> {
37813790
let set_mask_policy = map(
37823791
rule! {
@@ -3814,11 +3823,25 @@ pub fn modify_column_action(i: Input) -> IResult<ModifyColumnAction> {
38143823
},
38153824
);
38163825

3826+
let modify_column_comment = map(
3827+
rule! {
3828+
#modify_column_comment ~ ("," ~ COLUMN? ~ #modify_column_comment)*
3829+
},
3830+
|(column_def, column_def_vec)| {
3831+
let mut column_defs = vec![column_def];
3832+
column_def_vec
3833+
.iter()
3834+
.for_each(|(_, _, column_def)| column_defs.push(column_def.clone()));
3835+
ModifyColumnAction::Comment(column_defs)
3836+
},
3837+
);
3838+
38173839
rule!(
38183840
#set_mask_policy
38193841
| #unset_mask_policy
38203842
| #convert_stored_computed_column
38213843
| #modify_column_type
3844+
| #modify_column_comment
38223845
)(i)
38233846
}
38243847

‎src/query/ast/tests/it/parser.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,8 @@ SELECT * from s;"#,
304304
r#"ALTER TABLE t MODIFY COLUMN b SET MASKING POLICY mask;"#,
305305
r#"ALTER TABLE t MODIFY COLUMN b UNSET MASKING POLICY;"#,
306306
r#"ALTER TABLE t MODIFY COLUMN a int DEFAULT 1, COLUMN b float;"#,
307-
r#"ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, COLUMN b float NOT NULL COMMENT 'column b';"#,
307+
r#"ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, b float NOT NULL;"#,
308+
r#"ALTER TABLE t MODIFY COLUMN a comment 'column a', COLUMN b COMMENT 'column b';"#,
308309
r#"ALTER TABLE t MODIFY COLUMN a int;"#,
309310
r#"ALTER TABLE t MODIFY a int;"#,
310311
r#"ALTER TABLE t MODIFY COLUMN a DROP STORED;"#,
@@ -1041,6 +1042,7 @@ fn test_statement_error() {
10411042
r#"drop table IDENTIFIER(a)"#,
10421043
r#"drop table IDENTIFIER(:a)"#,
10431044
r#"SHOW GRANTS ON task t1;"#,
1045+
r#"ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, COLUMN b float NOT NULL COMMENT 'column b';"#,
10441046
// dictionary
10451047
r#"CREATE OR REPLACE DICTIONARY my_catalog.my_database.my_dictionary
10461048
(

‎src/query/ast/tests/it/testdata/stmt-error.txt‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1048,6 +1048,16 @@ error:
10481048
| ^^^^ unexpected `task`, expecting `STAGE`, `TABLE`, `WAREHOUSE`, `DATABASE`, or `UDF`
10491049

10501050

1051+
---------- Input ----------
1052+
ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, COLUMN b float NOT NULL COMMENT 'column b';
1053+
---------- Output ---------
1054+
error:
1055+
--> SQL:1:75
1056+
|
1057+
1 | ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, COLUMN b float NOT NULL COMMENT 'column b';
1058+
| ^^^^^^^ unexpected `COMMENT`, expecting `FORMAT`, `DEFAULT`, `NOT`, `NULL`, `,`, or `;`
1059+
1060+
10511061
---------- Input ----------
10521062
CREATE OR REPLACE DICTIONARY my_catalog.my_database.my_dictionary
10531063
(

‎src/query/ast/tests/it/testdata/stmt.txt‎

Lines changed: 65 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13988,9 +13988,9 @@ AlterTable(
1398813988

1398913989

1399013990
---------- Input ----------
13991-
ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, COLUMN b float NOT NULL COMMENT 'column b';
13991+
ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, b float NOT NULL;
1399213992
---------- Output ---------
13993-
ALTER TABLE t MODIFY COLUMN a Int32 NULL DEFAULT 1, b Float32 NOT NULL COMMENT 'column b'
13993+
ALTER TABLE t MODIFY COLUMN a Int32 NULL DEFAULT 1, b Float32 NOT NULL
1399413994
---------- AST ------------
1399513995
AlterTable(
1399613996
AlterTableStmt {
@@ -14048,7 +14048,7 @@ AlterTable(
1404814048
ColumnDefinition {
1404914049
name: Identifier {
1405014050
span: Some(
14051-
57..58,
14051+
50..51,
1405214052
),
1405314053
name: "b",
1405414054
quote: None,
@@ -14058,9 +14058,68 @@ AlterTable(
1405814058
Float32,
1405914059
),
1406014060
expr: None,
14061-
comment: Some(
14062-
"column b",
14063-
),
14061+
comment: None,
14062+
},
14063+
],
14064+
),
14065+
},
14066+
},
14067+
)
14068+
14069+
14070+
---------- Input ----------
14071+
ALTER TABLE t MODIFY COLUMN a comment 'column a', COLUMN b COMMENT 'column b';
14072+
---------- Output ---------
14073+
ALTER TABLE t MODIFY COLUMN a COMMENT 'column a', b COMMENT 'column b'
14074+
---------- AST ------------
14075+
AlterTable(
14076+
AlterTableStmt {
14077+
if_exists: false,
14078+
table_reference: Table {
14079+
span: Some(
14080+
12..13,
14081+
),
14082+
catalog: None,
14083+
database: None,
14084+
table: Identifier {
14085+
span: Some(
14086+
12..13,
14087+
),
14088+
name: "t",
14089+
quote: None,
14090+
ident_type: None,
14091+
},
14092+
alias: None,
14093+
temporal: None,
14094+
with_options: None,
14095+
pivot: None,
14096+
unpivot: None,
14097+
sample: None,
14098+
},
14099+
action: ModifyColumn {
14100+
action: Comment(
14101+
[
14102+
ColumnComment {
14103+
name: Identifier {
14104+
span: Some(
14105+
28..29,
14106+
),
14107+
name: "a",
14108+
quote: None,
14109+
ident_type: None,
14110+
},
14111+
comment: "column a",
14112+
},
14113+
ColumnComment {
14114+
name: Identifier {
14115+
span: Some(
14116+
57..58,
14117+
),
14118+
name: "b",
14119+
quote: None,
14120+
ident_type: None,
14121+
},
14122+
comment: "column b",
1406414123
},
1406514124
],
1406614125
),

‎src/query/service/src/interpreters/interpreter_table_modify_column.rs‎

Lines changed: 52 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -145,14 +145,14 @@ impl ModifyTableColumnInterpreter {
145145
async fn do_set_data_type(
146146
&self,
147147
table: Arc<dyn Table>,
148-
field_and_comments: &[(TableField, String)],
148+
field_and_comments: &[TableField],
149149
) -> Result<PipelineBuildResult> {
150150
let schema = table.schema().as_ref().clone();
151151
let table_info = table.get_table_info();
152152
let mut new_schema = schema.clone();
153153
let mut default_expr_binder = DefaultExprBinder::try_new(self.ctx.clone())?;
154154
// first check default expr before lock table
155-
for (field, _comment) in field_and_comments {
155+
for field in field_and_comments {
156156
if let Some((i, old_field)) = schema.column_with_name(&field.name) {
157157
// if the field has different leaf column numbers, we need drop the old column
158158
// and add a new one to generate new column id. otherwise, leaf column ids will conflict.
@@ -208,9 +208,8 @@ impl ModifyTableColumnInterpreter {
208208

209209
let mut table_info = table.get_table_info().clone();
210210
table_info.meta.fill_field_comments();
211-
let mut modify_comment = false;
212-
for (field, comment) in field_and_comments {
213-
if let Some((i, old_field)) = schema.column_with_name(&field.name) {
211+
for field in field_and_comments {
212+
if let Some((_i, old_field)) = schema.column_with_name(&field.name) {
214213
if old_field.data_type != field.data_type {
215214
// If the column is defined in bloom index columns,
216215
// check whether the data type is supported for bloom index.
@@ -237,10 +236,6 @@ impl ModifyTableColumnInterpreter {
237236
}
238237
}
239238
}
240-
if table_info.meta.field_comments[i] != *comment {
241-
table_info.meta.field_comments[i] = comment.to_string();
242-
modify_comment = true;
243-
}
244239
} else {
245240
return Err(ErrorCode::UnknownColumn(format!(
246241
"Cannot find column {}",
@@ -250,14 +245,14 @@ impl ModifyTableColumnInterpreter {
250245
}
251246

252247
// check if schema has changed
253-
if schema == new_schema && !modify_comment {
248+
if schema == new_schema {
254249
return Ok(PipelineBuildResult::create());
255250
}
256251

257252
let mut modified_field_indices = HashSet::new();
258253
let new_schema_without_computed_fields = new_schema.remove_computed_fields();
259254
if schema != new_schema {
260-
for (field, _) in field_and_comments {
255+
for field in field_and_comments {
261256
let field_index = new_schema_without_computed_fields.index_of(&field.name)?;
262257
let old_field = schema.field_with_name(&field.name)?;
263258
let is_alter_column_string_to_binary =
@@ -454,6 +449,49 @@ impl ModifyTableColumnInterpreter {
454449
.await
455450
}
456451

452+
// Set column comment.
453+
async fn do_set_comment(
454+
&self,
455+
table: Arc<dyn Table>,
456+
field_and_comments: &[(TableField, String)],
457+
) -> Result<PipelineBuildResult> {
458+
let schema = table.schema().as_ref().clone();
459+
let table_info = table.get_table_info();
460+
461+
let catalog_name = table_info.catalog();
462+
let catalog = self.ctx.get_catalog(catalog_name).await?;
463+
464+
let mut table_info = table.get_table_info().clone();
465+
table_info.meta.fill_field_comments();
466+
let mut modify_comment = false;
467+
for (field, comment) in field_and_comments {
468+
if let Some((i, _)) = schema.column_with_name(&field.name) {
469+
if table_info.meta.field_comments[i] != *comment {
470+
table_info.meta.field_comments[i] = comment.to_string();
471+
modify_comment = true;
472+
}
473+
} else {
474+
return Err(ErrorCode::UnknownColumn(format!(
475+
"Cannot find column {}",
476+
field.name
477+
)));
478+
}
479+
}
480+
481+
if modify_comment {
482+
commit_table_meta(
483+
&self.ctx,
484+
table.as_ref(),
485+
&table_info,
486+
table_info.meta.clone(),
487+
catalog,
488+
)
489+
.await?;
490+
}
491+
492+
Ok(PipelineBuildResult::create())
493+
}
494+
457495
// unset data mask policy to a column is a ee feature.
458496
async fn do_unset_data_mask_policy(
459497
&self,
@@ -593,8 +631,9 @@ impl Interpreter for ModifyTableColumnInterpreter {
593631
self.do_unset_data_mask_policy(catalog, table, column.to_string())
594632
.await?
595633
}
596-
ModifyColumnAction::SetDataType(field_and_comment) => {
597-
self.do_set_data_type(table, field_and_comment).await?
634+
ModifyColumnAction::SetDataType(fields) => self.do_set_data_type(table, fields).await?,
635+
ModifyColumnAction::Comment(field_and_comment) => {
636+
self.do_set_comment(table, field_and_comment).await?
598637
}
599638
ModifyColumnAction::ConvertStoredComputedColumn(column) => {
600639
self.do_convert_stored_computed_column(

0 commit comments

Comments
 (0)