fix(dataset): reject reserved system column names at the commit chokepoint

[LuciferYang]

Summary

System columns (_rowid, _rowaddr, _rowoffset, _row_created_at_version, _row_last_updated_at_version) are virtual: they are injected at read time and never stored. If a stored top-level manifest field happens to share one of these names, the injected system column collides with it on the next scan, panicking in Projection::to_schema at schema.extend(...).unwrap().

The write/insert path already rejects these names (#7797), but the schema-evolution and raw-commit paths had no guard: add_columns, alter_columns (rename), drop_columns, Dataset::merge, and user-built Operations committed through Dataset::commit / CommitBuilder (including the Python and Java bindings) could all still land a reserved name in the stored schema.

This adds a single guard at the one commit chokepoint, Transaction::build_manifest, which every schema-bearing operation (Overwrite / Merge / Project) flows through. Other operations reuse the current manifest schema and cannot introduce a new name, so guarding this one spot covers every path. The check is top-level only, matching where the collision actually happens.

Grandfathering

Only newly introduced names are rejected. A reserved name already present in the current manifest schema is grandfathered, because schema evolution re-commits the full existing schema and older versions permitted some of these names (the row-version columns were reserved only recently). Rejecting a pre-existing name here would lock such a dataset out of all schema evolution.

Notes for reviewers

The guard fires at commit time, so merge / add_columns write their new column data files before the rejection, leaving orphan data files on a rejected commit. This is a strict improvement over the previous silent corruption, and the orphans are reclaimed by version cleanup.

The two guards are intentionally asymmetric: the write/insert guard (#7797) rejects all five names unconditionally, while this commit-time guard grandfathers names already in the manifest. The write path is strictly stricter; the commit-time path must let legacy datasets keep evolving.

Test plan

• build_manifest_rejects_newly_introduced_system_column_name / build_manifest_grandfathers_preexisting_system_column_name — unit tests over the guard's two branches.
• schema_evolution_rejects_reserved_system_column_names — 5 cases (one per name) over add_columns and alter_columns rename.
• merge_rejects_reserved_system_column_names — 5 cases over Dataset::merge.
• cargo fmt --all and cargo clippy --all --tests --benches -- -D warnings.

Summary by CodeRabbit

• Bug Fixes

  • Prevented schema changes from introducing reserved system column names.
  • Added consistent validation for column additions and renames, as well as merge, overwrite, and projection operations.
  • Existing datasets that already contain reserved system columns remain compatible.
  • Updated errors to clearly indicate the attempted column name is reserved.

• Tests

  • Added regression tests covering rejection of reserved system column names for schema evolution and dataset merges.

[coderabbitai]

📝 Walkthrough

Walkthrough

Reserved system column validation now runs during manifest installation, rejecting newly introduced reserved names while allowing existing ones. Parameterized tests cover schema evolution and merge operations across the reserved system column set.

Changes

Reserved column validation

Layer / File(s)	Summary
Manifest schema validation rust/lance/src/dataset/transaction.rs	Transaction::build_manifest rejects newly introduced system column names for overwrite, merge, and project operations, while allowing names already present in the current schema. Unit tests cover both rejection and grandfathering.
Schema evolution and merge regression coverage rust/lance/src/dataset/schema_evolution.rs, rust/lance/src/dataset/tests/dataset_merge_update.rs	Parameterized tests verify that column addition, renaming, and dataset merge operations reject each reserved system column name with a reserved-name error.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• lance-format/lance#7797: Both changes enforce reserved system-column name checks across write or schema-manipulation paths.

Suggested reviewers: xuanwo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: rejecting reserved system column names during commit-time validation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 6713-6718: Update the reserved-name tests to assert both the typed
error and message: in rust/lance/src/dataset/transaction.rs lines 6713-6718, add
an Error::InvalidInput match for err; in
rust/lance/src/dataset/schema_evolution.rs lines 2097-2112, add the same variant
assertion for both add_err and rename_err; and in
rust/lance/src/dataset/tests/dataset_merge_update.rs lines 4337-4345, assert
Error::InvalidInput before checking the message. Preserve the existing “reserved
name” message checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f50167e6-3b3d-4b09-9e39-e6b2c035a998

📥 Commits

Reviewing files that changed from the base of the PR and between 654f0d4 and 8b291c3.

📒 Files selected for processing (3)

• rust/lance/src/dataset/schema_evolution.rs
• rust/lance/src/dataset/tests/dataset_merge_update.rs
• rust/lance/src/dataset/transaction.rs

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

rust/lance/src/dataset/schema_evolution.rs (1)

2073-2085: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the standard test fixtures and URI.

Replace manual ArrowSchema/RecordBatch::try_new boilerplate with record_batch!(), and use a plain memory:// URI instead of TempStrDir in these tests.

• rust/lance/src/dataset/schema_evolution.rs#L2073-L2085: update the dataset fixture.
• rust/lance/src/dataset/tests/dataset_merge_update.rs#L4309-L4337: update both left and right batch fixtures.

As per coding guidelines, tests should use record_batch!() and plain "memory://" URIs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/schema_evolution.rs` around lines 2073 - 2085, Replace
the manual ArrowSchema, RecordBatch, and RecordBatchIterator setup in the
dataset fixture at rust/lance/src/dataset/schema_evolution.rs:2073-2085 with the
standard record_batch!() fixture, and use a plain "memory://" URI instead of
TempStrDir. Apply the same fixture and URI changes to both left and right batch
setups at rust/lance/src/dataset/tests/dataset_merge_update.rs:4309-4337,
preserving each test’s existing batch data and Dataset::write flow.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@rust/lance/src/dataset/schema_evolution.rs`:
- Around line 2073-2085: Replace the manual ArrowSchema, RecordBatch, and
RecordBatchIterator setup in the dataset fixture at
rust/lance/src/dataset/schema_evolution.rs:2073-2085 with the standard
record_batch!() fixture, and use a plain "memory://" URI instead of TempStrDir.
Apply the same fixture and URI changes to both left and right batch setups at
rust/lance/src/dataset/tests/dataset_merge_update.rs:4309-4337, preserving each
test’s existing batch data and Dataset::write flow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 93153ace-ecb8-4cac-8dfc-16709600e517

📥 Commits

Reviewing files that changed from the base of the PR and between 8b291c3 and 9f962be.

📒 Files selected for processing (3)

• rust/lance/src/dataset/schema_evolution.rs
• rust/lance/src/dataset/tests/dataset_merge_update.rs
• rust/lance/src/dataset/transaction.rs