Skip to content

feat: support schema evolution DDLs (ADD/DROP/RENAME/ALTER COLUMN) - #752

Open
puchengy wants to merge 8 commits into
lance-format:mainfrom
puchengy:schema-evolution-ddls
Open

feat: support schema evolution DDLs (ADD/DROP/RENAME/ALTER COLUMN)#752
puchengy wants to merge 8 commits into
lance-format:mainfrom
puchengy:schema-evolution-ddls

Conversation

@puchengy

@puchengy puchengy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Closes #62.

Adds support for the schema-evolution DDLs requested in #62 by extending BaseLanceNamespaceSparkCatalog.alterTable to handle Spark TableChange.ColumnChange requests, delegating to the underlying Lance dataset schema-evolution API via a new LanceSchemaEvolution helper.

Supported operations

DDL Lance API
ALTER TABLE ... ADD COLUMN[S] Dataset.addColumns
ALTER TABLE ... DROP COLUMN [IF EXISTS] Dataset.dropColumns
ALTER TABLE ... RENAME COLUMN Dataset.alterColumns (rename)
ALTER TABLE ... ALTER COLUMN ... DROP/SET NOT NULL Dataset.alterColumns (nullability)

Atomic, all-or-nothing semantics

To honor Spark's alterTable contract, an accepted request is validated in full against the current schema and then committed through exactly one Lance core operation (addColumns / dropColumns / alterColumns — each commits its whole batch atomically):

  • Same-kind changes are batched into that single call (e.g. ADD COLUMNS (a, b); rename + nullability edits to the same column are merged into one ColumnAlteration).
  • Requests that would require more than one core mutation are rejected before the first write: mixing column additions, drops, and alterations in one statement, or combining any column change with TBLPROPERTIES changes (which commit separately). This keeps a rejected ALTER TABLE from leaving the table partially mutated. Heterogeneous batching in one commit is not yet supported by the core.

Deliberately unsupported (rejected before any mutation)

  • ALTER COLUMN ... TYPE — throws UnsupportedOperationException. The lance-core:10.0.0-rc.3 JNI create_column_alteration reads the cast target from the Java ArrowType.toString() (e.g. "Int(64, true)") and parses it with Rust DataType::from_str, swallowing the parse failure with .ok() — so the cast target is dropped and the type change becomes a silent no-op (the commit lands but the stored type is unchanged; verified for both integer- and float-widening). This is fixed upstream in fix(java): carry alterColumns cast type across FFI via C Data Interface lance#8417 (carries the cast type across FFI via the Arrow C Data Interface), but that fix is not yet in a published org.lance:lance-core artifact — the newest published version (11.0.0-beta.3) predates it, and Lance does not publish nightly/snapshot Java builds. Once a lance release containing #8417 ships, this can be enabled by bumping <lance.version> and replacing the rejection with the castTo path.
  • ADD COLUMN with a DEFAULT value — Lance's all-null add cannot backfill a default, so it is rejected rather than silently filling NULL.
  • ADD COLUMN on a legacy-format (file_format_version='LEGACY') table — the core all-null add path rejects legacy datasets; rejected up front with a clear message instead of a raw core error.
  • Positional (FIRST/AFTER) adds, nested-column adds, and column-comment updates.

Tests

  • Java unit tests in SparkLanceNamespaceTestBase (run against both directory and REST namespaces): ADD/DROP/RENAME COLUMN and ALTER COLUMN DROP NOT NULL happy paths; batched multi-column ADD; and rejection cases for mixed-kind requests, column-change-plus-TBLPROPERTIES, DROP COLUMN IF EXISTS on a missing column, DEFAULT values, legacy format, and unsupported type changes.
  • PySpark integration tests in TestDDLAlterTableColumns.
  • TestSparkDirectoryNamespace full suite passes locally (66/66); make lint clean.

Docs updated in docs/src/operations/ddl/alter-table.md.

🤖 Generated with Claude Code

Closes lance-format#62.

Extend `BaseLanceNamespaceSparkCatalog.alterTable` to handle Spark
`TableChange.ColumnChange` requests, translating them into the
corresponding Lance dataset operations via a new `LanceSchemaEvolution`
helper:

- `ALTER TABLE ADD COLUMN`   -> `Dataset.addColumns`
- `ALTER TABLE DROP COLUMN`  -> `Dataset.dropColumns`
- `ALTER TABLE RENAME COLUMN`-> `Dataset.alterColumns` (rename)
- `ALTER TABLE ALTER COLUMN ... DROP/SET NOT NULL` -> `Dataset.alterColumns` (nullability)

