diff --git a/crates/omnigraph-compiler/src/catalog/schema_plan.rs b/crates/omnigraph-compiler/src/catalog/schema_plan.rs index f9c708d..08fe16a 100644 --- a/crates/omnigraph-compiler/src/catalog/schema_plan.rs +++ b/crates/omnigraph-compiler/src/catalog/schema_plan.rs @@ -562,18 +562,22 @@ fn plan_properties( .iter() .filter(|property| !consumed.contains(&property.name)) { - steps.push(SchemaMigrationStep::UnsupportedChange { - entity: format!( - "{}:{}.{}", - schema_type_kind_key(type_kind), - type_name, - leftover.name - ), - reason: format!( - "removing property '{}.{}' is not supported in schema migration v1", - type_name, leftover.name - ), - code: Some(crate::lint::codes::OG_DS_104.code.to_string()), + // Property removed from the desired schema: emit + // DropProperty { Soft } per docs/schema-lint-v1-plan.md + // commit #3. The Soft mode reuses the existing + // stage_overwrite rewrite path — batch_for_schema_apply_rewrite + // iterates target_schema.fields(), so the dropped column is + // naturally projected away. The prior Lance version retains + // the column until cleanup_old_versions runs, matching the + // OG-DS-104 destructive-tier expectation that data remains + // recoverable via time travel until cleanup. Hard mode (with + // immediate compact_files + cleanup_old_versions) lands in + // commit #5, gated by --allow-data-loss. + steps.push(SchemaMigrationStep::DropProperty { + type_kind, + type_name: type_name.to_string(), + property_name: leftover.name.clone(), + mode: DropMode::Soft, }); } @@ -926,6 +930,67 @@ node Account @rename_from("User") { })); } + #[test] + fn plan_emits_soft_drop_for_removed_nullable_property() { + // Removing a property from the desired schema emits + // DropProperty { Soft } (schema-lint v1 chassis commit #3, + // MR-694). The plan is `supported = true` — the apply path + // handles soft drop via the existing stage_overwrite rewrite + // projection. Verified at the integration level by + // `apply_schema_drops_a_nullable_property_softly_preserves_prior_version` + // in `crates/omnigraph/tests/schema_apply.rs`. + let accepted = build_schema_ir( + &parse_schema( + r#" +node Person { + name: String @key + age: I32? +} +"#, + ) + .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-property plan must be supported: {plan:?}" + ); + assert!( + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::DropProperty { + type_kind: SchemaTypeKind::Node, + type_name, + property_name, + mode: DropMode::Soft, + .. + } if type_name == "Person" && property_name == "age" + )), + "expected DropProperty {{ Soft }} step in plan: {plan:?}", + ); + // Negative: no UnsupportedChange anywhere in the plan. + assert!( + !plan + .steps + .iter() + .any(|step| matches!(step, UnsupportedChange { .. })), + "soft 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.rs b/crates/omnigraph/src/db/omnigraph.rs index 50d4963..bf34ff9 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -18,8 +18,8 @@ use omnigraph_compiler::catalog::{Catalog, EdgeType, NodeType}; use omnigraph_compiler::schema::parser::parse_schema; use omnigraph_compiler::types::ScalarType; use omnigraph_compiler::{ - SchemaIR, SchemaMigrationPlan, SchemaMigrationStep, SchemaTypeKind, build_catalog_from_ir, - build_schema_ir, plan_schema_migration, + DropMode, SchemaIR, SchemaMigrationPlan, SchemaMigrationStep, SchemaTypeKind, + build_catalog_from_ir, build_schema_ir, plan_schema_migration, }; use crate::db::graph_coordinator::{GraphCoordinator, PublishedSnapshot}; diff --git a/crates/omnigraph/src/db/omnigraph/schema_apply.rs b/crates/omnigraph/src/db/omnigraph/schema_apply.rs index 8d2eb28..f764e22 100644 --- a/crates/omnigraph/src/db/omnigraph/schema_apply.rs +++ b/crates/omnigraph/src/db/omnigraph/schema_apply.rs @@ -138,15 +138,39 @@ pub(super) async fn apply_schema_with_lock( } SchemaMigrationStep::UpdateTypeMetadata { .. } | SchemaMigrationStep::UpdatePropertyMetadata { .. } => {} - SchemaMigrationStep::DropType { .. } | SchemaMigrationStep::DropProperty { .. } => { - // Dormant — variants exist on the IR but the planner - // doesn't emit them yet. Implementation lands in a - // subsequent commit (see docs/schema-lint-v1-plan.md). - // If a SchemaIR JSON containing one of these arrives - // (e.g. round-tripped from a future tool), fail - // explicitly rather than silently misclassifying. + SchemaMigrationStep::DropProperty { + type_kind, + type_name, + mode, + .. + } => { + // Soft = reuse the existing stage_overwrite rewrite + // path. batch_for_schema_apply_rewrite iterates the + // *target* schema fields, so a property absent from + // desired_catalog is naturally projected away. The + // prior Lance version retains the dropped column, + // so reads at the previous snapshot still see it + // (time-travel reversibility). Hard mode (immediate + // compact_files + cleanup_old_versions for actual + // data deletion) lands in commit #5 gated by + // --allow-data-loss. + if !matches!(mode, DropMode::Soft) { + return Err(OmniError::manifest_internal( + "DropProperty { Hard } not yet implemented (commit #5)", + )); + } + let table_key = schema_table_key(*type_kind, type_name); + if table_key.starts_with("edge:") { + changed_edge_tables = true; + } + 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( - "drop-step variant is not yet implemented in apply_schema", + "DropType not yet implemented (commit #4)", )); } step @ SchemaMigrationStep::UnsupportedChange { .. } => { diff --git a/crates/omnigraph/tests/schema_apply.rs b/crates/omnigraph/tests/schema_apply.rs index fac5ab4..99484db 100644 --- a/crates/omnigraph/tests/schema_apply.rs +++ b/crates/omnigraph/tests/schema_apply.rs @@ -101,17 +101,23 @@ async fn apply_schema_unsupported_plan_does_not_advance_manifest() { ); } -// ─── Destructive / safety-tier rejections ──────────────────────────────────── +// ─── Destructive / safety-tier behavior ────────────────────────────────────── // -// Schema migration v1 only accepts additive change: add type, add nullable -// property, add index, rename. Every other shape returns an -// `UnsupportedChange` step that surfaces as an error from `apply_schema`, -// without advancing the manifest. These tests pin that contract for the -// destructive shapes (drop type, drop property, narrow type, add required, -// remove constraint) so a regression in the planner can't silently allow them. +// Schema migration v1 accepts: +// - Additive change: add type, add nullable property, add index, rename. +// - DropProperty { Soft } via the schema-lint v1 chassis (commit #3 of MR-694) +// — the dropped column is removed from the current manifest version but +// remains reachable via Lance time travel at the prior version, until +// `omnigraph cleanup` runs. Hard mode (immediate data cleanup) lands in +// commit #5 gated by `--allow-data-loss`. +// +// Every other destructive shape (drop type, narrow type, add required without +// backfill, remove constraint) still returns an `UnsupportedChange` step that +// surfaces as an error from `apply_schema`. These tests pin the current +// contract so a regression in the planner can't silently change behavior. #[tokio::test] -async fn apply_schema_rejects_dropping_a_property_with_data() { +async fn apply_schema_drops_a_nullable_property_softly_preserves_prior_version() { let dir = tempfile::tempdir().unwrap(); let mut db = init_and_load(&dir).await; @@ -122,25 +128,96 @@ async fn apply_schema_rejects_dropping_a_property_with_data() { .unwrap() .version(); - // Drop `age` from Person. v1 doesn't support property removal even when - // the column is nullable — it would silently destroy data. + // Drop `age` from Person. v1 + chassis commit #3 emit + // `DropProperty { Soft }`; the rewrite path projects to the + // target schema (no `age`), commits via stage_overwrite. Row + // counts are unchanged — only the column is dropped from the + // current schema view. let desired = TEST_SCHEMA.replace(" age: I32?\n", ""); - let err = db.apply_schema(&desired).await.unwrap_err(); - let msg = err.to_string(); + + // Confirm the plan emits DropProperty { Soft } (not UnsupportedChange). + let plan = db.plan_schema(&desired).await.unwrap(); + assert!(plan.supported, "drop-property plan must be supported"); assert!( - msg.contains("OG-DS-104"), - "expected schema-lint code OG-DS-104 in error, got: {msg}" + plan.steps.iter().any(|step| matches!( + step, + SchemaMigrationStep::DropProperty { + type_kind: SchemaTypeKind::Node, + type_name, + property_name, + mode: omnigraph_compiler::DropMode::Soft, + .. + } if type_name == "Person" && property_name == "age" + )), + "expected DropProperty {{ type=Person, property=age, mode=Soft }} in plan; got {plan:?}", ); - // Manifest didn't advance and existing rows are untouched. - assert_eq!( - db.snapshot_of(ReadTarget::branch("main")) - .await - .unwrap() - .version(), - before_version + let result = db.apply_schema(&desired).await.unwrap(); + assert!(result.supported); + assert!(result.applied); + + // Manifest advanced; row count unchanged. + let after_version = db + .snapshot_of(ReadTarget::branch("main")) + .await + .unwrap() + .version(); + assert!( + after_version > before_version, + "manifest version should advance after soft drop; before={before_version}, after={after_version}", ); assert_eq!(count_rows(&db, "node:Person").await, people_before); + + // (a) Current snapshot: `age` is gone from the dataset schema. + let current_snapshot = db.snapshot_of(ReadTarget::branch("main")).await.unwrap(); + let current_ds = current_snapshot.open("node:Person").await.unwrap(); + let current_fields = current_ds + .schema() + .fields + .iter() + .map(|f| f.name.clone()) + .collect::>(); + assert!( + !current_fields.iter().any(|f| f == "age"), + "current Person dataset schema must not include 'age' after soft drop; got fields {current_fields:?}", + ); + + // (b) Time travel: at the pre-drop manifest version, the prior + // Person dataset version still has `age`. 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(); + let pre_drop_ds = pre_drop_snapshot.open("node:Person").await.unwrap(); + let pre_drop_fields = pre_drop_ds + .schema() + .fields + .iter() + .map(|f| f.name.clone()) + .collect::>(); + assert!( + pre_drop_fields.iter().any(|f| f == "age"), + "pre-drop Person dataset schema must still include 'age' (time-travel reversibility); got fields {pre_drop_fields:?}", + ); + + // (c) Reopen consistency: close the engine, reopen, verify the + // drop is preserved (column still absent from current schema). + 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(); + let reopened_ds = reopened_snapshot.open("node:Person").await.unwrap(); + let reopened_fields = reopened_ds + .schema() + .fields + .iter() + .map(|f| f.name.clone()) + .collect::>(); + assert!( + !reopened_fields.iter().any(|f| f == "age"), + "after reopen, Person dataset schema must still lack 'age'; got fields {reopened_fields:?}", + ); } #[tokio::test]