diff --git a/crates/dbx-core/src/table_structure_sql/columns.rs b/crates/dbx-core/src/table_structure_sql/columns.rs index 49a8518bd..6f7ec5578 100644 --- a/crates/dbx-core/src/table_structure_sql/columns.rs +++ b/crates/dbx-core/src/table_structure_sql/columns.rs @@ -12,6 +12,7 @@ use super::util::{ clean, is_protected_manticore_id_column, normalize_default, original_comment, original_default, qualified_table, quote_ident, quote_string, }; +use std::collections::HashSet; pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mut Vec) -> Vec { let capabilities = capabilities_for(options.database_type); @@ -22,6 +23,15 @@ pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mu 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() }; + // Pre-compute the minimal set of existing columns that really need an explicit move. + // For MySQL/ClickHouse we keep the largest already-ordered subset in place and only + // emit FIRST/AFTER SQL for columns outside that subset. + let reordered_existing_column_ids = + if has_original_column_positions && matches!(dialect, StructureDialect::Mysql | StructureDialect::ClickHouse) { + planned_existing_column_move_ids(&active_columns) + } else { + HashSet::new() + }; let mut statements = Vec::new(); for column in &options.columns { @@ -52,8 +62,11 @@ pub(super) fn build_column_sql(options: &TableStructureSqlOptions, warnings: &mu String::new() }; let desired_previous_column_id = active_previous_column_id(&active_columns, active_index); + // A position change only matters when this column is part of the planned move set + // and its predecessor still differs in the simulated order. let has_position_change = has_original_column_positions && matches!(dialect, StructureDialect::Mysql | StructureDialect::ClickHouse) + && reordered_existing_column_ids.contains(&column.id) && column.original.is_some() && column.original_position.is_some() && simulated_column_position_changed(&simulated_column_order, &column.id, desired_previous_column_id); @@ -314,6 +327,79 @@ pub(super) fn original_active_column_order(columns: &[&EditableStructureColumn]) original_columns.into_iter().map(|column| column.id.clone()).collect() } +/// Returns the ids of existing columns that must be explicitly moved to reach the target order. +/// +/// The function keeps the longest subsequence of existing columns whose relative order is already +/// correct, and marks only the remaining columns for FIRST/AFTER reordering SQL. +pub(super) fn planned_existing_column_move_ids(columns: &[&EditableStructureColumn]) -> HashSet { + // Only existing columns with an original position participate in move planning. + // Newly added columns are positioned directly from the target order. + let reorderable_columns: Vec<_> = columns + .iter() + .filter_map(|column| { + column + .original + .as_ref() + .zip(column.original_position) + .map(|_| (column.id.as_str(), column.original_position.unwrap_or(0))) + }) + .collect(); + if reorderable_columns.len() < 2 { + return HashSet::new(); + } + + // Map the target order back to original positions, then keep the largest increasing subsequence. + let original_positions: Vec<_> = reorderable_columns.iter().map(|(_, position)| *position).collect(); + // Columns inside the LIS can stay where they are; everything else needs an explicit move. + let kept_indices: HashSet<_> = longest_increasing_subsequence_indices(&original_positions).into_iter().collect(); + + reorderable_columns + .into_iter() + .enumerate() + .filter(|(index, _)| !kept_indices.contains(index)) + .map(|(_, (column_id, _))| column_id.to_string()) + .collect() +} + +/// Returns the indices of one longest increasing subsequence within `values`. +/// +/// In the reorder planner, an increasing subsequence represents existing columns whose relative +/// order still matches the original table layout, so they can remain untouched. +fn longest_increasing_subsequence_indices(values: &[usize]) -> Vec { + if values.is_empty() { + return Vec::new(); + } + + // O(n^2) is sufficient here because table editors deal with relatively small column counts + // and the simpler implementation is easier to maintain. + let mut lengths = vec![1; values.len()]; + let mut previous = vec![None; values.len()]; + let mut best_end_index = 0; + + for current_index in 0..values.len() { + for previous_index in 0..current_index { + if values[previous_index] < values[current_index] && lengths[previous_index] + 1 > lengths[current_index] { + lengths[current_index] = lengths[previous_index] + 1; + previous[current_index] = Some(previous_index); + } + } + + if lengths[current_index] > lengths[best_end_index] { + best_end_index = current_index; + } + } + + // Reconstruct the subsequence by following the predecessor chain backwards. + let mut indices = Vec::new(); + let mut cursor = Some(best_end_index); + while let Some(index) = cursor { + indices.push(index); + cursor = previous[index]; + } + indices.reverse(); + indices +} + pub(super) fn active_previous_column_id<'a>(columns: &[&'a EditableStructureColumn], index: usize) -> Option<&'a str> { if index == 0 { None diff --git a/crates/dbx-core/src/table_structure_sql/tests.rs b/crates/dbx-core/src/table_structure_sql/tests.rs index e7085c12e..e9fe92e72 100644 --- a/crates/dbx-core/src/table_structure_sql/tests.rs +++ b/crates/dbx-core/src/table_structure_sql/tests.rs @@ -107,14 +107,14 @@ fn builds_mysql_column_and_index_changes() { assert_eq!(result.warnings, Vec::::new()); assert_eq!( - result.statements, - vec![ - "ALTER TABLE `users` CHANGE COLUMN `name` `display_name` varchar(120) NOT NULL DEFAULT 'guest' COMMENT 'Shown name';", - "ALTER TABLE `users` ADD COLUMN `email` varchar(255) NOT NULL;", - "DROP INDEX `idx_old` ON `users`;", - "CREATE UNIQUE INDEX `uniq_users_email` ON `users` (`email`);", - ] - ); + result.statements, + vec![ + "ALTER TABLE `users` CHANGE COLUMN `name` `display_name` varchar(120) NOT NULL DEFAULT 'guest' COMMENT 'Shown name';", + "ALTER TABLE `users` ADD COLUMN `email` varchar(255) NOT NULL;", + "DROP INDEX `idx_old` ON `users`;", + "CREATE UNIQUE INDEX `uniq_users_email` ON `users` (`email`);", + ] + ); } #[test] @@ -490,7 +490,7 @@ fn gbase8a_allows_mysql_style_column_reorder() { }); assert_eq!(result.warnings, Vec::::new()); - assert_eq!(result.statements, vec!["ALTER TABLE `users` MODIFY COLUMN `email` varchar(255) AFTER `id`;"]); + assert_eq!(result.statements, vec!["ALTER TABLE `users` MODIFY COLUMN `name` varchar(255) AFTER `email`;"]); } #[test] @@ -935,10 +935,7 @@ fn builds_mysql_column_reorder_statements() { assert_eq!(result.warnings, Vec::::new()); assert_eq!( result.statements, - vec![ - "ALTER TABLE `users` MODIFY COLUMN `email` varchar(255) AFTER `id`;", - "ALTER TABLE `users` CHANGE COLUMN `name` `display_name` varchar(120);", - ] + vec!["ALTER TABLE `users` CHANGE COLUMN `name` `display_name` varchar(120) AFTER `email`;"] ); } @@ -1044,7 +1041,75 @@ fn mysql_existing_column_reorder_does_not_reorder_columns_shifted_by_prior_move( }); assert_eq!(result.warnings, Vec::::new()); - assert_eq!(result.statements, vec!["ALTER TABLE `users` MODIFY COLUMN `email` varchar(255) AFTER `id`;"]); + assert_eq!(result.statements, vec!["ALTER TABLE `users` MODIFY COLUMN `name` varchar(255) AFTER `email`;"]); +} + +#[test] +fn mysql_moving_first_column_to_end_uses_single_reorder_statement() { + let mut col_0 = column("col_0"); + col_0.data_type = "int(11)".to_string(); + col_0.is_nullable = false; + col_0.original_position = Some(0); + col_0.original = Some(ColumnInfo { + name: "col_0".to_string(), + data_type: "int(11)".to_string(), + is_nullable: false, + column_default: None, + is_primary_key: false, + extra: None, + comment: None, + }); + + let mut col_1 = column("col_1"); + col_1.original_position = Some(1); + col_1.original = Some(ColumnInfo { + name: "col_1".to_string(), + data_type: "varchar(255)".to_string(), + is_nullable: true, + column_default: None, + is_primary_key: false, + extra: None, + comment: None, + }); + + let mut col_2 = column("col_2"); + col_2.original_position = Some(2); + col_2.original = Some(ColumnInfo { + name: "col_2".to_string(), + data_type: "varchar(255)".to_string(), + is_nullable: true, + column_default: None, + is_primary_key: false, + extra: None, + comment: None, + }); + + let mut col_3 = column("col_3"); + col_3.original_position = Some(3); + col_3.original = Some(ColumnInfo { + name: "col_3".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![col_1, col_2, col_3, col_0], + 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 `col_0` int(11) NOT NULL AFTER `col_3`;"]); } #[test] @@ -1381,12 +1446,12 @@ fn builds_duckdb_create_table_statements() { assert_eq!(result.warnings, Vec::::new()); assert_eq!( - result.statements, - vec![ - "CREATE TABLE \"events\" (\n \"name\" VARCHAR NOT NULL,\n \"created_at\" TIMESTAMP DEFAULT current_timestamp\n);", - "CREATE INDEX \"idx_events_name\" ON \"events\" (\"name\");", - ] - ); + result.statements, + vec![ + "CREATE TABLE \"events\" (\n \"name\" VARCHAR NOT NULL,\n \"created_at\" TIMESTAMP DEFAULT current_timestamp\n);", + "CREATE INDEX \"idx_events_name\" ON \"events\" (\"name\");", + ] + ); } #[test]