Alter table #104

guojidan · 2023-11-27T06:58:24Z

What problem does this PR solve?

Add corresponding issue link with summary if exists -->

Issue link: #101

What is changed and how it works?

Code changes

Has Rust code change
Has CI related scripts change

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Note for reviewer

…e_aggregation cause index scan error

KKould · 2023-11-28T07:24:21Z

src/binder/alter_table.rs

+    ) -> Result<LogicalPlan, BindError> {
+        let table_name: Arc<String> = Arc::new(split_name(&lower_case_name(name))?.1.to_string());
+
+        // we need convert ColumnDef to ColumnCatalog


is it what you need: https://github.com/KipData/KipSQL/blob/e7dbac15d7f8fac1e42b6b4291fc7251e2685d88/src/binder/create_table.rs#L85

It looks like it has been used below, so this comment is probably unnecessary?

this annotation is deprecated, I forget remove it

KKould · 2023-11-28T07:25:29Z

src/binder/insert.rs

@@ -42,7 +42,7 @@ impl<'a, T: Transaction> Binder<'a, T> {
                }
            }
            let mut rows = Vec::with_capacity(expr_rows.len());
-
+            println!("{:?}", expr_rows);


It seems that I forgot to remove it during debugging.

KKould · 2023-11-28T07:32:19Z

src/planner/operator/alter_table.rs

+
+#[derive(Debug, PartialEq, Clone)]
+pub enum AlterTableOperator {
+    AddColumn(AddColumn),


Convert these enumerations into their corresponding Operators so that they can correspond better when writing execution operators, otherwise they will be coupled together. Maybe the Alert-related Operators can be placed in a directory to categorize them.

KKould · 2023-11-28T07:32:57Z

src/storage/kip.rs

@@ -161,6 +162,64 @@ impl Transaction for KipTransaction {
        Ok(())
    }

+    fn alter_table(&mut self, op: &AlterTableOperator) -> Result<(), StorageError> {


same to AlertOperator Comment

KKould · 2023-11-28T07:35:39Z

tests/slt/alter_table.slt

@@ -0,0 +1,19 @@
+statement ok
+create table alter_table(id int primary key, v1 int)


In fact, the field defaults to Not Null. This probably also explains that when filling the Null value in https://github.com/KipData/KipSQL/pull/104/files#diff-ef2e7111e3dcf8c93251d804dcf4416c7fcc6a99a00aa5fd137a822d90f35eb4, you need to check whether it is allowed to be Null and throw an exception.

check in there: https://github.com/KipData/KipSQL/pull/104/files#diff-f2254317cf50142c53882a4e2cbe4715cf85b403c38071ed0edfa7b53a42d986R177

check in there: https://github.com/KipData/KipSQL/pull/104/files#diff-f2254317cf50142c53882a4e2cbe4715cf85b403c38071ed0edfa7b53a42d986R177

It should be necessary to determine whether there is a default value here. When it is not nullable but has a default value, it is probably allowed to execute.

As mentioned above, it can be declared as tests/slt/alter_table/add_column.slt

KKould · 2023-12-04T09:36:27Z

src/planner/operator/alter_table.rs

+use crate::catalog::{ColumnCatalog, TableName};
+
+#[derive(Debug, PartialEq, Clone)]
+pub struct AddColumnOperator {


For alert functions that contain multiple operators, you can change the file name to add_column and wrap it in the alert folder.

KKould · 2023-12-04T09:37:01Z

src/execution/executor/ddl/alter_table.rs

+    storage::Transaction,
+};
+
+pub struct AddColumn {


For alert functions that contain multiple operators, you can change the file name to add_column and wrap it in the alert folder.

KKould · 2023-12-04T09:44:07Z

src/storage/kip.rs

+        // we need catalog generate col_id && index_id
+        // generally catalog is immutable, so do not worry it changed when alter table going on
+        if let Some(mut catalog) = self.table(table_name.clone()).cloned() {
+            // not yet supported default value


Already supported

KKould · 2023-12-04T09:53:06Z

src/storage/kip.rs

+            let col_id = catalog.add_column(column.clone())?;
+
+            if column.desc.is_unique {
+                let meta = IndexMeta {


note for me: Creating IndexMeta this way is not beautiful

KKould · 2023-12-04T10:11:25Z

src/storage/kip.rs

+            let column = catalog.get_column_by_id(&col_id).unwrap();
+            let (key, value) = TableCodec::encode_column(&table_name, column)?;
+            self.tx.set(key, value);
+


Since many operations require TableCatalog to read metainformation, there is a dedicated cache for the catalog in Transaction, and the catalog in the cache should be updated here. Tips: Transaction is executed serially

KKould · 2023-12-04T10:18:27Z

src/storage/kip.rs

+            if_not_exists,
+            column,
+        } = op;
+        // we need catalog generate col_id && index_id


note for me

KKould · 2023-12-05T12:37:39Z

everything looks good! thx ;)

guojidan and others added 10 commits November 27, 2023 07:10

alter table

19fccbe

refactor(column): remove field: table_name

96476cf

feat(create_table): add default on CreateTable

83fcda2

feat(cast): support CAST Function

04f076f

feat: Constraints for CreateTable and Fix Or BinaryOperator in scop…

ee60f32

…e_aggregation cause index scan error

version up

dafdbbc

feat(create_table): support if not exists

6e89bbe

version up

6ce812e

code fmt

0edc879

support add column

8b54b7c

guojidan force-pushed the alter-table branch from 47e81cd to 8b54b7c Compare November 27, 2023 07:13

KKould self-requested a review November 27, 2023 09:08

KKould assigned guojidan Nov 27, 2023

KKould added the enhancement New feature or request label Nov 27, 2023

guojidan and others added 2 commits November 28, 2023 08:36

Merge branch 'main' into alter-table

6545289

fix conflict

33c3f0f

KKould reviewed Nov 28, 2023

View reviewed changes

split operator

0cd6841

KKould reviewed Dec 4, 2023

View reviewed changes

src/storage/kip.rs Outdated

if_not_exists,

column,

} = op;

// we need catalog generate col_id && index_id

Copy link

Member

KKould Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note for me

guojidan added 2 commits December 5, 2023 02:40

optimize

7fb1354

optimize

08feb58

guojidan added 2 commits December 5, 2023 09:33

fix tuple decode err

0f996fa

fmt

5cb427e

KKould merged commit eb9ab5a into KipData:main Dec 5, 2023
3 checks passed

KKould mentioned this pull request Dec 16, 2023

DDL Support #97

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alter table #104

Alter table #104

guojidan commented Nov 27, 2023

KKould Nov 28, 2023

KKould Nov 28, 2023

guojidan Nov 28, 2023

KKould Nov 28, 2023

KKould Nov 28, 2023

KKould Nov 28, 2023

KKould Nov 28, 2023

guojidan Nov 28, 2023

KKould Nov 30, 2023

KKould Dec 4, 2023 •

edited

Loading

KKould Dec 4, 2023

KKould Dec 4, 2023

KKould Dec 4, 2023

KKould Dec 4, 2023

KKould Dec 4, 2023

KKould Dec 4, 2023

KKould commented Dec 5, 2023

		@@ -0,0 +1,19 @@
		statement ok
		create table alter_table(id int primary key, v1 int)

Alter table #104

Alter table #104

Conversation

guojidan commented Nov 27, 2023

What problem does this PR solve?

What is changed and how it works?

Code changes

Check List

Note for reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKould Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKould commented Dec 5, 2023

KKould Dec 4, 2023 •

edited

Loading