fix: bugs introduced by alter table options (#4953)

* fix: ChangeTableOptions display * fix: partition number disappear after altering table options * Update src/table/src/metadata.rs Co-authored-by: Ruihang Xia <[email protected]> --------- Co-authored-by: Lei, HUANG <[email protected]> Co-authored-by: Ruihang Xia <[email protected]>
GreptimeTeam · Nov 6, 2024 · 305767e · 305767e
1 parent 22a662f
commit 305767e
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 33 deletions.
diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs
@@ -486,6 +486,8 @@ mod tests {
         let AlterTableOperation::ChangeTableOptions { options } = &alter.alter_operation else {
             unreachable!()
         };
+
+        assert_eq!(sql, alter.to_string());
         let res = options
             .iter()
             .map(|o| (o.key.as_str(), o.value.as_str()))
@@ -495,16 +497,16 @@ mod tests {
 
     #[test]
     fn test_parse_alter_column() {
-        check_parse_alter_table("ALTER TABLE test_table SET 'a'='A';", &[("a", "A")]);
+        check_parse_alter_table("ALTER TABLE test_table SET 'a'='A'", &[("a", "A")]);
         check_parse_alter_table(
             "ALTER TABLE test_table SET 'a'='A','b'='B'",
             &[("a", "A"), ("b", "B")],
         );
         check_parse_alter_table(
-            "ALTER TABLE test_table SET 'a'='A','b'='B','c'='C';",
+            "ALTER TABLE test_table SET 'a'='A','b'='B','c'='C'",
             &[("a", "A"), ("b", "B"), ("c", "C")],
         );
-        check_parse_alter_table("ALTER TABLE test_table SET 'a'=NULL;", &[("a", "")]);
+        check_parse_alter_table("ALTER TABLE test_table SET 'a'=NULL", &[("a", "")]);
 
         ParserContext::create_with_dialect(
             "ALTER TABLE test_table SET a INTEGER",

diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs
@@ -16,6 +16,7 @@ use std::fmt::{Debug, Display};
 
 use api::v1;
 use common_query::AddColumnLocation;
+use itertools::Itertools;
 use sqlparser::ast::{ColumnDef, DataType, Ident, ObjectName, TableConstraint};
 use sqlparser_derive::{Visit, VisitMut};
 
@@ -68,7 +69,7 @@ pub enum AlterTableOperation {
         column_name: Ident,
         target_type: DataType,
     },
-    /// `MODIFY <table attrs key> = <table attr value>`
+    /// `SET <table attrs key> = <table attr value>`
     ChangeTableOptions { options: Vec<ChangeTableOption> },
     /// `DROP COLUMN <name>`
     DropColumn { name: Ident },
@@ -101,9 +102,19 @@ impl Display for AlterTableOperation {
                 write!(f, r#"MODIFY COLUMN {column_name} {target_type}"#)
             }
             AlterTableOperation::ChangeTableOptions { options } => {
-                for ChangeTableOption { key, value } in options {
-                    write!(f, r#"MODIFY '{key}'='{value}', "#)?;
-                }
+                let kvs = options
+                    .iter()
+                    .map(|ChangeTableOption { key, value }| {
+                        if !value.is_empty() {
+                            format!("'{key}'='{value}'")
+                        } else {
+                            format!("'{key}'=NULL")
+                        }
+                    })
+                    .join(",");
+
+                write!(f, "SET {kvs}")?;
+
                 Ok(())
             }
         }

diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs
@@ -201,15 +201,7 @@ impl TableMeta {
                 self.change_column_types(table_name, columns)
             }
             // No need to rebuild table meta when renaming tables.
-            AlterKind::RenameTable { .. } => {
-                let mut meta_builder = TableMetaBuilder::default();
-                let _ = meta_builder
-                    .schema(self.schema.clone())
-                    .primary_key_indices(self.primary_key_indices.clone())
-                    .engine(self.engine.clone())
-                    .next_column_id(self.next_column_id);
-                Ok(meta_builder)
-            }
+            AlterKind::RenameTable { .. } => Ok(self.new_meta_builder()),
             AlterKind::ChangeTableOptions { options } => self.change_table_options(options),
         }
     }
@@ -229,13 +221,9 @@ impl TableMeta {
                 }
             }
         }
-        let mut builder = TableMetaBuilder::default();
-        builder
-            .options(new_options)
-            .schema(self.schema.clone())
-            .primary_key_indices(self.primary_key_indices.clone())
-            .engine(self.engine.clone())
-            .next_column_id(self.next_column_id);
+        let mut builder = self.new_meta_builder();
+        builder.options(new_options);
+
         Ok(builder)
     }
 
@@ -266,9 +254,14 @@ impl TableMeta {
         Ok(desc)
     }
 
+    /// Create a [`TableMetaBuilder`].
+    ///
+    /// Note: please always use this function to create the builder.
     fn new_meta_builder(&self) -> TableMetaBuilder {
         let mut builder = TableMetaBuilder::default();
         let _ = builder
+            .schema(self.schema.clone())
+            .primary_key_indices(self.primary_key_indices.clone())
             .engine(&self.engine)
             .options(self.options.clone())
             .created_on(self.created_on)

diff --git a/tests/cases/standalone/common/alter/alter_table_options.result b/tests/cases/standalone/common/alter/alter_table_options.result
@@ -1,11 +1,33 @@
-CREATE TABLE ato(i INTEGER, j TIMESTAMP TIME INDEX);
+CREATE TABLE ato(i INTEGER, j TIMESTAMP TIME INDEX, PRIMARY KEY(i));
 
 Affected Rows: 0
 
+INSERT INTO ato VALUES(1, now()), (2, now());
+
+Affected Rows: 2
+
+SELECT i FROM ato;
+
++---+
+| i |
++---+
+| 1 |
+| 2 |
++---+
+
 ALTER TABLE ato SET 'ttl'='1d';
 
 Affected Rows: 0
 
+SELECT i FROM ato;
+
++---+
+| i |
++---+
+| 1 |
+| 2 |
++---+
+
 SHOW CREATE TABLE ato;
 
 +-------+------------------------------------+
@@ -14,7 +36,8 @@ SHOW CREATE TABLE ato;
 | ato   | CREATE TABLE IF NOT EXISTS "ato" ( |
 |       |   "i" INT NULL,                    |
 |       |   "j" TIMESTAMP(3) NOT NULL,       |
-|       |   TIME INDEX ("j")                 |
+|       |   TIME INDEX ("j"),                |
+|       |   PRIMARY KEY ("i")                |
 |       | )                                  |
 |       |                                    |
 |       | ENGINE=mito                        |
@@ -27,6 +50,15 @@ ALTER TABLE ato SET 'ttl'='2d';
 
 Affected Rows: 0
 
+SELECT i FROM ato;
+
++---+
+| i |
++---+
+| 1 |
+| 2 |
++---+
+
 SHOW CREATE TABLE ato;
 
 +-------+------------------------------------+
@@ -35,7 +67,8 @@ SHOW CREATE TABLE ato;
 | ato   | CREATE TABLE IF NOT EXISTS "ato" ( |
 |       |   "i" INT NULL,                    |
 |       |   "j" TIMESTAMP(3) NOT NULL,       |
-|       |   TIME INDEX ("j")                 |
+|       |   TIME INDEX ("j"),                |
+|       |   PRIMARY KEY ("i")                |
 |       | )                                  |
 |       |                                    |
 |       | ENGINE=mito                        |
@@ -48,6 +81,15 @@ ALTER TABLE ato SET 'ttl'=NULL;
 
 Affected Rows: 0
 
+SELECT i FROM ato;
+
++---+
+| i |
++---+
+| 1 |
+| 2 |
++---+
+
 SHOW CREATE TABLE ato;
 
 +-------+------------------------------------+
@@ -56,14 +98,15 @@ SHOW CREATE TABLE ato;
 | ato   | CREATE TABLE IF NOT EXISTS "ato" ( |
 |       |   "i" INT NULL,                    |
 |       |   "j" TIMESTAMP(3) NOT NULL,       |
-|       |   TIME INDEX ("j")                 |
+|       |   TIME INDEX ("j"),                |
+|       |   PRIMARY KEY ("i")                |
 |       | )                                  |
 |       |                                    |
 |       | ENGINE=mito                        |
 |       |                                    |
 +-------+------------------------------------+
 
-ALTER TABLE ato SET 'ttl'='0d';
+ALTER TABLE ato SET 'ttl'='1s';
 
 Affected Rows: 0
 
@@ -75,13 +118,25 @@ SHOW CREATE TABLE ato;
 | ato   | CREATE TABLE IF NOT EXISTS "ato" ( |
 |       |   "i" INT NULL,                    |
 |       |   "j" TIMESTAMP(3) NOT NULL,       |
-|       |   TIME INDEX ("j")                 |
+|       |   TIME INDEX ("j"),                |
+|       |   PRIMARY KEY ("i")                |
 |       | )                                  |
 |       |                                    |
 |       | ENGINE=mito                        |
-|       |                                    |
+|       | WITH(                              |
+|       |   ttl = '1s'                       |
+|       | )                                  |
 +-------+------------------------------------+
 
+SELECT i FROM ato;
+
++---+
+| i |
++---+
+| 1 |
+| 2 |
++---+
+
 DROP TABLE ato;
 
 Affected Rows: 0

diff --git a/tests/cases/standalone/common/alter/alter_table_options.sql b/tests/cases/standalone/common/alter/alter_table_options.sql
@@ -1,19 +1,31 @@
-CREATE TABLE ato(i INTEGER, j TIMESTAMP TIME INDEX);
+CREATE TABLE ato(i INTEGER, j TIMESTAMP TIME INDEX, PRIMARY KEY(i));
+
+INSERT INTO ato VALUES(1, now()), (2, now());
+
+SELECT i FROM ato;
 
 ALTER TABLE ato SET 'ttl'='1d';
 
+SELECT i FROM ato;
+
 SHOW CREATE TABLE ato;
 
 ALTER TABLE ato SET 'ttl'='2d';
 
+SELECT i FROM ato;
+
 SHOW CREATE TABLE ato;
 
 ALTER TABLE ato SET 'ttl'=NULL;
 
+SELECT i FROM ato;
+
 SHOW CREATE TABLE ato;
 
-ALTER TABLE ato SET 'ttl'='0d';
+ALTER TABLE ato SET 'ttl'='1s';
 
 SHOW CREATE TABLE ato;
 
-DROP TABLE ato;
+SELECT i FROM ato;
+
+DROP TABLE ato;