Skip to content

Commit 067c603

Browse files
committed
refactor(ddl): destructure Column for clarity + future-proofing in comment-only check
Address review feedback from @callicles: - Replace clone-and-normalise trick with exhaustive destructuring. Adding a new Column field is now a compile error rather than silently joining the "no drop" set. - Lead the doc with the rule (drop required unless only `comment` differs), demote ClickHouse rationale below. No behaviour change; all 50 ddl_ordering tests still pass.
1 parent 2419c50 commit 067c603

1 file changed

Lines changed: 49 additions & 12 deletions

File tree

apps/framework-cli/src/infrastructure/olap/ddl_ordering.rs

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1348,21 +1348,58 @@ struct HandledDependencies {
13481348
readded_projections: HashSet<String>,
13491349
}
13501350

1351-
/// Returns true if the diff between `before` and `after` requires dependent
1352-
/// indexes/projections to be dropped before `ALTER TABLE ... MODIFY COLUMN`.
1351+
/// Returns `true` if `before` and `after` differ in any field other than `comment`.
13531352
///
1354-
/// ClickHouse rejects `MODIFY COLUMN` on an indexed column only when the
1355-
/// modification changes on-disk layout or evaluated values (e.g. `data_type`,
1356-
/// `default`, `codec`, `materialized`). A pure `comment` change is accepted
1357-
/// without dropping the dependent, and re-creating the index would force
1358-
/// ClickHouse to rebuild it lazily on merges, degrading data-skipping until then.
1353+
/// `ALTER TABLE ... MODIFY COLUMN` is rejected by ClickHouse on an indexed
1354+
/// column whenever the modification changes on-disk layout or evaluated values
1355+
/// (`data_type`, `default`, `codec`, `materialized`, etc.). A pure `comment`
1356+
/// change is the one case ClickHouse accepts without dropping dependent
1357+
/// indexes/projections — and re-creating those would force a lazy index
1358+
/// rebuild on merges, degrading data-skipping on existing parts until then.
13591359
///
1360-
/// Conservatively, any field other than `comment` differing forces the drop,
1361-
/// so new `Column` fields default to the safe behaviour.
1360+
/// Implemented via exhaustive destructuring so that adding a new `Column`
1361+
/// field is a compile error: the author must explicitly decide whether the
1362+
/// field belongs in the "requires drop" set.
13621363
fn column_modify_requires_dependent_drop(before: &Column, after: &Column) -> bool {
1363-
let mut normalised = before.clone();
1364-
normalised.comment = after.comment.clone();
1365-
normalised != *after
1364+
let Column {
1365+
name: _,
1366+
comment: _, // metadata-only; ClickHouse accepts the MODIFY without dropping dependents
1367+
data_type: b_data_type,
1368+
required: b_required,
1369+
unique: b_unique,
1370+
primary_key: b_primary_key,
1371+
default: b_default,
1372+
annotations: b_annotations,
1373+
ttl: b_ttl,
1374+
codec: b_codec,
1375+
materialized: b_materialized,
1376+
alias: b_alias,
1377+
} = before;
1378+
let Column {
1379+
name: _,
1380+
comment: _,
1381+
data_type: a_data_type,
1382+
required: a_required,
1383+
unique: a_unique,
1384+
primary_key: a_primary_key,
1385+
default: a_default,
1386+
annotations: a_annotations,
1387+
ttl: a_ttl,
1388+
codec: a_codec,
1389+
materialized: a_materialized,
1390+
alias: a_alias,
1391+
} = after;
1392+
1393+
b_data_type != a_data_type
1394+
|| b_required != a_required
1395+
|| b_unique != a_unique
1396+
|| b_primary_key != a_primary_key
1397+
|| b_default != a_default
1398+
|| b_annotations != a_annotations
1399+
|| b_ttl != a_ttl
1400+
|| b_codec != a_codec
1401+
|| b_materialized != a_materialized
1402+
|| b_alias != a_alias
13661403
}
13671404

13681405
/// Emit drop + re-add ops for every index/projection that references `column_name`.

0 commit comments

Comments
 (0)