`ALTER COLUMN ... TYPE` is rejected with a clear
`UnsupportedOperationException`: the current lance-core JNI drops the
cast target type on the way to Rust (it parses `ArrowType.toString()`
with `DataType::from_str` and swallows the failure), which would
otherwise turn a type change into a silent no-op. Column-comment updates
and positional (`FIRST`/`AFTER`) adds are likewise unsupported.

Adds Java unit tests (directory + REST namespaces) and PySpark
integration tests covering the happy path and the unsupported-type-change
error, and documents the new operations in the ALTER TABLE docs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The schema-evolution direction is useful, but this revision does not yet preserve Spark's alterTable contract or the connector's supported table formats. The four independent findings share that boundary: requested changes and options must either take effect exactly or fail before any mutation.

A viable revision should validate the complete ordered request against the current schema, commit it through one atomic core operation (or reject multi-change requests until that exists), and explicitly handle unsupported defaults, options, and legacy-format additions before writing.

private LanceSchemaEvolution() {}

static void apply(Dataset dataset, List<ColumnChange> changes) {
for (ColumnChange change : changes) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply commits each change during iteration, so a later rejection leaves earlier mutations durable. Spark's TableCatalog.alterTable contract says that when any change is rejected, none should be applied.

Reproducer run on this head
assertThrows(
    UnsupportedOperationException.class,
    () ->
        catalog.alterTable(
            ident,
            TableChange.addColumn(new String[] {"added"}, DataTypes.IntegerType),
            TableChange.updateColumnType(new String[] {"id"}, DataTypes.LongType)));
assertFalse(
    Arrays.asList(catalog.loadTable(ident).schema().fieldNames()).contains("added"));

The exception assertion passed, but the final assertion failed because added was still present after reload.

Validate the full ordered request first, then commit it atomically. If the core cannot batch these changes, reject a multi-change request before the first write.

if (add.comment() != null) {
metadataBuilder.putString("comment", add.comment());
}
StructField field =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddColumn.defaultValue() is never read, so this reports success while existing rows receive NULL instead of the requested backfill.

Reproducer run on this head
ColumnDefaultValue defaultValue =
    new ColumnDefaultValue(
        "7", new LiteralValue<>(7, DataTypes.IntegerType));
catalog.alterTable(
    ident,
    TableChange.addColumn(
        new String[] {"with_default"},
        DataTypes.IntegerType,
        true,
        null,
        null,
        defaultValue));
Row row = spark.table(fullName).select("with_default").head();
assertFalse(row.isNullAt(0));
assertEquals(7, row.getInt(0));

The first assertion failed: the observed value was NULL.

Honor the default through an appropriate Lance operation, or reject non-null defaults before any mutation instead of silently discarding them.

addColumn(dataset, (AddColumn) change);
} else if (change instanceof DeleteColumn) {
DeleteColumn delete = (DeleteColumn) change;
dataset.dropColumns(Collections.singletonList(path(delete.fieldNames())));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ignores DeleteColumn.ifExists(), so DROP COLUMN IF EXISTS still fails for a missing column.

Reproducer run on this head
assertDoesNotThrow(
    () ->
        catalog.alterTable(
            ident,
            TableChange.deleteColumn(new String[] {"missing"}, true)));

The assertion failed because dropColumns threw for missing.

Resolve the current schema before mutation and skip a missing field when ifExists is true; retain the error when it is false.

new StructField(fieldNames[0], add.dataType(), add.isNullable(), metadataBuilder.build());
Schema arrowSchema =
LanceArrowUtils.toArrowSchema(new StructType(new StructField[] {field}), "UTC", true);
dataset.addColumns(arrowSchema.getFields());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all-null addColumns path rejects datasets created with the connector's supported file_format_version='LEGACY', so the newly documented DDL does not work across supported table formats.

Reproducer run on this head
spark.sql(
    "CREATE TABLE " + fullName +
    " (id INT) TBLPROPERTIES ('file_format_version'='LEGACY')");
assertDoesNotThrow(
    () -> spark.sql("ALTER TABLE " + fullName + " ADD COLUMN added INT"));

The assertion failed because Dataset.addColumns rejected the legacy dataset.

Use a legacy-capable schema-evolution path, or explicitly scope and document the operation to stable-format tables and reject legacy tables before calling the unsupported core API.

westonpace pushed a commit to lance-format/lance that referenced this pull request Aug 7, 2026
…ce (#8417)

## Problem

`Dataset.alterColumns(...)` with a `castTo(...)` alteration silently
drops the cast: the commit lands (version bumps) but the stored column
type is unchanged.

Root cause is in the JNI `create_column_alteration`
(`java/lance-jni/src/blocking_dataset.rs`). The cast target type was
marshalled by calling the Java `ArrowType.toString()` (e.g. `"Int(64,
true)"`, `"FloatingPoint(DOUBLE)"`) and parsing the string with
`arrow_schema::DataType::from_str`, then discarding any parse error with
`.ok()`:

```rust
let data_type_str: String = env.get_string(&jstring)?.into();
DataType::from_str(&data_type_str)
    .map_err(|e| Error::input_error(e.to_string()))
    .ok()   // parse failure -> None -> cast silently dropped
```

`DataType`'s `FromStr` grammar does not accept the `Debug`-style strings
that `ArrowType.toString()` produces for parameterized types, so
`data_type` became `None` for anything beyond the few types whose
`toString()` happens to match (e.g. `Utf8`). The existing
`DatasetTest.testAlterColumns` only asserted field *names* after a cast,
so the dropped type went unnoticed.

## Fix

Transfer the cast target type through the Arrow **C Data Interface**,
mirroring the existing `addColumns(Schema)` path:

- **Java** (`Dataset.alterColumns`): export one field per requested cast
— in the same order as the alterations — into an `ArrowSchema`, and pass
its memory address to the native method.
- **JNI** (`inner_alter_columns`): import the schema via
`FFI_ArrowSchema` and attach each imported `DataType` to the
corresponding `ColumnAlteration`.

Rename-only and nullability-only alterations are unaffected. Removes the
now-unused `DataType` / `FromStr` imports.

## Test

Adds `DatasetTest.testAlterColumnsCastType`: widens `id` from `Int32` to
`Int64`, then does a combined rename+cast, asserting the resulting Arrow
type (not just the field name).

## Context

Surfaced while implementing schema-evolution DDLs in `lance-spark`
(lance-format/lance-spark#752), where `ALTER COLUMN ... TYPE` had to be
rejected because of this bug. With this fix released, lance-spark can
enable type changes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback on the schema-evolution DDL support:

- Validate the entire ordered ALTER TABLE request against the current
  schema before mutating anything, so a rejected change never leaves the
  table partially mutated (Spark's alterTable "all-or-nothing" contract).
- Honor DROP COLUMN IF EXISTS: a missing column is skipped instead of
  raising, while a plain DROP COLUMN on a missing column still fails.
- Reject ADD COLUMN with a DEFAULT value rather than silently filling
  NULL (Lance's all-null add cannot backfill a default).
- Reject ADD COLUMN on legacy-format (file_format_version='LEGACY')
  tables up front instead of surfacing a raw core error.

Adds unit tests for atomic rejection, DROP COLUMN IF EXISTS, default
rejection, and legacy-format rejection; a DROP COLUMN IF EXISTS
integration test; and documents the new limitations.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The new prevalidation resolves the earlier default, IF EXISTS, and legacy-format findings, but all-or-none behavior is still not preserved because validated changes are committed as separate Lance mutations.

A viable revision must compile each request into one proven-atomic core mutation. Until heterogeneous batching exists, reject multi-mutation requests—including schema-plus-property requests—before the first write.

// the schema state at the point each change is applied.
Set<String> current = topLevelFieldNames(dataset);
for (ColumnChange change : changes) {
applyOne(dataset, change, current);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop still commits each accepted change through a separate native mutation, so prevalidation cannot prevent partial durable state when a later change fails for a data-dependent reason.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName + " (id BIGINT, name STRING)");
spark.sql("INSERT INTO " + fullName + " VALUES (NULL, 'Alice')");

Identifier ident = Identifier.of(new String[] {"default"}, tableName);
assertThrows(
    Exception.class,
    () ->
        catalog.alterTable(
            ident,
            TableChange.addColumn(new String[] {"added"}, DataTypes.IntegerType),
            TableChange.updateColumnNullability(new String[] {"id"}, false)));
assertFalse(
    Arrays.asList(catalog.loadTable(ident).schema().fieldNames()).contains("added"));

The nullability change was rejected because id contains NULL, but the final assertion failed (added remained present).

Apply the ordered request through one atomic core transaction. If the core cannot do that yet, reject any invocation requiring multiple native mutations—and mixed column/property requests in the catalog layer—before the first write.

Address remaining review feedback: preserve Spark's all-or-nothing
alterTable contract by committing an accepted request through exactly one
Lance core operation instead of a sequence of per-change commits.

- Same-kind column changes are batched into a single core call:
  ADD -> addColumns, DROP -> dropColumns, RENAME/nullability ->
  alterColumns (rename + nullability edits to the same column are merged
  into one ColumnAlteration).
- Requests that would require more than one core mutation are rejected
  before the first write: mixing column additions, drops, and alterations
  in one statement, and combining any column change with TBLPROPERTIES
  changes (which commit separately).

Adds tests for batched multi-column ADD, mixed-kind rejection, and
column-change-plus-TBLPROPERTIES rejection; documents the single-mutation
restriction.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The single-core-mutation direction fixes partial commits, but the current compiler still accepts ordered and nested requests that it does not translate faithfully. These independent failures share one contract: every accepted TableChange sequence must have the same ordered result Spark specifies.

A viable revision should canonicalize dependent renames onto the original core target, preserve or prevalidate intermediate changes that a core builder would collapse, and reject nested paths before mutation unless full-path semantics are implemented.

if (change instanceof RenameColumn) {
RenameColumn rename = (RenameColumn) change;
builders
.computeIfAbsent(path(rename.fieldNames()), ColumnAlteration.Builder::new)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keying builders by each change's literal path breaks dependent changes after a rename: validation treats names as evolving, but the compiled core batch targets the initial schema. The request is accepted and then fails instead of producing the requested schema.

Canonicalize post-rename paths back to the original source builder, or reject rename-dependent sequences during prevalidation when the core batch cannot represent them.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName
    + " (id BIGINT NOT NULL, name STRING NOT NULL)");
catalog.alterTable(
    ident,
    TableChange.renameColumn(new String[] {"name"}, "full_name"),
    TableChange.updateColumnNullability(
        new String[] {"full_name"}, true));

assertArrayEquals(
    new String[] {"id", "full_name"},
    catalog.loadTable(ident).schema().fieldNames());

The call threw IllegalArgumentException: Column "full_name" does not exist from Dataset.alterColumns.

UpdateColumnNullability updateNull = (UpdateColumnNullability) change;
builders
.computeIfAbsent(path(updateNull.fieldNames()), ColumnAlteration.Builder::new)
.nullable(updateNull.nullable());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated nullability edits on one path are reduced to the last builder value, so required intermediate validation is silently skipped. Spark's TableCatalog.alterTable contract requires changes to be applied in order.

Reject non-equivalent repeated transitions before mutation, or perform every data-dependent intermediate validation before coalescing them into one core operation.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName
    + " (id BIGINT NOT NULL, name STRING)");
spark.sql("INSERT INTO " + fullName + " VALUES (1, NULL)");

assertThrows(
    RuntimeException.class,
    () -> catalog.alterTable(
        ident,
        TableChange.updateColumnNullability(
            new String[] {"name"}, false)));

assertThrows(
    RuntimeException.class,
    () -> catalog.alterTable(
        ident,
        TableChange.updateColumnNullability(
            new String[] {"name"}, false),
        TableChange.updateColumnNullability(
            new String[] {"name"}, true)));

The single SET NOT NULL assertion passed because existing data contains NULL; the ordered two-change assertion failed because no exception was thrown and the final true overwrote the required first transition.

return Kind.ADD;
} else if (change instanceof DeleteColumn) {
DeleteColumn delete = (DeleteColumn) change;
if (topLevelFields.contains(topLevelName(delete.fieldNames()))) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This existence check truncates every field path to its top-level name. For a nested IF EXISTS request, s.missing is treated as present whenever s exists, then sent to the core and rejected. That violates the DeleteColumn.ifExists no-error contract and the documented top-level-only validation boundary.

Reject every multi-part DROP/RENAME/nullability path before mutation, or implement full-path existence tracking rather than using only element zero.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName
    + " (id BIGINT NOT NULL, s STRUCT<x: INT>)");

catalog.alterTable(
    ident,
    TableChange.deleteColumn(
        new String[] {"s", "missing"}, true));

assertArrayEquals(
    new String[] {"id", "s"},
    catalog.loadTable(ident).schema().fieldNames());

The no-op request threw IllegalArgumentException: Column s.missing does not exist from Dataset.dropColumns.

Address review feedback on the single-mutation compiler: the batched
core operation applies to the current schema with no ordering between
its entries, so validating against an evolving schema (or coalescing by
path) accepted requests the core cannot represent.

- Validate every referenced column against the current schema, not an
  evolving one, so a change that depends on an earlier change in the same
  request (e.g. altering a column by its just-assigned new name) is
  rejected before any write instead of failing mid-commit.
- Reject a request that targets the same column more than once: order is
  lost when changes collapse into one batch, and intermediate validation
  would be skipped.
- Reject nested (multi-part) column paths up front (top-level only),
  instead of truncating to the leading path element and sending a path
  the core rejects — this keeps DROP COLUMN IF EXISTS's no-error contract
  for nested paths.

Adds tests for rename-dependent rejection, repeated-target rejection,
nested-path rejection, and a distinct-column rename+nullability batch;
documents the restrictions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

This revision resolves the prior dependent-target, repeated-target, and nested-path findings, but the compiler still does not preserve exact column identities and ordered rename semantics. Both remaining failures share the same boundary: accepted changes must be equivalent to Spark's ordered request while Lance source paths must identify top-level fields unambiguously.

A viable revision should separately simulate rename destination occupancy in request order while retaining the current-schema representability checks, and restore canonical Lance paths for every DROP/RENAME/nullability source.

RenameColumn rename = (RenameColumn) change;
requireTopLevel(rename.fieldNames(), "Renaming nested columns");
requireExists(currentFields, rename.fieldNames()[0]);
requireDistinct(touched, rename.fieldNames()[0]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validates each rename source against the current schema but never checks destination occupancy in request order. Because the core batch has no entry ordering, an invalid first rename can be accepted as a simultaneous rename, violating Spark's ordered alterTable contract.

Keep the current-schema checks needed by the core, but also simulate source removal and destination insertion in request order; reject a rename when its destination is occupied at that step.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName + " (a INT, b INT)");

assertThrows(
    RuntimeException.class,
    () -> catalog.alterTable(
        ident,
        TableChange.renameColumn(new String[] {"a"}, "b"),
        TableChange.renameColumn(new String[] {"b"}, "c")));

assertArrayEquals(
    new String[] {"a", "b"},
    catalog.loadTable(ident).schema().fieldNames());

The assertThrows assertion failed because the call returned successfully. In ordered execution, a -> b must fail before b -> c because b already exists.

if (delete.ifExists() && !currentFields.contains(name)) {
continue;
}
toDrop.add(name);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes a column name as an unescaped Lance field path. A legal top-level name containing a backtick validates successfully, but the core path parser cannot resolve the raw name; the same issue applies to the raw source paths created at lines 216 and 220.

Use FieldPathUtils.canonicalPath for DROP and ColumnAlteration source paths so special characters are quoted and embedded backticks are escaped.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName + " (`a``b` INT, keep INT)");

catalog.alterTable(
    ident,
    TableChange.deleteColumn(new String[] {"a`b"}, false));

assertArrayEquals(
    new String[] {"keep"},
    catalog.loadTable(ident).schema().fieldNames());

The dataset was created successfully, but the drop threw IllegalArgumentException: Column ab does not exist`.

Address review feedback on the single-mutation compiler:

- Simulate rename source-removal and destination-insertion in request
  order during validation, rejecting a rename whose destination name is
  already occupied at that step. The core alterColumns batch is unordered,
  so without this an ordered request like RENAME a->b, b->c would be
  wrongly accepted as simultaneous renames instead of failing on the
  a->b collision.
- Pass top-level column names to dropColumns/alterColumns verbatim (the
  Lance path for a top-level field is the name itself), with a test for a
  special-character column name.

Adds tests for ordered rename-to-occupied rejection and a
special-character column drop.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The ordered-rename fix resolves that part of the prior review, but this revision still does not preserve exact top-level column identities when a name contains Lance path syntax.

A viable revision should canonicalize every DROP, RENAME, and nullability source path before passing it to Lance.

continue;
}
// Only top-level columns are supported, so the Lance path is the field name verbatim.
toDrop.add(name);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing name verbatim to a Lance field-path API breaks legal top-level columns whose names contain path syntax. The request passes Spark-side validation but an accepted DROP fails; the same raw-source defect remains in the ColumnAlteration builders at lines 238 and 242.

Use FieldPathUtils.canonicalPath for the DROP and all ColumnAlteration source paths so special characters are quoted and embedded backticks are escaped.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName + " (`a``b` INT, keep INT)");

catalog.alterTable(
    ident,
    TableChange.deleteColumn(new String[] {"a`b"}, false));

assertArrayEquals(
    new String[] {"keep"},
    catalog.loadTable(ident).schema().fieldNames());

Expected the table schema to contain only keep; the DROP instead threw IllegalArgumentException: Column ab does not exist`.

Escape every DROP, RENAME, and nullability source column name into a
canonical Lance field path (via FieldPathUtils.canonicalPath) before
passing it to the core, so top-level names containing path syntax (dots,
spaces, etc.) resolve to the intended field instead of being rejected or
misparsed.

Adds special-character-name drop and rename tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The new canonicalization fixes RENAME and nullability source paths, but DROP still does not preserve exact top-level column identities because the core drop API rejects embedded backticks under both canonical and raw representations.

A viable revision should use a DROP representation proven against embedded backticks, or reject those requests before mutation until the core supports them.

if (delete.ifExists() && !currentFields.contains(name)) {
continue;
}
toDrop.add(canonicalPath(name));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Canonicalizing the DROP name still leaves a legal Spark column undroppable. For a field named a`b, this passes the canonical string shown below, but Dataset.dropColumns treats it as an absent literal field rather than resolving the actual name. RENAME and nullability passed with the same special name, so this is specific to the DROP API boundary.

field name:     a`b
DROP argument:  `a``b`

If this core version has no exact representation for such names (the raw name fails too), detect and reject these DROP requests before dropColumns; otherwise use a proven DROP-specific representation. Keep canonical paths for ColumnAlteration.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName + " (`a``b` INT, keep INT)");

catalog.alterTable(
    ident,
    TableChange.deleteColumn(new String[] {"a`b"}, false));

assertArrayEquals(
    new String[] {"keep"},
    catalog.loadTable(ident).schema().fieldNames());

Expected the schema to contain only keep; instead dropColumns threw RuntimeException because the canonical argument was not found while the available field was a`b.

The current core drop API cannot resolve a column whose name contains a
backtick under either the canonical (escaped) or raw representation.
Detect such names during validation and reject the DROP up front with a
clear message, instead of letting it fail mid-commit with a confusing
"field not found". RENAME and nullability keep using canonical paths,
which the core resolves correctly.

Adds a test for the backtick-name DROP rejection.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The new core-capability guard fixes the existing-field failure, but it changes DROP COLUMN IF EXISTS into an error when a missing requested name contains a backtick.

A viable revision should resolve absence first and apply the unsupported-name rejection only to an existing field that would reach dropColumns.

// The current core drop API cannot resolve a column whose name contains a backtick under
// either the canonical (escaped) or raw representation, so reject it up front instead of
// failing mid-commit with a confusing "field not found".
if (name.indexOf('`') >= 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard runs before the missing/ifExists branch, so a missing top-level name containing a backtick throws instead of being ignored. IF EXISTS is an existence contract and must not depend on whether an absent spelling would be representable by the core.

Check absence first: allow a missing ifExists change to reach the existing skip path, preserve the missing-column error when it is false, and reject the backtick only when the field actually exists.

Reproducer run on this head
spark.sql("CREATE TABLE " + fullName + " (id INT)");

catalog.alterTable(
    ident,
    TableChange.deleteColumn(new String[] {"missing`name"}, true));

assertArrayEquals(
    new String[] {"id"},
    catalog.loadTable(ident).schema().fieldNames());

Expected a no-op; the call instead threw UnsupportedOperationException: Dropping a column whose name contains a backtick is not supported by the current Lance version: missingname`.

Resolve column absence before applying the backtick-representability
guard, so DROP COLUMN IF EXISTS on a missing column is a no-op regardless
of how the absent name is spelled. The unsupported-backtick rejection now
applies only to a column that actually exists and would reach dropColumns;
a plain DROP of a missing column still errors.

Adds a test for DROP COLUMN IF EXISTS on a missing backtick name.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: approve.

The missing-name ordering fix now preserves DROP COLUMN IF EXISTS while rejecting only existing backtick-named fields that the current core cannot drop. The complete change keeps accepted schema evolution atomic and faithful to Spark’s ordered request semantics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

schema evolution DDLs

1 participant