Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Modify Column Type #3701

Closed
wants to merge 5 commits into from
Closed

feat: Modify Column Type #3701

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f725658
feat: support Modify Column like postgresql, e.g. `ALTER TABLE t1 ALT…
KKould Apr 13, 2024
730bc94
test: unit/e2e test for ModifyColumn
KKould Apr 14, 2024
de0dfee
fix: unit test error fix
KKould Apr 14, 2024
21d6d16
test: more failed cases for `modify_col.sql`
KKould Apr 14, 2024
cb0884a
chore: fix code
KKould Apr 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ etcd-client = "0.12"
fst = "0.4.7"
futures = "0.3"
futures-util = "0.3"
greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "b97efbf92a0bf9abcfa1d8fe0ffe8741a2e7309e" }
greptime-proto = { git = "https://github.com/KKould/greptime-proto.git", rev = "a47b66e1d2010037fad2fbe45b2c1231b6ec33df" }
humantime = "2.1"
humantime-serde = "1.1"
itertools = "0.10"
Expand Down
62 changes: 59 additions & 3 deletions src/common/grpc-expr/src/alter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,18 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use api::helper::ColumnDataTypeWrapper;
use api::v1::add_column_location::LocationType;
use api::v1::alter_expr::Kind;
use api::v1::{
column_def, AddColumnLocation as Location, AlterExpr, CreateTableExpr, DropColumns,
RenameTable, SemanticType,
ModifyColumns, RenameTable, SemanticType,
};
use common_query::AddColumnLocation;
use datatypes::schema::{ColumnSchema, RawSchema};
use snafu::{ensure, OptionExt, ResultExt};
use table::metadata::TableId;
use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest};
use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest, ModifyColumnRequest};

use crate::error::{
InvalidColumnDefSnafu, MissingFieldSnafu, MissingTimestampColumnSnafu, Result,
Expand Down Expand Up @@ -67,6 +68,25 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result<Alter
Kind::DropColumns(DropColumns { drop_columns }) => AlterKind::DropColumns {
names: drop_columns.into_iter().map(|c| c.name).collect(),
},
Kind::ModifyColumns(ModifyColumns { modify_columns }) => {
let modify_column_requests = modify_columns
.into_iter()
.map(|mc| {
let target_type =
ColumnDataTypeWrapper::new(mc.target_type(), mc.target_type_extension)
.into();

Ok(ModifyColumnRequest {
column_name: mc.column_name,
target_type,
})
})
.collect::<Result<Vec<_>>>()?;

AlterKind::ModifyColumns {
columns: modify_column_requests,
}
}
Kind::RenameTable(RenameTable { new_table_name }) => {
AlterKind::RenameTable { new_table_name }
}
Expand Down Expand Up @@ -137,7 +157,9 @@ fn parse_location(location: Option<Location>) -> Result<Option<AddColumnLocation

#[cfg(test)]
mod tests {
use api::v1::{AddColumn, AddColumns, ColumnDataType, ColumnDef, DropColumn, SemanticType};
use api::v1::{
AddColumn, AddColumns, ColumnDataType, ColumnDef, DropColumn, ModifyColumn, SemanticType,
};
use datatypes::prelude::ConcreteDataType;

use super::*;
Expand Down Expand Up @@ -286,4 +308,38 @@ mod tests {
assert_eq!(1, drop_names.len());
assert_eq!("mem_usage".to_string(), drop_names.pop().unwrap());
}

#[test]
fn test_modify_column_expr() {
let expr = AlterExpr {
catalog_name: "test_catalog".to_string(),
schema_name: "test_schema".to_string(),
table_name: "monitor".to_string(),

kind: Some(Kind::ModifyColumns(ModifyColumns {
modify_columns: vec![ModifyColumn {
column_name: "mem_usage".to_string(),
target_type: ColumnDataType::String as i32,
target_type_extension: None,
}],
})),
};

let alter_request = alter_expr_to_request(1, expr).unwrap();
assert_eq!(alter_request.catalog_name, "test_catalog");
assert_eq!(alter_request.schema_name, "test_schema");
assert_eq!("monitor".to_string(), alter_request.table_name);

let mut modify_columns = match alter_request.alter_kind {
AlterKind::ModifyColumns { columns } => columns,
_ => unreachable!(),
};

let modify_column = modify_columns.pop().unwrap();
assert_eq!("mem_usage", modify_column.column_name);
assert_eq!(
ConcreteDataType::string_datatype(),
modify_column.target_type
);
}
}
170 changes: 116 additions & 54 deletions src/common/meta/src/ddl/alter_table/region_request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
use api::v1::alter_expr::Kind;
use api::v1::region::region_request::Body;
use api::v1::region::{
alter_request, AddColumn, AddColumns, AlterRequest, DropColumn, DropColumns, RegionColumnDef,
RegionRequest, RegionRequestHeader,
alter_request, AddColumn, AddColumns, AlterRequest, DropColumn, DropColumns, ModifyColumn,
ModifyColumns, RegionColumnDef, RegionRequest, RegionRequestHeader,
};
use common_telemetry::tracing_context::TracingContext;
use snafu::OptionExt;
Expand Down Expand Up @@ -104,6 +104,21 @@ fn create_proto_alter_kind(
drop_columns,
})))
}
Kind::ModifyColumns(x) => {
let modify_columns = x
.modify_columns
.iter()
.map(|modify_column| ModifyColumn {
column_name: modify_column.column_name.clone(),
target_type: modify_column.target_type,
target_type_extension: modify_column.target_type_extension.clone(),
})
.collect();

Ok(Some(alter_request::Kind::ModifyColumns(ModifyColumns {
modify_columns,
})))
}
Kind::RenameTable(_) => Ok(None),
}
}
Expand All @@ -119,7 +134,7 @@ mod tests {
use api::v1::region::RegionColumnDef;
use api::v1::{
region, AddColumn, AddColumnLocation, AddColumns, AlterExpr, ColumnDataType,
ColumnDef as PbColumnDef, SemanticType,
ColumnDef as PbColumnDef, ModifyColumn, ModifyColumns, SemanticType,
};
use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME};
use store_api::storage::RegionId;
Expand Down Expand Up @@ -194,47 +209,14 @@ mod tests {
.await
.unwrap();

let task = AlterTableTask {
alter_table: AlterExpr {
catalog_name: DEFAULT_CATALOG_NAME.to_string(),
schema_name: DEFAULT_SCHEMA_NAME.to_string(),
table_name: table_name.to_string(),
kind: Some(Kind::AddColumns(AddColumns {
add_columns: vec![AddColumn {
column_def: Some(PbColumnDef {
name: "my_tag3".to_string(),
data_type: ColumnDataType::String as i32,
is_nullable: true,
default_constraint: b"hello".to_vec(),
semantic_type: SemanticType::Tag as i32,
comment: String::new(),
..Default::default()
}),
location: Some(AddColumnLocation {
location_type: LocationType::After as i32,
after_column_name: "my_tag2".to_string(),
}),
}],
})),
},
};

let mut procedure =
AlterTableProcedure::new(cluster_id, table_id, task, ddl_context).unwrap();
procedure.on_prepare().await.unwrap();
let Some(Body::Alter(alter_region_request)) =
procedure.make_alter_region_request(region_id).unwrap().body
else {
unreachable!()
};
assert_eq!(alter_region_request.region_id, region_id.as_u64());
assert_eq!(alter_region_request.schema_version, 1);
assert_eq!(
alter_region_request.kind,
Some(region::alter_request::Kind::AddColumns(
region::AddColumns {
add_columns: vec![region::AddColumn {
column_def: Some(RegionColumnDef {
{
let add_column_task = AlterTableTask {
alter_table: AlterExpr {
catalog_name: DEFAULT_CATALOG_NAME.to_string(),
schema_name: DEFAULT_SCHEMA_NAME.to_string(),
table_name: table_name.to_string(),
kind: Some(Kind::AddColumns(AddColumns {
add_columns: vec![AddColumn {
column_def: Some(PbColumnDef {
name: "my_tag3".to_string(),
data_type: ColumnDataType::String as i32,
Expand All @@ -244,15 +226,95 @@ mod tests {
comment: String::new(),
..Default::default()
}),
column_id: 3,
}),
location: Some(AddColumnLocation {
location_type: LocationType::After as i32,
after_column_name: "my_tag2".to_string(),
}),
}]
}
))
);
location: Some(AddColumnLocation {
location_type: LocationType::After as i32,
after_column_name: "my_tag2".to_string(),
}),
}],
})),
},
};

let mut procedure = AlterTableProcedure::new(
cluster_id,
table_id,
add_column_task,
ddl_context.clone(),
)
.unwrap();
procedure.on_prepare().await.unwrap();
let Some(Body::Alter(alter_region_request)) =
procedure.make_alter_region_request(region_id).unwrap().body
else {
unreachable!()
};
assert_eq!(alter_region_request.region_id, region_id.as_u64());
assert_eq!(alter_region_request.schema_version, 1);
assert_eq!(
alter_region_request.kind,
Some(region::alter_request::Kind::AddColumns(
region::AddColumns {
add_columns: vec![region::AddColumn {
column_def: Some(RegionColumnDef {
column_def: Some(PbColumnDef {
name: "my_tag3".to_string(),
data_type: ColumnDataType::String as i32,
is_nullable: true,
default_constraint: b"hello".to_vec(),
semantic_type: SemanticType::Tag as i32,
comment: String::new(),
..Default::default()
}),
column_id: 3,
}),
location: Some(AddColumnLocation {
location_type: LocationType::After as i32,
after_column_name: "my_tag2".to_string(),
}),
}]
}
))
);
}
{
let modify_column_task = AlterTableTask {
alter_table: AlterExpr {
catalog_name: DEFAULT_CATALOG_NAME.to_string(),
schema_name: DEFAULT_SCHEMA_NAME.to_string(),
table_name: table_name.to_string(),
kind: Some(Kind::ModifyColumns(ModifyColumns {
modify_columns: vec![ModifyColumn {
column_name: "cpu".to_string(),
target_type: ColumnDataType::String as i32,
target_type_extension: None,
}],
})),
},
};

let mut procedure =
AlterTableProcedure::new(cluster_id, table_id, modify_column_task, ddl_context)
.unwrap();
procedure.on_prepare().await.unwrap();
let Some(Body::Alter(alter_region_request)) =
procedure.make_alter_region_request(region_id).unwrap().body
else {
unreachable!()
};
assert_eq!(alter_region_request.region_id, region_id.as_u64());
assert_eq!(alter_region_request.schema_version, 1);
assert_eq!(
alter_region_request.kind,
Some(region::alter_request::Kind::ModifyColumns(
region::ModifyColumns {
modify_columns: vec![region::ModifyColumn {
column_name: "cpu".to_string(),
target_type: ColumnDataType::String as i32,
target_type_extension: None,
}]
}
))
);
}
}
}
2 changes: 1 addition & 1 deletion src/common/meta/src/ddl/alter_table/update_metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl AlterTableProcedure {
AlterKind::RenameTable { new_table_name } => {
new_info.name = new_table_name.to_string();
}
AlterKind::DropColumns { .. } => {}
AlterKind::DropColumns { .. } | AlterKind::ModifyColumns { .. } => {}
}

Ok(new_info)
Expand Down
Loading