Skip to content

Commit d6fe352

Browse files
BohuTANGBohuTANG
authored
Revert "fix(query): alter column comment can not change column type" (#18192)
Revert "fix(query): alter column comment can not change column type (#18181)" This reverts commit 598b25d.
1 parent 598b25d commit d6fe352

File tree

13 files changed

+45
-221
lines changed

13 files changed

+45
-221
lines changed

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

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -980,24 +980,6 @@ 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-
1001983
#[derive(Debug, Clone, PartialEq, Drive, DriveMut)]
1002984
pub enum ModifyColumnAction {
1003985
// (column name id, masking policy name)
@@ -1008,8 +990,6 @@ pub enum ModifyColumnAction {
1008990
SetDataType(Vec<ColumnDefinition>),
1009991
// column name id
1010992
ConvertStoredComputedColumn(Identifier),
1011-
// (column name id, new comment)
1012-
Comment(Vec<ColumnComment>),
1013993
}
1014994

1015995
impl Display for ModifyColumnAction {
@@ -1027,7 +1007,6 @@ impl Display for ModifyColumnAction {
10271007
ModifyColumnAction::ConvertStoredComputedColumn(column) => {
10281008
write!(f, "{} DROP STORED", column)?
10291009
}
1030-
ModifyColumnAction::Comment(columns) => write_comma_separated_list(f, columns)?,
10311010
}
10321011

10331012
Ok(())

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

Lines changed: 11 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -3729,19 +3729,27 @@ 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+
37323739
map_res(
37333740
rule! {
37343741
#ident
37353742
~ #type_name
37363743
~ ( #nullable | #expr )*
3737-
: "`<column name> <type> [DEFAULT <expr>]`"
3744+
~ ( #comment )?
3745+
: "`<column name> <type> [DEFAULT <expr>] [COMMENT '<comment>']`"
37383746
},
3739-
|(name, data_type, constraints)| {
3747+
|(name, data_type, constraints, comment)| {
37403748
let mut def = ColumnDefinition {
37413749
name,
37423750
data_type,
37433751
expr: None,
3744-
comment: None,
3752+
comment,
37453753
};
37463754
for constraint in constraints {
37473755
match constraint {
@@ -3769,23 +3777,6 @@ pub fn modify_column_type(i: Input) -> IResult<ColumnDefinition> {
37693777
)(i)
37703778
}
37713779

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-
37893780
pub fn modify_column_action(i: Input) -> IResult<ModifyColumnAction> {
37903781
let set_mask_policy = map(
37913782
rule! {
@@ -3823,25 +3814,11 @@ pub fn modify_column_action(i: Input) -> IResult<ModifyColumnAction> {
38233814
},
38243815
);
38253816

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-
38393817
rule!(
38403818
#set_mask_policy
38413819
| #unset_mask_policy
38423820
| #convert_stored_computed_column
38433821
| #modify_column_type
3844-
| #modify_column_comment
38453822
)(i)
38463823
}
38473824

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -304,8 +304,7 @@ 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, b float NOT NULL;"#,
308-
r#"ALTER TABLE t MODIFY COLUMN a comment 'column a', COLUMN b COMMENT 'column b';"#,
307+
r#"ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, COLUMN b float NOT NULL COMMENT 'column b';"#,
309308
r#"ALTER TABLE t MODIFY COLUMN a int;"#,
310309
r#"ALTER TABLE t MODIFY a int;"#,
311310
r#"ALTER TABLE t MODIFY COLUMN a DROP STORED;"#,
@@ -1042,7 +1041,6 @@ fn test_statement_error() {
10421041
r#"drop table IDENTIFIER(a)"#,
10431042
r#"drop table IDENTIFIER(:a)"#,
10441043
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';"#,
10461044
// dictionary
10471045
r#"CREATE OR REPLACE DICTIONARY my_catalog.my_database.my_dictionary
10481046
(

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1048,16 +1048,6 @@ 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-
10611051
---------- Input ----------
10621052
CREATE OR REPLACE DICTIONARY my_catalog.my_database.my_dictionary
10631053
(

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

Lines changed: 6 additions & 65 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, b float NOT NULL;
13991+
ALTER TABLE t MODIFY COLUMN a int NULL DEFAULT 1, COLUMN b float NOT NULL COMMENT 'column b';
1399213992
---------- Output ---------
13993-
ALTER TABLE t MODIFY COLUMN a Int32 NULL DEFAULT 1, b Float32 NOT NULL
13993+
ALTER TABLE t MODIFY COLUMN a Int32 NULL DEFAULT 1, b Float32 NOT NULL COMMENT 'column b'
1399413994
---------- AST ------------
1399513995
AlterTable(
1399613996
AlterTableStmt {
@@ -14048,7 +14048,7 @@ AlterTable(
1404814048
ColumnDefinition {
1404914049
name: Identifier {
1405014050
span: Some(
14051-
50..51,
14051+
57..58,
1405214052
),
1405314053
name: "b",
1405414054
quote: None,
@@ -14058,68 +14058,9 @@ AlterTable(
1405814058
Float32,
1405914059
),
1406014060
expr: None,
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",
14061+
comment: Some(
14062+
"column b",
14063+
),
1412314064
},
1412414065
],
1412514066
),

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

Lines changed: 13 additions & 52 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],
148+
field_and_comments: &[(TableField, String)],
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 in field_and_comments {
155+
for (field, _comment) 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,8 +208,9 @@ impl ModifyTableColumnInterpreter {
208208

209209
let mut table_info = table.get_table_info().clone();
210210
table_info.meta.fill_field_comments();
211-
for field in field_and_comments {
212-
if let Some((_i, old_field)) = schema.column_with_name(&field.name) {
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) {
213214
if old_field.data_type != field.data_type {
214215
// If the column is defined in bloom index columns,
215216
// check whether the data type is supported for bloom index.
@@ -236,6 +237,10 @@ impl ModifyTableColumnInterpreter {
236237
}
237238
}
238239
}
240+
if table_info.meta.field_comments[i] != *comment {
241+
table_info.meta.field_comments[i] = comment.to_string();
242+
modify_comment = true;
243+
}
239244
} else {
240245
return Err(ErrorCode::UnknownColumn(format!(
241246
"Cannot find column {}",
@@ -245,14 +250,14 @@ impl ModifyTableColumnInterpreter {
245250
}
246251

247252
// check if schema has changed
248-
if schema == new_schema {
253+
if schema == new_schema && !modify_comment {
249254
return Ok(PipelineBuildResult::create());
250255
}
251256

252257
let mut modified_field_indices = HashSet::new();
253258
let new_schema_without_computed_fields = new_schema.remove_computed_fields();
254259
if schema != new_schema {
255-
for field in field_and_comments {
260+
for (field, _) in field_and_comments {
256261
let field_index = new_schema_without_computed_fields.index_of(&field.name)?;
257262
let old_field = schema.field_with_name(&field.name)?;
258263
let is_alter_column_string_to_binary =
@@ -449,49 +454,6 @@ impl ModifyTableColumnInterpreter {
449454
.await
450455
}
451456

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-
495457
// unset data mask policy to a column is a ee feature.
496458
async fn do_unset_data_mask_policy(
497459
&self,
@@ -631,9 +593,8 @@ impl Interpreter for ModifyTableColumnInterpreter {
631593
self.do_unset_data_mask_policy(catalog, table, column.to_string())
632594
.await?
633595
}
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?
596+
ModifyColumnAction::SetDataType(field_and_comment) => {
597+
self.do_set_data_type(table, field_and_comment).await?
637598
}
638599
ModifyColumnAction::ConvertStoredComputedColumn(column) => {
639600
self.do_convert_stored_computed_column(

0 commit comments

Comments
 (0)