mirror of
https://github.com/lancedb/lancedb.git
synced 2026-10-12 14:52:26 +00:00
fix: reject empty column alterations and unknown Python keys (#4417)
Fixes #4390 Lance accepts column alterations with every change field unset, so a path-only Python call could commit a new local version without changing the schema. The Python binding also silently discarded unknown keys such as `nulable`. Validate every alteration in the shared Rust `Table` API before dispatching to either backend, and reject unknown dictionary keys in the Python binding while preserving the legacy `name` alias. Invalid batches leave local schema and version unchanged and send no remote request. Validation: - Reproduced both reported calls on the original extension: versions advanced from 1 to 2 to 3 with an unchanged schema; all 13 new invalid-input Python cases failed before the fix. - Rebuilt the extension with `uv run --extra tests --extra dev maturin develop --extras tests,dev`; all 18 focused Python cases pass across sync/async and local/mocked remote tables. - `cargo test --quiet --features remote -p lancedb --lib alter_column`: all eight tests pass, including both new regressions. - `cargo clippy --quiet --features remote --tests --examples` passes across the workspace. - `cargo fmt --all`, `ruff check .`, and `ruff format .` pass. - `typos` 1.26.0 passes across the repository; intentional misspellings in regression fixtures use the existing line-specific spelling exemptions. <!-- lance-gatekeeper-fix:v1 agent=36a547cd11108898893456042e38628b generation=1 --> --------- Co-authored-by: Gatefixer <313497061+lancedb-gatefixer[bot]@users.noreply.github.com>
This commit is contained in:
@@ -2617,6 +2617,10 @@ class Table(ABC):
|
||||
to nullable. Currently, you cannot change a nullable column to
|
||||
non-nullable.
|
||||
|
||||
Each alteration must specify at least one of "rename", "data_type",
|
||||
or "nullable". The legacy key "name" is also accepted for renaming.
|
||||
Unknown keys raise ValueError before any alterations are applied.
|
||||
|
||||
Returns
|
||||
-------
|
||||
AlterColumnsResult
|
||||
@@ -6874,6 +6878,10 @@ class AsyncTable:
|
||||
to nullable. Currently, you cannot change a nullable column to
|
||||
non-nullable.
|
||||
|
||||
Each alteration must specify at least one of "rename", "data_type",
|
||||
or "nullable". The legacy key "name" is also accepted for renaming.
|
||||
Unknown keys raise ValueError before any alterations are applied.
|
||||
|
||||
Returns
|
||||
-------
|
||||
AlterColumnsResult
|
||||
|
||||
@@ -119,6 +119,77 @@ async def test_async_remote_db():
|
||||
assert table_names == []
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"alteration, match",
|
||||
[
|
||||
({"path": "id"}, "One of rename, nullable or data_type"),
|
||||
(
|
||||
{"path": "id", "nulable": False}, # spellchecker:disable-line
|
||||
"Unknown column alteration key 'nulable'", # spellchecker:disable-line
|
||||
),
|
||||
(
|
||||
{
|
||||
"path": "id",
|
||||
"rename": "new_id",
|
||||
"nulable": False, # spellchecker:disable-line
|
||||
},
|
||||
"Unknown column alteration key 'nulable'", # spellchecker:disable-line
|
||||
),
|
||||
],
|
||||
)
|
||||
def test_remote_alter_columns_rejects_invalid_before_request(alteration, match):
|
||||
requests = []
|
||||
|
||||
def handler(request):
|
||||
requests.append(request.path)
|
||||
request.send_response(200)
|
||||
request.send_header("Content-Type", "application/json")
|
||||
request.end_headers()
|
||||
request.wfile.write(b'{"version": 1, "schema": {"fields": []}}')
|
||||
|
||||
with mock_lancedb_connection(handler) as db:
|
||||
table = db.open_table("test")
|
||||
requests.clear()
|
||||
|
||||
with pytest.raises(ValueError, match=match):
|
||||
table.alter_columns(alteration)
|
||||
|
||||
assert requests == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"alteration, match",
|
||||
[
|
||||
({"path": "id"}, "One of rename, nullable or data_type"),
|
||||
(
|
||||
{"path": "id", "nulable": False}, # spellchecker:disable-line
|
||||
"Unknown column alteration key 'nulable'", # spellchecker:disable-line
|
||||
),
|
||||
],
|
||||
)
|
||||
async def test_async_remote_alter_columns_rejects_invalid_before_request(
|
||||
alteration, match
|
||||
):
|
||||
requests = []
|
||||
|
||||
def handler(request):
|
||||
requests.append(request.path)
|
||||
request.send_response(200)
|
||||
request.send_header("Content-Type", "application/json")
|
||||
request.end_headers()
|
||||
request.wfile.write(b'{"version": 1, "schema": {"fields": []}}')
|
||||
|
||||
async with mock_lancedb_connection_async(handler) as db:
|
||||
table = await db.open_table("test")
|
||||
requests.clear()
|
||||
|
||||
with pytest.raises(ValueError, match=match):
|
||||
await table.alter_columns(alteration)
|
||||
|
||||
assert requests == []
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_async_checkout():
|
||||
def handler(request):
|
||||
|
||||
@@ -4170,14 +4170,74 @@ async def test_add_columns_with_schema(mem_db_async: AsyncConnection):
|
||||
)
|
||||
|
||||
|
||||
def test_alter_columns(mem_db: DBConnection):
|
||||
@pytest.mark.parametrize("rename_key", ["rename", "name"])
|
||||
def test_alter_columns(mem_db: DBConnection, rename_key):
|
||||
data = pa.table({"id": [0, 1]})
|
||||
table = mem_db.create_table("my_table", data=data)
|
||||
alter_columns_res = table.alter_columns({"path": "id", "rename": "new_id"})
|
||||
alter_columns_res = table.alter_columns({"path": "id", rename_key: "new_id"})
|
||||
assert alter_columns_res.version == 2
|
||||
assert table.to_arrow().column_names == ["new_id"]
|
||||
|
||||
|
||||
INVALID_COLUMN_ALTERATIONS = [
|
||||
(({"path": "id"},), "One of rename, nullable or data_type"),
|
||||
(
|
||||
({"path": "id", "nulable": False},), # spellchecker:disable-line
|
||||
"Unknown column alteration key 'nulable'", # spellchecker:disable-line
|
||||
),
|
||||
(
|
||||
(
|
||||
{
|
||||
"path": "id",
|
||||
"rename": "new_id",
|
||||
"nulable": False, # spellchecker:disable-line
|
||||
},
|
||||
),
|
||||
"Unknown column alteration key 'nulable'", # spellchecker:disable-line
|
||||
),
|
||||
(
|
||||
({"path": "id", "rename": "new_id"}, {"path": "id"}),
|
||||
"One of rename, nullable or data_type",
|
||||
),
|
||||
]
|
||||
|
||||
|
||||
def test_alter_columns_nullable_false(mem_db: DBConnection):
|
||||
table = mem_db.create_table("my_table", data=pa.table({"id": [0, 1]}))
|
||||
result = table.alter_columns({"path": "id", "nullable": False})
|
||||
assert result.version == 2
|
||||
assert not table.schema.field("id").nullable
|
||||
|
||||
|
||||
@pytest.mark.parametrize("alterations, match", INVALID_COLUMN_ALTERATIONS)
|
||||
def test_alter_columns_rejects_invalid(mem_db: DBConnection, alterations, match):
|
||||
table = mem_db.create_table("my_table", data=pa.table({"id": [0, 1]}))
|
||||
initial_version = table.version
|
||||
initial_schema = table.schema
|
||||
|
||||
with pytest.raises(ValueError, match=match):
|
||||
table.alter_columns(*alterations)
|
||||
|
||||
assert table.version == initial_version
|
||||
assert table.schema == initial_schema
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("alterations, match", INVALID_COLUMN_ALTERATIONS)
|
||||
async def test_alter_columns_rejects_invalid_async(
|
||||
mem_db_async: AsyncConnection, alterations, match
|
||||
):
|
||||
table = await mem_db_async.create_table("my_table", data=pa.table({"id": [0, 1]}))
|
||||
initial_version = await table.version()
|
||||
initial_schema = await table.schema()
|
||||
|
||||
with pytest.raises(ValueError, match=match):
|
||||
await table.alter_columns(*alterations)
|
||||
|
||||
assert await table.version() == initial_version
|
||||
assert await table.schema() == initial_schema
|
||||
|
||||
|
||||
def test_update_field_metadata(mem_db: DBConnection):
|
||||
data = pa.table({"id": [0, 1], "category": ["a", "b"]})
|
||||
table = mem_db.create_table("my_table", data=data)
|
||||
@@ -4203,10 +4263,11 @@ def test_update_field_metadata(mem_db: DBConnection):
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_alter_columns_async(mem_db_async: AsyncConnection):
|
||||
@pytest.mark.parametrize("rename_key", ["rename", "name"])
|
||||
async def test_alter_columns_async(mem_db_async: AsyncConnection, rename_key):
|
||||
data = pa.table({"id": [0, 1]})
|
||||
table = await mem_db_async.create_table("my_table", data=data)
|
||||
alter_columns_res = await table.alter_columns({"path": "id", "rename": "new_id"})
|
||||
alter_columns_res = await table.alter_columns({"path": "id", rename_key: "new_id"})
|
||||
assert alter_columns_res.version == 2
|
||||
assert (await table.to_arrow()).column_names == ["new_id"]
|
||||
alter_columns_res = await table.alter_columns(
|
||||
|
||||
@@ -1897,6 +1897,17 @@ impl Table {
|
||||
let alterations = alterations
|
||||
.iter()
|
||||
.map(|alteration| {
|
||||
for key in alteration.keys().iter() {
|
||||
let key = key.extract::<String>()?;
|
||||
if !matches!(
|
||||
key.as_str(),
|
||||
"path" | "rename" | "name" | "nullable" | "data_type"
|
||||
) {
|
||||
return Err(PyValueError::new_err(format!(
|
||||
"Unknown column alteration key '{key}'"
|
||||
)));
|
||||
}
|
||||
}
|
||||
let path = alteration
|
||||
.get_item("path")?
|
||||
.ok_or_else(|| PyValueError::new_err("Missing path"))?
|
||||
|
||||
@@ -4694,6 +4694,25 @@ mod tests {
|
||||
assert_eq!(result.rows_updated, if old_server { 0 } else { 5 });
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_alter_columns_rejects_missing_changes_before_request() {
|
||||
let table = Table::new_with_handler::<String>("my_table", |request| {
|
||||
panic!("Unexpected request: {}", request.url().path())
|
||||
});
|
||||
|
||||
for alterations in [
|
||||
vec![ColumnAlteration::new("id".into())],
|
||||
vec![
|
||||
ColumnAlteration::new("id".into()).rename("new_id".into()),
|
||||
ColumnAlteration::new("id".into()),
|
||||
],
|
||||
] {
|
||||
let err = table.alter_columns(&alterations).await.unwrap_err();
|
||||
assert!(matches!(err, Error::InvalidInput { .. }), "got {err:?}");
|
||||
assert!(err.to_string().contains("path 'id'"));
|
||||
}
|
||||
}
|
||||
|
||||
#[rstest]
|
||||
#[case(true)]
|
||||
#[case(false)]
|
||||
|
||||
@@ -1919,11 +1919,28 @@ impl Table {
|
||||
self.inner.function_errors(&request).await
|
||||
}
|
||||
|
||||
/// Change a column's name or nullability.
|
||||
/// Change a column's name, data type, or nullability.
|
||||
///
|
||||
/// Each alteration must specify at least one of `rename`, `data_type`, or
|
||||
/// `nullable`. Returns [`Error::InvalidInput`] before applying any changes
|
||||
/// if an alteration does not specify any of these fields.
|
||||
pub async fn alter_columns(
|
||||
&self,
|
||||
alterations: &[ColumnAlteration],
|
||||
) -> Result<AlterColumnsResult> {
|
||||
for alteration in alterations {
|
||||
if alteration.rename.is_none()
|
||||
&& alteration.nullable.is_none()
|
||||
&& alteration.data_type.is_none()
|
||||
{
|
||||
return Err(Error::InvalidInput {
|
||||
message: format!(
|
||||
"One of rename, nullable or data_type must be specified for path '{}'",
|
||||
alteration.path
|
||||
),
|
||||
});
|
||||
}
|
||||
}
|
||||
self.inner.alter_columns(alterations).await
|
||||
}
|
||||
|
||||
|
||||
@@ -807,6 +807,29 @@ mod tests {
|
||||
|
||||
// Alter Columns Tests
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_alter_columns_rejects_missing_changes() {
|
||||
let conn = connect("memory://").execute().await.unwrap();
|
||||
let batch = record_batch!(("id", Int32, [1, 2, 3])).unwrap();
|
||||
let table = conn.create_table("test", batch).execute().await.unwrap();
|
||||
let initial_version = table.version().await.unwrap();
|
||||
let initial_schema = table.schema().await.unwrap();
|
||||
|
||||
for alterations in [
|
||||
vec![ColumnAlteration::new("id".into())],
|
||||
vec![
|
||||
ColumnAlteration::new("id".into()).rename("new_id".into()),
|
||||
ColumnAlteration::new("id".into()),
|
||||
],
|
||||
] {
|
||||
let err = table.alter_columns(&alterations).await.unwrap_err();
|
||||
assert!(matches!(err, Error::InvalidInput { .. }), "got {err:?}");
|
||||
assert!(err.to_string().contains("path 'id'"));
|
||||
assert_eq!(table.version().await.unwrap(), initial_version);
|
||||
assert_eq!(table.schema().await.unwrap(), initial_schema);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_alter_column_rename() {
|
||||
let conn = connect("memory://").execute().await.unwrap();
|
||||
|
||||
Reference in New Issue
Block a user