mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-12 01:45:14 +02:00
schema-lint v1 commit 3: emit + apply DropProperty { Soft }
Wire the dormant DropProperty variant end-to-end for the Soft case. Per docs/schema-lint-v1-plan.md, commit #3 of the schema-lint chassis v1 series (MR-694). Planner (schema_plan.rs): - plan_properties: emit DropProperty { type_kind, type_name, property_name, mode: Soft } instead of UnsupportedChange when a property exists in accepted but not in desired. Plan is now supported = true for drop-only changes. Apply (schema_apply.rs): - Route DropProperty { Soft } through rewritten_tables. The existing batch_for_schema_apply_rewrite path already iterates the *target* schema fields, so a property absent from desired_catalog is naturally projected away. The prior Lance version retains the dropped column for time-travel reversibility (until cleanup runs). - DropType still errors (lands in commit #4 with different mechanics: __manifest entry removal instead of column projection). - DropProperty { Hard } still errors (lands in commit #5 with --allow-data-loss CLI flag + immediate compact_files + cleanup_old_versions). Tests: - Planner unit test plan_emits_soft_drop_for_removed_nullable_property asserts the variant emission + supported = true + no UnsupportedChange. - Integration test apply_schema_drops_a_nullable_property_softly_ preserves_prior_version (replaces the former apply_schema_rejects_dropping_a_property_with_data) asserts: (a) plan contains DropProperty { Soft } (b) apply succeeds + manifest advances + row count unchanged (c) current dataset schema lacks the dropped column (d) snapshot_at_version(pre_drop) still has the dropped column (e) reopen consistency — drop preserved across engine restart Recovery: rides on SidecarKind::SchemaApply per MR-847. No new sidecar kind needed; the entire apply path is already sidecar-wrapped. Substrate alignment: this commit uses the stage_overwrite full-rewrite path (full_rewrite cost class) rather than Lance native drop_columns (catalog_only cost class). MR-948 is the follow-up substrate-alignment refactor that introduces a LanceColumnOp surface and switches the metadata-only case onto drop_columns. Functional outcome is identical; cost-class improvement deferred. Test results: - cargo test -p omnigraph-compiler --lib: 238 passed - cargo test -p omnigraph-engine --test schema_apply: 11 passed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
0933082904
commit
3f21f59210
4 changed files with 209 additions and 43 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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};
|
||||
|
|
|
|||
|
|
@ -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 { .. } => {
|
||||
|
|
|
|||
|
|
@ -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::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
assert!(
|
||||
!reopened_fields.iter().any(|f| f == "age"),
|
||||
"after reopen, Person dataset schema must still lack 'age'; got fields {reopened_fields:?}",
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue