mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-09 01:35:18 +02:00
schema-lint v1 commit 4: emit + apply DropType { Soft } (#99)
Wire the second half of the dormant Drop* family. Per docs/dev/schema-lint-v1-plan.md, commit #4 of the schema-lint chassis v1 series (MR-694). Builds on commit #3 (PR #90, DropProperty Soft). Planner (schema_plan.rs): - plan_nodes leftover loop: emit DropType { Node, name, Soft } instead of UnsupportedChange (OG-DS-102) for node-type removals. - plan_edges leftover loop: emit DropType { Edge, name, Soft } instead of UnsupportedChange (OG-DS-103) for edge-type removals. Apply (schema_apply.rs): - New dropped_tables: BTreeSet<String> accumulator alongside added_tables / renamed_tables / rewritten_tables. - DropType arm in the metadata loop populates dropped_tables for Soft mode. Hard mode errors (lands in commit #5 with --allow-data-loss). - New tombstone-emission loop after the rename sidecar build: for each dropped table, push to sidecar_tombstones AND populate table_tombstones with table_version + 1. The existing manifest publish path converts table_tombstones into ManifestChange::Tombstone operations — no new manifest plumbing needed. - Soft DropType has no Phase B per-table write; the tombstone is the entire change. Lance dataset files are retained — prior __manifest versions still reference them, so time travel + branch-from-snapshot can read the dropped table until cleanup_old_versions runs. - Rides on SidecarKind::SchemaApply per MR-847 (already established by commit #3). Tests: - Planner unit test plan_emits_soft_drop_for_removed_node_and_edge_types asserts both Node and Edge DropType { Soft } emission for the Company + WorksAt combined drop, plus no UnsupportedChange. - Integration test apply_schema_drops_node_and_referencing_edge_softly (replaces apply_schema_rejects_dropping_a_node_type): asserts plan emission, apply success, current manifest entries absent, pre-drop manifest entries present (time-travel reversibility), reopen consistency. - Integration test apply_schema_drops_an_edge_type_softly (replaces apply_schema_rejects_dropping_an_edge_type): single edge drop, asserts other tables untouched, time-travel reversibility. Test results: - cargo test -p omnigraph-compiler --lib: 239 passed (1 new + 238) - cargo test -p omnigraph-engine --test schema_apply: 11 passed (2 converted + 9 unchanged) Pending for v1 completion: - Commit #5: --allow-data-loss CLI flag + Hard mode promotion in planner + immediate compact_files + cleanup_old_versions for both DropProperty and DropType. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Andrew Altshuler
GitHub
parent
e98347eb7b
commit
58cee158d8
3 changed files with 273 additions and 45 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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::<String, HashMap<String, String>>::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
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue