-
Notifications
You must be signed in to change notification settings - Fork 248
schema-lint chassis v1.0: DropProperty Soft + code-tagged diagnostics (MR-694) #90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
babacb4
7ecbda6
0933082
3f21f59
2b468b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,29 @@ pub enum SchemaTypeKind { | |||||||
| Edge, | ||||||||
| } | ||||||||
|
|
||||||||
| /// How a drop step interacts with data. | ||||||||
| /// | ||||||||
| /// - **`Soft`** — catalog tombstone only. The type / property is hidden | ||||||||
| /// from queries but the underlying Lance column / dataset is retained | ||||||||
| /// on disk. Reversible via `omnigraph schema unhide` (forthcoming). | ||||||||
| /// Tier: `safe`. | ||||||||
| /// - **`Hard`** — actual data removal. The Lance column is rewritten | ||||||||
| /// without the property, or the Lance dataset is dropped. Irreversible | ||||||||
| /// short of branch / snapshot restore. Tier: `destructive`; requires | ||||||||
| /// `--allow-data-loss` to apply. | ||||||||
| /// | ||||||||
| /// The planner emits `Soft` by default; `--allow-data-loss` on the apply | ||||||||
| /// CLI promotes drops to `Hard`. This is the dimension orthogonal to | ||||||||
| /// `SafetyTier` from the schema-lint chassis (`crate::lint`): tier | ||||||||
| /// describes the rule's class; mode describes the operator's intent for | ||||||||
| /// data treatment. | ||||||||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] | ||||||||
| #[serde(rename_all = "snake_case")] | ||||||||
| pub enum DropMode { | ||||||||
| Soft, | ||||||||
| Hard, | ||||||||
| } | ||||||||
|
|
||||||||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||||||||
| pub struct SchemaMigrationPlan { | ||||||||
| pub supported: bool, | ||||||||
|
|
@@ -62,6 +85,28 @@ pub enum SchemaMigrationStep { | |||||||
| property_name: String, | ||||||||
| annotations: Vec<Annotation>, | ||||||||
| }, | ||||||||
| /// Remove a node or edge type. Soft mode tombstones in the catalog | ||||||||
| /// and retains data on disk; Hard mode drops the Lance dataset and | ||||||||
| /// requires `--allow-data-loss`. | ||||||||
| /// | ||||||||
| /// Dormant in this commit — emitted by the planner in a later | ||||||||
| /// commit (see `docs/schema-lint-v1-plan.md`). | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: Custom agent: Flag AI Slop and Fabricated Changes Comments reference nonexistent docs path Prompt for AI agents
Suggested change
|
||||||||
| DropType { | ||||||||
| type_kind: SchemaTypeKind, | ||||||||
| name: String, | ||||||||
| mode: DropMode, | ||||||||
| }, | ||||||||
| /// Remove a property from an existing type. Soft mode tombstones | ||||||||
| /// the property in the catalog and retains the Lance column; Hard | ||||||||
| /// mode rewrites the column out and requires `--allow-data-loss`. | ||||||||
| /// | ||||||||
| /// Dormant in this commit. | ||||||||
| DropProperty { | ||||||||
| type_kind: SchemaTypeKind, | ||||||||
| type_name: String, | ||||||||
| property_name: String, | ||||||||
| mode: DropMode, | ||||||||
| }, | ||||||||
| UnsupportedChange { | ||||||||
| entity: String, | ||||||||
| reason: String, | ||||||||
|
|
@@ -93,6 +138,24 @@ impl SchemaMigrationStep { | |||||||
| _ => None, | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /// If this step carries a schema-lint code, return the full | ||||||||
| /// catalog entry — including family, safety tier, and default | ||||||||
| /// severity. Used by renderers that want to display richer | ||||||||
| /// context than just the code string (e.g. `omnigraph schema | ||||||||
| /// plan` annotating each line with its tier). | ||||||||
| /// | ||||||||
| /// Returns `None` for steps that carry no code (the 12 of 17 | ||||||||
| /// `UnsupportedChange` paths still untagged in v0, plus every | ||||||||
| /// non-`UnsupportedChange` variant). | ||||||||
| pub fn diagnostic(&self) -> Option<&'static crate::lint::DiagnosticCode> { | ||||||||
| match self { | ||||||||
| Self::UnsupportedChange { | ||||||||
| code: Some(c), .. | ||||||||
| } => crate::lint::lookup(c), | ||||||||
| _ => None, | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Prompt for AI agents
Suggested change
|
||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn plan_schema_migration( | ||||||||
|
|
@@ -499,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, | ||||||||
| }); | ||||||||
|
Comment on lines
+576
to
581
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 DropProperty on interfaces crashes apply with 'missing source table' because interfaces have no Lance dataset The Under the old code, interface property removal emitted Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+576
to
581
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 User-facing schema-lint docs not updated after property-drop behavior change (AGENTS.md Rule 1 violation)
Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -863,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( | ||||||||
|
|
@@ -935,4 +1063,56 @@ node Person @description("new") { | |||||||
| }], | ||||||||
| })); | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn drop_steps_round_trip_through_serde() { | ||||||||
| // The DropType / DropProperty variants are dormant in this | ||||||||
| // commit — the planner doesn't emit them yet — but their | ||||||||
| // serde shape needs to be stable from day one. A future | ||||||||
| // SchemaIR JSON containing one of these must deserialize | ||||||||
| // back to the same value. This test pins the wire format | ||||||||
| // so a v0 schema-ir consumer never sees a surprise variant | ||||||||
| // shape after v1 ships. | ||||||||
| let steps = vec![ | ||||||||
| SchemaMigrationStep::DropType { | ||||||||
| type_kind: SchemaTypeKind::Node, | ||||||||
| name: "Person".to_string(), | ||||||||
| mode: DropMode::Soft, | ||||||||
| }, | ||||||||
| SchemaMigrationStep::DropType { | ||||||||
| type_kind: SchemaTypeKind::Edge, | ||||||||
| name: "Knows".to_string(), | ||||||||
| mode: DropMode::Hard, | ||||||||
| }, | ||||||||
| SchemaMigrationStep::DropProperty { | ||||||||
| type_kind: SchemaTypeKind::Node, | ||||||||
| type_name: "Person".to_string(), | ||||||||
| property_name: "age".to_string(), | ||||||||
| mode: DropMode::Soft, | ||||||||
| }, | ||||||||
| SchemaMigrationStep::DropProperty { | ||||||||
| type_kind: SchemaTypeKind::Interface, | ||||||||
| type_name: "Named".to_string(), | ||||||||
| property_name: "alias".to_string(), | ||||||||
| mode: DropMode::Hard, | ||||||||
| }, | ||||||||
| ]; | ||||||||
|
|
||||||||
| for step in steps { | ||||||||
| let json = serde_json::to_string(&step).expect("serialize"); | ||||||||
| let round_trip: SchemaMigrationStep = | ||||||||
| serde_json::from_str(&json).expect("deserialize"); | ||||||||
| assert_eq!(step, round_trip, "round-trip mismatch on {json}"); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn drop_mode_serde_uses_snake_case() { | ||||||||
| // External tools may write SchemaIR JSON by hand. Pin the | ||||||||
| // wire form so we don't silently break them later. | ||||||||
| assert_eq!(serde_json::to_string(&DropMode::Soft).unwrap(), "\"soft\""); | ||||||||
| assert_eq!(serde_json::to_string(&DropMode::Hard).unwrap(), "\"hard\""); | ||||||||
| let soft: DropMode = serde_json::from_str("\"soft\"").unwrap(); | ||||||||
| assert_eq!(soft, DropMode::Soft); | ||||||||
| } | ||||||||
| } | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: According to linked Linear issue MR-694, schema plan output should surface code-tagged/classified diagnostics per step, but the new
DropPropertyoutput only shows(soft mode)and omits code/tier.Prompt for AI agents