diff --git a/crates/omnigraph-compiler/src/catalog/schema_plan.rs b/crates/omnigraph-compiler/src/catalog/schema_plan.rs index 08fe16a..a20820d 100644 --- a/crates/omnigraph-compiler/src/catalog/schema_plan.rs +++ b/crates/omnigraph-compiler/src/catalog/schema_plan.rs @@ -324,13 +324,18 @@ fn plan_nodes( .iter() .filter(|node| !consumed.contains(&node.name)) { - steps.push(SchemaMigrationStep::UnsupportedChange { - entity: format!("node:{}", leftover.name), - reason: format!( - "removing node type '{}' is not supported in schema migration v1", - leftover.name - ), - code: Some(crate::lint::codes::OG_DS_102.code.to_string()), + // Node type removed from the desired schema: emit + // DropType { Node, Soft } per docs/dev/schema-lint-v1-plan.md + // commit #4. Soft = remove the table's entry from the current + // __manifest version; data files retained; previous manifest + // versions still reference the table, so Lance time travel + // restores it until cleanup_old_versions ages out the older + // __manifest entries. Hard mode (immediate dataset deletion) + // lands in commit #5 gated by --allow-data-loss. + steps.push(SchemaMigrationStep::DropType { + type_kind: SchemaTypeKind::Node, + name: leftover.name.clone(), + mode: DropMode::Soft, }); } @@ -442,13 +447,15 @@ fn plan_edges( .iter() .filter(|edge| !consumed.contains(&edge.name)) { - steps.push(SchemaMigrationStep::UnsupportedChange { - entity: format!("edge:{}", leftover.name), - reason: format!( - "removing edge type '{}' is not supported in schema migration v1", - leftover.name - ), - code: Some(crate::lint::codes::OG_DS_103.code.to_string()), + // Edge type removed from the desired schema: emit + // DropType { Edge, Soft } per docs/dev/schema-lint-v1-plan.md + // commit #4. Same Soft mechanics as node-type drops — manifest + // entry tombstoned, data files retained, reversible via Lance + // time travel until cleanup. + steps.push(SchemaMigrationStep::DropType { + type_kind: SchemaTypeKind::Edge, + name: leftover.name.clone(), + mode: DropMode::Soft, }); } } @@ -991,6 +998,81 @@ node Person { ); } + #[test] + fn plan_emits_soft_drop_for_removed_node_and_edge_types() { + // Removing a node type + the edge type that references it + // emits two DropType { Soft } steps (chassis v1 commit #4, + // MR-694). The plan is `supported = true` — apply tombstones + // both manifest entries. Time-travel reversibility is verified + // at the integration level by + // `apply_schema_drops_node_and_referencing_edge_softly` + // in `crates/omnigraph/tests/schema_apply.rs`. + let accepted = build_schema_ir( + &parse_schema( + r#" +node Person { + name: String @key +} + +node Company { + name: String @key +} + +edge WorksAt: Person -> Company +"#, + ) + .unwrap(), + ) + .unwrap(); + let desired = build_schema_ir( + &parse_schema( + r#" +node Person { + name: String @key +} +"#, + ) + .unwrap(), + ) + .unwrap(); + + let plan = plan_schema_migration(&accepted, &desired).unwrap(); + assert!( + plan.supported, + "drop-type plan must be supported: {plan:?}" + ); + assert!( + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::DropType { + type_kind: SchemaTypeKind::Node, + name, + mode: DropMode::Soft, + } if name == "Company" + )), + "expected DropType {{ Node, Company, Soft }} in plan: {plan:?}", + ); + assert!( + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::DropType { + type_kind: SchemaTypeKind::Edge, + name, + mode: DropMode::Soft, + } if name == "WorksAt" + )), + "expected DropType {{ Edge, WorksAt, Soft }} in plan: {plan:?}", + ); + // Negative: no UnsupportedChange anywhere in the plan. + assert!( + !plan + .steps + .iter() + .any(|step| matches!(step, UnsupportedChange { .. })), + "soft type drop must not emit UnsupportedChange: {plan:?}", + ); + } + #[test] fn plan_rejects_required_property_addition() { let accepted = build_schema_ir( diff --git a/crates/omnigraph/src/db/omnigraph/schema_apply.rs b/crates/omnigraph/src/db/omnigraph/schema_apply.rs index f764e22..bdc5e5c 100644 --- a/crates/omnigraph/src/db/omnigraph/schema_apply.rs +++ b/crates/omnigraph/src/db/omnigraph/schema_apply.rs @@ -78,6 +78,7 @@ pub(super) async fn apply_schema_with_lock( let mut renamed_tables = HashMap::new(); let mut rewritten_tables = BTreeSet::new(); let mut indexed_tables = BTreeSet::new(); + let mut dropped_tables = BTreeSet::new(); let mut property_renames = HashMap::>::new(); let mut changed_edge_tables = false; @@ -165,13 +166,31 @@ pub(super) async fn apply_schema_with_lock( } rewritten_tables.insert(table_key); } - SchemaMigrationStep::DropType { .. } => { - // DropType (whole-table drop via __manifest entry - // removal) lands in commit #4 — different mechanics - // from DropProperty. - return Err(OmniError::manifest_internal( - "DropType not yet implemented (commit #4)", - )); + SchemaMigrationStep::DropType { + type_kind, + name, + mode, + } => { + // Soft = remove the table's entry from the current + // __manifest version via a tombstone. The Lance + // dataset files are retained — prior __manifest + // versions still reference them, so Lance time + // travel + branch-from-snapshot can read the dropped + // table until `omnigraph cleanup` ages out the older + // manifest versions. No per-table write happens here; + // the tombstone is the entire change. Hard mode + // (immediate dataset deletion via cleanup) lands in + // commit #5 gated by --allow-data-loss. + if !matches!(mode, DropMode::Soft) { + return Err(OmniError::manifest_internal( + "DropType { Hard } not yet implemented (commit #5)", + )); + } + let table_key = schema_table_key(*type_kind, name); + if table_key.starts_with("edge:") { + changed_edge_tables = true; + } + dropped_tables.insert(table_key); } step @ SchemaMigrationStep::UnsupportedChange { .. } => { return Err(OmniError::manifest( @@ -243,6 +262,26 @@ pub(super) async fn apply_schema_with_lock( tombstone_version: source_entry.table_version.saturating_add(1), }); } + // Soft DropType: mark each dropped table for tombstoning in the + // recovery sidecar AND in the live table_tombstones map. The + // mechanism mirrors rename's source-table tombstone — manifest + // entry removed at version+1, dataset files retained, time-travel + // reachable until cleanup. No Phase B write happens for these + // tables; the recovery sidecar is purely the manifest delta. + for dropped_table_key in &dropped_tables { + let entry = snapshot.entry(dropped_table_key).ok_or_else(|| { + OmniError::manifest(format!( + "missing table '{}' for soft drop when building recovery sidecar", + dropped_table_key + )) + })?; + let tombstone_version = entry.table_version.saturating_add(1); + sidecar_tombstones.push(crate::db::manifest::SidecarTombstone { + table_key: dropped_table_key.clone(), + tombstone_version, + }); + table_tombstones.insert(dropped_table_key.clone(), tombstone_version); + } // Acquire per-(table_key, branch) queues for every existing table // that schema_apply will rewrite or re-index. New tables (added or diff --git a/crates/omnigraph/tests/schema_apply.rs b/crates/omnigraph/tests/schema_apply.rs index 99484db..57a6073 100644 --- a/crates/omnigraph/tests/schema_apply.rs +++ b/crates/omnigraph/tests/schema_apply.rs @@ -221,7 +221,7 @@ async fn apply_schema_drops_a_nullable_property_softly_preserves_prior_version() } #[tokio::test] -async fn apply_schema_rejects_dropping_a_node_type() { +async fn apply_schema_drops_node_and_referencing_edge_softly() { let dir = tempfile::tempdir().unwrap(); let mut db = init_and_load(&dir).await; let before_version = db @@ -230,7 +230,11 @@ async fn apply_schema_rejects_dropping_a_node_type() { .unwrap() .version(); - // Drop the `Company` node type and its outgoing edge that references it. + // Drop the `Company` node type and the `WorksAt` edge that references it. + // Per schema-lint v1 chassis commit #4 (MR-694), this emits two + // `DropType { Soft }` steps; apply tombstones both manifest entries. + // Lance dataset files are retained, so time-travel back to the + // pre-drop manifest version still resolves both tables. let desired = r#" node Person { name: String @key @@ -241,23 +245,96 @@ edge Knows: Person -> Person { since: Date? } "#; - let err = db.apply_schema(desired).await.unwrap_err(); - let msg = err.to_string(); + + // Confirm the plan emits both DropType { Soft } steps. + let plan = db.plan_schema(desired).await.unwrap(); + assert!(plan.supported, "drop-type plan must be supported"); assert!( - msg.contains("OG-DS-102") || msg.contains("OG-DS-103"), - "expected schema-lint code OG-DS-102 or OG-DS-103 in error, got: {msg}" + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::DropType { + type_kind: SchemaTypeKind::Node, + name, + mode: omnigraph_compiler::DropMode::Soft, + } if name == "Company" + )), + "expected DropType {{ Node, Company, Soft }} in plan: {plan:?}", ); - assert_eq!( - db.snapshot_of(ReadTarget::branch("main")) - .await - .unwrap() - .version(), - before_version + assert!( + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::DropType { + type_kind: SchemaTypeKind::Edge, + name, + mode: omnigraph_compiler::DropMode::Soft, + } if name == "WorksAt" + )), + "expected DropType {{ Edge, WorksAt, Soft }} in plan: {plan:?}", + ); + + let result = db.apply_schema(desired).await.unwrap(); + assert!(result.supported); + assert!(result.applied); + + let after_version = db + .snapshot_of(ReadTarget::branch("main")) + .await + .unwrap() + .version(); + assert!( + after_version > before_version, + "manifest version should advance after soft type drop; before={before_version}, after={after_version}", + ); + + // (a) Current snapshot: both manifest entries are gone. + let current_snapshot = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); + assert!( + current_snapshot.entry("node:Company").is_none(), + "current manifest must not list node:Company after soft drop", + ); + assert!( + current_snapshot.entry("edge:WorksAt").is_none(), + "current manifest must not list edge:WorksAt after soft drop", + ); + // Person + Knows still present (Person wasn't dropped; Knows is in desired). + assert!( + current_snapshot.entry("node:Person").is_some(), + "node:Person must remain in the manifest", + ); + + // (b) Time travel: at the pre-drop manifest version, both dropped + // tables are still listed. Soft drop is reversible via Lance's + // version graph until `omnigraph cleanup` runs. + let pre_drop_snapshot = db.snapshot_at_version(before_version).await.unwrap(); + assert!( + pre_drop_snapshot.entry("node:Company").is_some(), + "pre-drop manifest must still list node:Company (time-travel reversibility)", + ); + assert!( + pre_drop_snapshot.entry("edge:WorksAt").is_some(), + "pre-drop manifest must still list edge:WorksAt (time-travel reversibility)", + ); + + // (c) Reopen consistency: drop is preserved across engine restart. + let uri = dir.path().to_str().unwrap().to_string(); + drop(db); + let reopened = Omnigraph::open(&uri).await.unwrap(); + let reopened_snapshot = reopened + .snapshot_of(ReadTarget::branch("main")) + .await + .unwrap(); + assert!( + reopened_snapshot.entry("node:Company").is_none(), + "after reopen, node:Company must still be absent from the current manifest", + ); + assert!( + reopened_snapshot.entry("edge:WorksAt").is_none(), + "after reopen, edge:WorksAt must still be absent from the current manifest", ); } #[tokio::test] -async fn apply_schema_rejects_dropping_an_edge_type() { +async fn apply_schema_drops_an_edge_type_softly() { let dir = tempfile::tempdir().unwrap(); let mut db = init_and_load(&dir).await; let before_version = db @@ -266,20 +343,50 @@ async fn apply_schema_rejects_dropping_an_edge_type() { .unwrap() .version(); - // Drop only the `WorksAt` edge. + // Drop only the `WorksAt` edge. Per chassis v1 commit #4, this + // emits `DropType { Edge, WorksAt, Soft }`; apply tombstones the + // edge:WorksAt manifest entry. The Company node and Person node + // remain intact. let desired = TEST_SCHEMA.replace("\nedge WorksAt: Person -> Company", ""); - let err = db.apply_schema(&desired).await.unwrap_err(); - let msg = err.to_string(); + + let plan = db.plan_schema(&desired).await.unwrap(); + assert!(plan.supported); assert!( - msg.contains("OG-DS-103"), - "expected schema-lint code OG-DS-103 in error, got: {msg}" + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::DropType { + type_kind: SchemaTypeKind::Edge, + name, + mode: omnigraph_compiler::DropMode::Soft, + } if name == "WorksAt" + )), + "expected DropType {{ Edge, WorksAt, Soft }} in plan: {plan:?}", ); - assert_eq!( - db.snapshot_of(ReadTarget::branch("main")) - .await - .unwrap() - .version(), - before_version + + let result = db.apply_schema(&desired).await.unwrap(); + assert!(result.applied); + + let after_version = db + .snapshot_of(ReadTarget::branch("main")) + .await + .unwrap() + .version(); + assert!(after_version > before_version); + + let current_snapshot = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); + assert!( + current_snapshot.entry("edge:WorksAt").is_none(), + "current manifest must not list edge:WorksAt", + ); + // Other tables untouched. + assert!(current_snapshot.entry("node:Person").is_some()); + assert!(current_snapshot.entry("node:Company").is_some()); + assert!(current_snapshot.entry("edge:Knows").is_some()); + + let pre_drop_snapshot = db.snapshot_at_version(before_version).await.unwrap(); + assert!( + pre_drop_snapshot.entry("edge:WorksAt").is_some(), + "pre-drop manifest must still list edge:WorksAt", ); }