feat: add set_nullable support for ALTER TABLE

[william-ch-databricks]

🥞 Stacked PR

Use this link to review incremental changes.

• stack/alter-table-1-refactor-state [Files changed] [MERGED]
  • stack/alter-table-2-supports-data-files [Files changed] [MERGED]
    • stack/alter-table-3-framework-add-column [Files changed] [MERGED]
      • stack/alter-table-4-set-nullable [Files changed]
        • stack/alter-table-5-column-mapping-add [Files changed]
          • stack/alter-table-6-drop-column [Files changed]
            • stack/alter-table-7-rename-column [Files changed]
              • stack/alter-table-8-operation-string [Files changed]
                • stack/alter-table-9-is-blind-append [Files changed]

---

What changes are proposed in this pull request?

Adds the SetNullable operation to the ALTER TABLE framework, allowing columns to be changed
from NOT NULL to nullable. Supports nested columns via ColumnName paths.

modify_field_at_path function to recurse through struct and rebuild bottom-up to update nullable field.

How was this change tested?

• Unit tests for modify_field_at_path: top-level, nested, sibling preservation, nonexistent
  field, non-struct traversal, case-insensitive lookup
• Unit tests for SetNullable via apply_schema_operations: required->nullable, already
  nullable noop, deeply nested (3 levels), chained with add_column
• Integration tests in kernel/tests/alter_table.rs: set_nullable with schema reload,
  already-nullable noop, nonexistent column error, chained add_column + set_nullable

[scottsand-db]

Looks great! Minor comments

[scottsand-db]

• can we set a partition column to be nullable?
• let's x-test this against ColumnMaping please - CM OFF, NAME, ID
• let's write the CRC (checksum) after this commit, too
• let's checkpoint after this commit, too
• let's read back the table after that, too

[william-ch-databricks]

all added except for CRC.

I don't think we currently support CRC right now. I've added this to the issue tracking already. Can we address in follow-up?

const INCREMENTAL_SAFE_OPS: &[&str] = &[                                                                                                                                                              
      "WRITE", "MERGE", "UPDATE", "DELETE", "OPTIMIZE",                                                                                                                                                 
      "CREATE TABLE", "REPLACE TABLE",                                                                                                                                                                  
      "CREATE TABLE AS SELECT", "REPLACE TABLE AS SELECT", "CREATE OR REPLACE TABLE AS SELECT",
  ]; 

[scottsand-db]

We actually do have CRC right now! See snapshot.write_checksum()

[scottsand-db]

Food for thought: Claude tells me

  Should Fix

  S1. modify_indexmap_in_place is over-engineered; use IndexMap::get_mut

  File: kernel/src/transaction/schema_evolution.rs:93-111
  Flagged by: scovich-reviewer, code-review-refactor

  The swap_remove_full + insert + swap_indices dance is clever but surprising. IndexMap::get_mut(&key) returns Option<&mut V> in place with no removal/reinsertion, and the only caller's modifier (|mut field| { field.nullable = true; ... Ok(field) }) only needs mutable access, not ownership transfer. The current helper
  has only one caller and four tests protecting its own correctness.

  fn modify_indexmap_in_place<K, V, E>(
      map: &mut IndexMap<K, V>,
      key: &K,
      on_missing: impl FnOnce() -> E,
      f: impl FnOnce(&mut V) -> Result<(), E>,
  ) -> Result<(), E> { ... map.get_mut(key).ok_or_else(on_missing).and_then(f) }

  As a bonus, the docs-reviewer identified that the current helper leaves the map in an inconsistent state if f returns an error (key already removed, last element moved into its slot). The get_mut version eliminates that hazard.

[william-ch-databricks]

Hmm, this was because the inner fields weren't directly mutable. But we could expose this and make it a lot cleaner! PTAL at new code

[scottsand-db]

Are there other test-only functions in this file? Could we move this to be next to them? Are we able to move this inside of a test module scope? I think that would provide a compile-time guarantee that this is only used by test code and never by production code. I want to avoid anyone using this function in production code since it panics.

[william-ch-databricks]

I put it as a method on StructType, because it's used in kernel/tests/alter_tables.rs and kernel/src/transaction/schema_evolution.rs. It uses #[cfg(any(test, feature = "test-utils"))] to try to enforce that this won't be used in production. We could move this into test module scope, but I put it here to avoid duplicating it. Let me know what you think

[scottsand-db]

Can you ask Claude to read the Delta Spark "schema evolution set nullable" test suites and see if there are any other interesting test cases we should handle?

[william-ch-databricks]

added clustering column test

[scottsand-db]

Claude says (food for thought):

  The previous code rebuilt via StructType::try_new(fields.into_values()), which runs ensure_no_metadata_columns_in_field recursively and rebuilds metadata_columns. The new in-place mutation skips both. For SetNullable it's safe; for AddColumn the top-level metadata-column check remains, but nested metadata columns
  inside a struct field now slip through (theoretical today since they require new_unchecked).

  Fix: either re-run an equivalent nested-metadata-column scan after each op, or add a comment documenting the assumption that input fields came from try_new. The PR description doesn't mention this trade-off.

[william-ch-databricks]

Hmm, true, we should check for adding a column. But this doesn't impact set nullable/rename/drop as it doesn't change anything except for name/nullability

[scottsand-db]

Food for thought, claude says:

  walk_column_fields_by (kernel/src/schema/mod.rs:760-792) already implements the immutable case-sensitive descent. The PR re-implements the mutable case-insensitive descent as a free function that pokes at StructType internals via field_map_mut. Move it onto StructType::modify_field_at_path(path, FnOnce(&mut
  StructField) -> DeltaResult<()>) — this drops LeafAction, drops field_map_mut callers, tightens encapsulation, and brings the helper down from ~115 LOC to ~35 LOC. Could be deferred to the next stacked PR if the author files an issue, but the consolidation only gets harder once more callers exist.

[william-ch-databricks]

I think this could be a good idea, but I would prefer to consolidate later after the final implementation of modify_field_at_path is decided