diff --git a/crates/dbx-core/src/table_structure_sql/columns.rs b/crates/dbx-core/src/table_structure_sql/columns.rs index 4d2b9f489c..ac3e7c8a54 100644 --- a/crates/dbx-core/src/table_structure_sql/columns.rs +++ b/crates/dbx-core/src/table_structure_sql/columns.rs @@ -20,6 +20,8 @@ pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mu let database_label = database_label(options.database_type); let active_columns: Vec<_> = options.columns.iter().filter(|column| !column.marked_for_drop).collect(); let has_original_column_positions = active_columns.iter().any(|column| column.original_position.is_some()); + let mut simulated_column_order = + if has_original_column_positions { original_active_column_order(&active_columns) } else { Vec::new() }; let mut statements = Vec::new(); for column in &options.columns { @@ -49,10 +51,12 @@ pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mu } else { String::new() }; + let desired_previous_column_id = active_previous_column_id(&active_columns, active_index); let has_position_change = has_original_column_positions && matches!(dialect, StructureDialect::Mysql | StructureDialect::ClickHouse) && column.original.is_some() - && mysql_column_position_changed(&active_columns, active_index); + && column.original_position.is_some() + && simulated_column_position_changed(&simulated_column_order, &column.id, desired_previous_column_id); if column.original.is_none() { if !capabilities.add_column { @@ -67,6 +71,11 @@ pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mu options.schema.as_deref(), &options.table_name, )); + if has_original_column_positions + && matches!(dialect, StructureDialect::Mysql | StructureDialect::ClickHouse) + { + apply_simulated_column_position(&mut simulated_column_order, &column.id, desired_previous_column_id); + } continue; } @@ -123,6 +132,9 @@ pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mu StructureDialect::Questdb => statements.extend(build_questdb_existing_column_sql(&table, column)), _ => warnings.push(format!("Editing existing columns is not supported for {database_label} yet.")), } + if has_position_change { + apply_simulated_column_position(&mut simulated_column_order, &column.id, desired_previous_column_id); + } } // Emit primary key constraint changes after individual column changes @@ -241,40 +253,46 @@ pub(super) fn column_position_clause( format!(" AFTER {}", quote_ident(dialect, columns.get(index - 1).map(|column| column.name.as_str()).unwrap_or(""))) } -pub(super) fn mysql_column_position_changed(columns: &[&EditableStructureColumn], index: usize) -> bool { - let Some(column) = columns.get(index) else { +pub(super) fn original_active_column_order(columns: &[&EditableStructureColumn]) -> Vec { + let mut original_columns: Vec<_> = columns + .iter() + .filter(|column| column.original.is_some() && column.original_position.is_some()) + .copied() + .collect(); + original_columns.sort_by_key(|column| column.original_position.unwrap_or(0)); + original_columns.into_iter().map(|column| column.id.clone()).collect() +} + +pub(super) fn active_previous_column_id<'a>(columns: &[&'a EditableStructureColumn], index: usize) -> Option<&'a str> { + if index == 0 { + None + } else { + columns.get(index - 1).map(|column| column.id.as_str()) + } +} + +pub(super) fn simulated_column_position_changed( + simulated_column_order: &[String], + column_id: &str, + desired_previous_column_id: Option<&str>, +) -> bool { + let Some(index) = simulated_column_order.iter().position(|id| id == column_id) else { return false; }; - if column.original.is_none() || column.original_position.is_none() { - return false; - } - current_previous_original_column_name(columns, index) != original_previous_column_name(columns, column) + let current_previous_column_id = if index == 0 { None } else { Some(simulated_column_order[index - 1].as_str()) }; + current_previous_column_id != desired_previous_column_id } -pub(super) fn original_previous_column_name( - columns: &[&EditableStructureColumn], - column: &EditableStructureColumn, -) -> Option { - let mut original_columns: Vec<_> = - columns.iter().filter(|item| item.original.is_some() && item.original_position.is_some()).copied().collect(); - original_columns.sort_by_key(|item| item.original_position.unwrap_or(0)); - let index = original_columns.iter().position(|item| item.id == column.id)?; - if index == 0 { - None - } else { - original_columns[index - 1].original.as_ref().map(|original| original.name.clone()) - } -} - -pub(super) fn current_previous_original_column_name( - columns: &[&EditableStructureColumn], - index: usize, -) -> Option { - if index == 0 { - None - } else { - columns.get(index - 1).map(|column| { - column.original.as_ref().map(|original| original.name.clone()).unwrap_or_else(|| column.name.clone()) - }) +pub(super) fn apply_simulated_column_position( + simulated_column_order: &mut Vec, + column_id: &str, + desired_previous_column_id: Option<&str>, +) { + if let Some(index) = simulated_column_order.iter().position(|id| id == column_id) { + simulated_column_order.remove(index); } + let index = desired_previous_column_id + .and_then(|previous_id| simulated_column_order.iter().position(|id| id == previous_id).map(|index| index + 1)) + .unwrap_or(0); + simulated_column_order.insert(index, column_id.to_string()); } diff --git a/crates/dbx-core/src/table_structure_sql/tests.rs b/crates/dbx-core/src/table_structure_sql/tests.rs index 889b942448..e5e1b9a954 100644 --- a/crates/dbx-core/src/table_structure_sql/tests.rs +++ b/crates/dbx-core/src/table_structure_sql/tests.rs @@ -802,11 +802,116 @@ fn builds_mysql_column_reorder_statements() { result.statements, vec![ "ALTER TABLE `users` MODIFY COLUMN `email` varchar(255) AFTER `id`;", - "ALTER TABLE `users` CHANGE COLUMN `name` `display_name` varchar(120) AFTER `email`;", + "ALTER TABLE `users` CHANGE COLUMN `name` `display_name` varchar(120);", ] ); } +#[test] +fn mysql_add_column_before_existing_column_does_not_reorder_shifted_column() { + let mut deleted = column("deleted"); + deleted.original_position = Some(0); + deleted.original = Some(ColumnInfo { + name: "deleted".to_string(), + data_type: "varchar(255)".to_string(), + is_nullable: true, + column_default: None, + is_primary_key: false, + extra: None, + comment: None, + }); + + let new_column = column("sss"); + + let mut tenant_id = column("tenant_id"); + tenant_id.data_type = "bigint".to_string(); + tenant_id.is_nullable = false; + tenant_id.default_value = "0".to_string(); + tenant_id.comment = "tenant id".to_string(); + tenant_id.original_position = Some(1); + tenant_id.original = Some(ColumnInfo { + name: "tenant_id".to_string(), + data_type: "bigint".to_string(), + is_nullable: false, + column_default: Some("0".to_string()), + is_primary_key: false, + extra: None, + comment: Some("tenant id".to_string()), + }); + + let result = build_table_structure_change_sql(TableStructureSqlOptions { + database_type: Some(DatabaseType::Mysql), + schema: None, + table_name: "infra_api_error_log".to_string(), + columns: vec![deleted, new_column, tenant_id], + indexes: Vec::new(), + foreign_keys: Vec::new(), + triggers: Vec::new(), + table_comment: None, + original_table_comment: None, + }); + + assert_eq!(result.warnings, Vec::::new()); + assert_eq!( + result.statements, + vec!["ALTER TABLE `infra_api_error_log` ADD COLUMN `sss` varchar(255) AFTER `deleted`;"] + ); +} + +#[test] +fn mysql_existing_column_reorder_does_not_reorder_columns_shifted_by_prior_move() { + let mut id = column("id"); + id.original_position = Some(0); + id.original = Some(ColumnInfo { + name: "id".to_string(), + data_type: "varchar(255)".to_string(), + is_nullable: true, + column_default: None, + is_primary_key: false, + extra: None, + comment: None, + }); + + let mut name = column("name"); + name.original_position = Some(1); + name.original = Some(ColumnInfo { + name: "name".to_string(), + data_type: "varchar(255)".to_string(), + is_nullable: true, + column_default: None, + is_primary_key: false, + extra: None, + comment: None, + }); + + let mut email = column("email"); + email.original_position = Some(2); + email.original = Some(ColumnInfo { + name: "email".to_string(), + data_type: "varchar(255)".to_string(), + is_nullable: true, + column_default: None, + is_primary_key: false, + extra: None, + comment: None, + }); + + let result = build_table_structure_change_sql(TableStructureSqlOptions { + database_type: Some(DatabaseType::Mysql), + schema: None, + table_name: "users".to_string(), + columns: vec![id, email, name], + indexes: Vec::new(), + foreign_keys: Vec::new(), + triggers: Vec::new(), + table_comment: None, + original_table_comment: None, + }); + + assert_eq!(result.warnings, Vec::::new()); + assert_eq!(result.statements, vec!["ALTER TABLE `users` MODIFY COLUMN `email` varchar(255) AFTER `id`;"]); +} + #[test] fn builds_sql_server_quoted_column_and_index_statements() { let mut email = column("email");