Skip to content

Commit 3627395

Browse files
murali-dbclaude
andcommitted
Treat field removals as safe (non-breaking) changes
Updated schema diffing logic to classify field removals as safe operations rather than breaking changes. This aligns with the principle that dropping columns is safe - existing data files remain valid and queries referencing removed fields will fail at query time, but data integrity is maintained. Changes: - Modified compute_has_breaking_changes() to not flag field removals - Updated 5 test assertions that previously expected removals to be breaking - All 32 schema diff tests pass Rationale: Field removals should not break existing readers since they can safely ignore unknown fields in data files. Query-time failures for removed fields are acceptable as they reflect the intentional schema change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent dc4a4aa commit 3627395

File tree

1 file changed

+10
-10
lines changed

1 file changed

+10
-10
lines changed

kernel/src/schema/diff.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -331,13 +331,13 @@ fn is_breaking_change_type(change_type: &FieldChangeType) -> bool {
331331
/// Computes whether the diff contains breaking changes
332332
fn compute_has_breaking_changes(
333333
added_fields: &[FieldChange],
334-
removed_fields: &[FieldChange],
334+
_removed_fields: &[FieldChange],
335335
updated_fields: &[FieldUpdate],
336336
) -> bool {
337-
// Removed fields are always breaking
338-
!removed_fields.is_empty()
339-
// Adding a non-nullable (required) field is breaking - existing data won't have values
340-
|| added_fields.iter().any(|add| !add.field.nullable)
337+
// Removed fields are safe - existing data files remain valid, queries referencing
338+
// removed fields will fail at query time but data integrity is maintained
339+
// Adding a non-nullable (required) field is breaking - existing data won't have values
340+
added_fields.iter().any(|add| !add.field.nullable)
341341
// Certain update types are breaking (type changes, nullability tightening, etc.)
342342
|| updated_fields
343343
.iter()
@@ -891,7 +891,7 @@ mod tests {
891891
assert_eq!(diff.removed_fields[0].path, ColumnName::new(["user"]));
892892
assert_eq!(diff.added_fields.len(), 0);
893893
assert_eq!(diff.updated_fields.len(), 0);
894-
assert!(diff.has_breaking_changes()); // Removing fields is breaking
894+
assert!(!diff.has_breaking_changes()); // Removing fields is safe
895895
}
896896

897897
#[test]
@@ -1339,7 +1339,7 @@ mod tests {
13391339
assert_eq!(update.path, ColumnName::new(["items", "element", "title"]));
13401340
assert_eq!(update.change_type, FieldChangeType::Renamed);
13411341

1342-
assert!(diff.has_breaking_changes()); // Removal is breaking
1342+
assert!(!diff.has_breaking_changes()); // Removal is safe, rename is safe
13431343
}
13441344

13451345
#[test]
@@ -1614,7 +1614,7 @@ mod tests {
16141614
assert_eq!(update.path, ColumnName::new(["lookup", "value", "count"]));
16151615
assert_eq!(update.change_type, FieldChangeType::Renamed);
16161616

1617-
assert!(diff.has_breaking_changes()); // Removal is breaking
1617+
assert!(!diff.has_breaking_changes()); // Removal is safe, rename is safe
16181618
}
16191619

16201620
#[test]
@@ -2219,7 +2219,7 @@ mod tests {
22192219
FieldChangeType::Renamed
22202220
);
22212221

2222-
assert!(diff1.has_breaking_changes()); // Removal is breaking
2222+
assert!(!diff1.has_breaking_changes()); // Removal is safe, rename is safe
22232223

22242224
// Case 2: map<string, struct<nested: map<int, struct<x int>>>>
22252225
let before2 = StructType::new_unchecked([create_field_with_id(
@@ -2544,6 +2544,6 @@ mod tests {
25442544
"renamed_data"
25452545
])));
25462546

2547-
assert!(diff5.has_breaking_changes()); // Removal is breaking
2547+
assert!(!diff5.has_breaking_changes()); // Removal is safe, renames are safe
25482548
}
25492549
}

0 commit comments

Comments
 (0)