From efe330040434db9a94d80fbffd95f203d1fe3852 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Wed, 12 Aug 2026 10:05:42 +0800 Subject: [PATCH] feat: validate bound function call fields --- rust/lancedb/src/function/binding_snapshot.rs | 175 +++++++- ...erated_column_binding_snapshot_contract.rs | 392 +++++++++++++++++- 2 files changed, 559 insertions(+), 8 deletions(-) diff --git a/rust/lancedb/src/function/binding_snapshot.rs b/rust/lancedb/src/function/binding_snapshot.rs index 6fcabe895..b2868c483 100644 --- a/rust/lancedb/src/function/binding_snapshot.rs +++ b/rust/lancedb/src/function/binding_snapshot.rs @@ -11,7 +11,7 @@ use std::collections::HashSet; use arrow_schema::FieldRef; -use super::invalid_input; +use super::{FunctionCall, invalid_input}; use crate::Result; /// One top-level field identity from a single table snapshot. @@ -114,6 +114,45 @@ impl GeneratedColumnBindingSnapshot { .iter() .find(|entry| entry.field().name() == name) } + + /// Validate table-dependent field arguments of an already canonical call. + /// + /// For every field argument, finds the snapshot entry by stable Lance field + /// ID and requires exact Arrow [`arrow_schema::DataType`] equality. Literal + /// arguments are table-independent and ignored. Missing field ID or type + /// mismatch returns [`crate::Error::InvalidInput`] without modifying `call` + /// or this snapshot. + /// + /// This check is orthogonal to [`FunctionCall::validate_against`]: it does + /// not perform catalog lookup, Function identity/signature validation, or + /// table mutation. + pub fn validate_field_arguments(&self, call: &FunctionCall) -> Result<()> { + for (_parameter, argument) in call.arguments() { + let Some(field_id) = argument.field_id() else { + continue; + }; + let Some(entry) = self.entry_by_field_id(field_id) else { + return Err(invalid_input(format!( + "generated-column binding snapshot missing field id {field_id}" + ))); + }; + let expected = argument.data_type(); + let current = entry.field().data_type(); + if current != expected { + return Err(invalid_input(format!( + "generated-column binding snapshot field id {field_id} type mismatch: \ + expected {expected}, found {current}" + ))); + } + } + Ok(()) + } + + fn entry_by_field_id(&self, field_id: i32) -> Option<&GeneratedColumnBindingEntry> { + self.entries + .iter() + .find(|entry| entry.field_id() == field_id) + } } #[cfg(test)] @@ -183,4 +222,138 @@ mod tests { Err(Error::InvalidInput { .. }) )); } + + fn sample_function() -> crate::function::Function { + use crate::function::{ + Function, FunctionId, FunctionOutput, FunctionParameter, FunctionSignature, + }; + let id = FunctionId::try_new("fn.exact.snapshot.lib").unwrap(); + let signature = FunctionSignature::try_new( + vec![ + FunctionParameter::new("payload_arg", DataType::Utf8), + FunctionParameter::new("metric_arg", DataType::Int32), + ], + FunctionOutput::new(DataType::Int32, true), + ) + .unwrap(); + Function::new(id, signature) + } + + #[test] + fn validate_field_arguments_value_cases() { + use crate::function::{FunctionArgument, FunctionCall}; + use arrow_array::{ArrayRef, Int32Array}; + + let snapshot = GeneratedColumnBindingSnapshot::try_new(2, fields(), vec![2, 4, 8]).unwrap(); + let function = sample_function(); + + let valid = FunctionCall::try_new( + &function, + vec![ + ( + "payload_arg".to_string(), + FunctionArgument::try_field(2, DataType::Utf8).unwrap(), + ), + ( + "metric_arg".to_string(), + FunctionArgument::try_field(4, DataType::Int32).unwrap(), + ), + ], + ) + .unwrap(); + snapshot.validate_field_arguments(&valid).unwrap(); + + let missing = FunctionCall::try_new( + &function, + vec![ + ( + "payload_arg".to_string(), + FunctionArgument::try_field(99, DataType::Utf8).unwrap(), + ), + ( + "metric_arg".to_string(), + FunctionArgument::try_field(4, DataType::Int32).unwrap(), + ), + ], + ) + .unwrap(); + assert!(matches!( + snapshot.validate_field_arguments(&missing), + Err(Error::InvalidInput { .. }) + )); + + // Same stable ID, different Arrow type: exact-type equality must reject. + let type_mismatch = FunctionCall::try_new( + &function, + vec![ + ( + "payload_arg".to_string(), + // ID 4 is Int32 in the snapshot. + FunctionArgument::try_field(4, DataType::Utf8).unwrap(), + ), + ( + "metric_arg".to_string(), + FunctionArgument::try_field(4, DataType::Int32).unwrap(), + ), + ], + ) + .unwrap(); + let err = snapshot + .validate_field_arguments(&type_mismatch) + .unwrap_err(); + assert!(matches!(err, Error::InvalidInput { .. })); + let message = err.to_string(); + assert!(message.contains('4')); + assert!(message.contains("Utf8") && message.contains("Int32")); + assert!(!message.contains("Score") && !message.contains("text")); + + let mixed = FunctionCall::try_new( + &function, + vec![ + ( + "payload_arg".to_string(), + FunctionArgument::try_field(2, DataType::Utf8).unwrap(), + ), + ( + "metric_arg".to_string(), + FunctionArgument::try_literal( + Arc::new(Int32Array::from(vec![Some(1)])) as ArrayRef + ) + .unwrap(), + ), + ], + ) + .unwrap(); + snapshot.validate_field_arguments(&mixed).unwrap(); + + let literal_only_fn = { + use crate::function::{ + Function, FunctionId, FunctionOutput, FunctionParameter, FunctionSignature, + }; + Function::new( + FunctionId::try_new("fn.exact.snapshot.literal").unwrap(), + FunctionSignature::try_new( + vec![FunctionParameter::new("constant_arg", DataType::Int32)], + FunctionOutput::new(DataType::Int32, true), + ) + .unwrap(), + ) + }; + let literal_only = + FunctionCall::try_new( + &literal_only_fn, + vec![( + "constant_arg".to_string(), + FunctionArgument::try_literal( + Arc::new(Int32Array::from(vec![Some(9)])) as ArrayRef + ) + .unwrap(), + )], + ) + .unwrap(); + // Empty snapshot still accepts literal-only calls. + let empty = + GeneratedColumnBindingSnapshot::try_new(1, Vec::::new(), vec![]).unwrap(); + empty.validate_field_arguments(&literal_only).unwrap(); + } } diff --git a/rust/lancedb/tests/first_class_generated_column_binding_snapshot_contract.rs b/rust/lancedb/tests/first_class_generated_column_binding_snapshot_contract.rs index e3eca78ca..ed94317e9 100644 --- a/rust/lancedb/tests/first_class_generated_column_binding_snapshot_contract.rs +++ b/rust/lancedb/tests/first_class_generated_column_binding_snapshot_contract.rs @@ -1,19 +1,24 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright The LanceDB Authors -//! Contract tests for GeneratedColumnBindingSnapshot (FF-029). +//! Contract tests for GeneratedColumnBindingSnapshot (FF-029 / FF-030). //! -//! Pins the hidden value projection and Table seam used by generated-column -//! call binding. These tests intentionally fail to compile until that API -//! exists. They do not submit Jobs, mutate generated-column state, or resolve -//! authored Function calls. +//! Pins the hidden value projection, Table seam, and bound-call field +//! validation used by generated-column call binding. These tests intentionally +//! fail to compile until that API exists. They do not submit Jobs, mutate +//! generated-column state, or resolve authored Function calls. use std::sync::Arc; -use arrow_array::{Int32Array, RecordBatch, StringArray}; +use arrow_array::{ArrayRef, Int32Array, RecordBatch, StringArray}; use arrow_schema::{DataType, Field, Schema}; +use lance::dataset::NewColumnTransform; use lancedb::connect; -use lancedb::function::{GeneratedColumnBindingEntry, GeneratedColumnBindingSnapshot}; +use lancedb::function::{ + Function, FunctionArgument, FunctionCall, FunctionId, FunctionOutput, FunctionParameter, + FunctionSignature, GeneratedColumnBindingEntry, GeneratedColumnBindingSnapshot, +}; +use lancedb::table::ColumnAlteration; use lancedb::{Error, Result}; use tempfile::tempdir; @@ -25,6 +30,58 @@ fn sample_fields() -> Vec { ] } +fn sample_output() -> FunctionOutput { + FunctionOutput::new(DataType::Int32, true) +} + +/// Parameter names intentionally differ from table column names so any +/// name-based validation would fail these fixtures. +fn two_field_function() -> Result { + let id = FunctionId::try_new("fn.exact.binding.validate")?; + let signature = FunctionSignature::try_new( + vec![ + FunctionParameter::new("input_payload", DataType::Utf8), + FunctionParameter::new("metric_value", DataType::Int32), + ], + sample_output(), + )?; + Ok(Function::new(id, signature)) +} + +fn one_field_function() -> Result { + let id = FunctionId::try_new("fn.exact.binding.one-field")?; + let signature = FunctionSignature::try_new( + vec![FunctionParameter::new("payload_arg", DataType::Utf8)], + sample_output(), + )?; + Ok(Function::new(id, signature)) +} + +fn literal_only_function() -> Result { + let id = FunctionId::try_new("fn.exact.binding.literal-only")?; + let signature = FunctionSignature::try_new( + vec![FunctionParameter::new("constant_arg", DataType::Int32)], + sample_output(), + )?; + Ok(Function::new(id, signature)) +} + +fn mixed_function() -> Result { + let id = FunctionId::try_new("fn.exact.binding.mixed")?; + let signature = FunctionSignature::try_new( + vec![ + FunctionParameter::new("payload_arg", DataType::Utf8), + FunctionParameter::new("constant_arg", DataType::Int32), + ], + sample_output(), + )?; + Ok(Function::new(id, signature)) +} + +fn int_literal(value: Option) -> Result { + FunctionArgument::try_literal(Arc::new(Int32Array::from(vec![value])) as ArrayRef) +} + #[test] fn try_new_preserves_version_order_and_exact_lookup() -> Result<()> { let fields = sample_fields(); @@ -83,6 +140,123 @@ fn try_new_rejects_invalid_projections() { )); } +#[test] +fn validate_field_arguments_accepts_valid_and_mixed_bindings() -> Result<()> { + let snapshot = GeneratedColumnBindingSnapshot::try_new(3, sample_fields(), vec![3, 5, 9])?; + + let one = one_field_function()?; + let valid = FunctionCall::try_new( + &one, + vec![( + "payload_arg".to_string(), + FunctionArgument::try_field(3, DataType::Utf8)?, + )], + )?; + snapshot.validate_field_arguments(&valid)?; + + let two = two_field_function()?; + let multi = FunctionCall::try_new( + &two, + vec![ + ( + "input_payload".to_string(), + FunctionArgument::try_field(3, DataType::Utf8)?, + ), + ( + "metric_value".to_string(), + FunctionArgument::try_field(5, DataType::Int32)?, + ), + ], + )?; + snapshot.validate_field_arguments(&multi)?; + + let mixed_fn = mixed_function()?; + let mixed = FunctionCall::try_new( + &mixed_fn, + vec![ + ( + "payload_arg".to_string(), + FunctionArgument::try_field(3, DataType::Utf8)?, + ), + ("constant_arg".to_string(), int_literal(Some(42))?), + ], + )?; + snapshot.validate_field_arguments(&mixed)?; + Ok(()) +} + +#[test] +fn validate_field_arguments_rejects_missing_id_and_type_mismatch() -> Result<()> { + let snapshot = GeneratedColumnBindingSnapshot::try_new(3, sample_fields(), vec![3, 5, 9])?; + let one = one_field_function()?; + + let missing = FunctionCall::try_new( + &one, + vec![( + "payload_arg".to_string(), + FunctionArgument::try_field(99, DataType::Utf8)?, + )], + )?; + let err = snapshot + .validate_field_arguments(&missing) + .expect_err("missing stable field id"); + assert!(matches!(err, Error::InvalidInput { .. })); + let message = err.to_string(); + assert!( + message.contains("99"), + "diagnostics may name field id: {message}" + ); + assert!( + !message.contains("text") && !message.contains("score") && !message.contains("a.b"), + "diagnostics must not invent or use a column name: {message}" + ); + + // Same stable ID, different Arrow type: exact-type equality must reject. + // This covers Remote/other producer projections that keep the ID. + let type_mismatch = FunctionCall::try_new( + &one, + vec![( + "payload_arg".to_string(), + FunctionArgument::try_field(5, DataType::Utf8)?, + )], + )?; + let err = snapshot + .validate_field_arguments(&type_mismatch) + .expect_err("same-id exact type mismatch"); + assert!(matches!(err, Error::InvalidInput { .. })); + let message = err.to_string(); + assert!( + message.contains("5"), + "diagnostics may name field id: {message}" + ); + assert!( + message.contains("Utf8") && message.contains("Int32"), + "diagnostics may identify expected/current types: {message}" + ); + assert!( + !message.contains("score") && !message.contains("text"), + "diagnostics must not invent or use a column name: {message}" + ); + Ok(()) +} + +#[test] +fn validate_field_arguments_literal_only_ignores_table_fields() -> Result<()> { + // Snapshot has no field that a name-based binder could match to "constant_arg". + let snapshot = GeneratedColumnBindingSnapshot::try_new( + 1, + vec![Arc::new(Field::new("unrelated", DataType::Utf8, true))], + vec![11], + )?; + let function = literal_only_function()?; + let call = FunctionCall::try_new( + &function, + vec![("constant_arg".to_string(), int_literal(Some(7))?)], + )?; + snapshot.validate_field_arguments(&call)?; + Ok(()) +} + #[tokio::test] async fn table_seam_returns_atomic_native_snapshot() -> Result<()> { let tmp = tempdir().unwrap(); @@ -115,3 +289,207 @@ async fn table_seam_returns_atomic_native_snapshot() -> Result<()> { } Ok(()) } + +#[tokio::test] +async fn validate_field_arguments_survives_rename_on_real_table() -> Result<()> { + let tmp = tempdir().unwrap(); + let db = connect(tmp.path().to_str().unwrap()).execute().await?; + // Column names deliberately differ from Function parameter names. + let schema = Arc::new(Schema::new(vec![ + Field::new("source_text", DataType::Utf8, true), + Field::new("source_score", DataType::Int32, false), + ])); + let batch = RecordBatch::try_new( + schema, + vec![ + Arc::new(StringArray::from(vec![Some("hello")])), + Arc::new(Int32Array::from(vec![7])), + ], + )?; + let table = db.create_table("binding_rename", batch).execute().await?; + + let before = table.generated_column_binding_snapshot().await?; + let text_entry = before.field("source_text").expect("source_text"); + let score_entry = before.field("source_score").expect("source_score"); + let text_id = text_entry.field_id(); + let score_id = score_entry.field_id(); + assert_eq!(text_entry.field().data_type(), &DataType::Utf8); + assert_eq!(score_entry.field().data_type(), &DataType::Int32); + + let function = two_field_function()?; + let call = FunctionCall::try_new( + &function, + vec![ + ( + "input_payload".to_string(), + FunctionArgument::try_field(text_id, DataType::Utf8)?, + ), + ( + "metric_value".to_string(), + FunctionArgument::try_field(score_id, DataType::Int32)?, + ), + ], + )?; + before.validate_field_arguments(&call)?; + + table + .alter_columns(&[ColumnAlteration::new("source_text".into()).rename("renamed_text".into())]) + .await?; + + let after = table.generated_column_binding_snapshot().await?; + assert!(after.field("source_text").is_none()); + let renamed = after.field("renamed_text").expect("renamed_text"); + assert_eq!(renamed.field_id(), text_id); + assert_eq!(renamed.field().data_type(), &DataType::Utf8); + assert_eq!( + after + .field("source_score") + .expect("source_score") + .field_id(), + score_id + ); + after.validate_field_arguments(&call)?; + Ok(()) +} + +#[tokio::test] +async fn validate_field_arguments_rejects_drop_recreate_same_name_type() -> Result<()> { + let tmp = tempdir().unwrap(); + let db = connect(tmp.path().to_str().unwrap()).execute().await?; + let schema = Arc::new(Schema::new(vec![ + Field::new("keep_col", DataType::Int32, false), + Field::new("bound_col", DataType::Utf8, true), + ])); + let batch = RecordBatch::try_new( + schema, + vec![ + Arc::new(Int32Array::from(vec![1])), + Arc::new(StringArray::from(vec![Some("v")])), + ], + )?; + let table = db + .create_table("binding_drop_recreate", batch) + .execute() + .await?; + + let before = table.generated_column_binding_snapshot().await?; + let bound = before.field("bound_col").expect("bound_col"); + let old_id = bound.field_id(); + assert_eq!(bound.field().data_type(), &DataType::Utf8); + + let function = one_field_function()?; + let call = FunctionCall::try_new( + &function, + vec![( + "payload_arg".to_string(), + FunctionArgument::try_field(old_id, DataType::Utf8)?, + )], + )?; + before.validate_field_arguments(&call)?; + + table.drop_columns(&["bound_col"]).await?; + table + .add_columns() + .transform(NewColumnTransform::SqlExpressions(vec![( + "bound_col".into(), + "cast(NULL as string)".into(), + )])) + .execute() + .await?; + + let after = table.generated_column_binding_snapshot().await?; + let recreated = after.field("bound_col").expect("recreated bound_col"); + assert_eq!(recreated.field().data_type(), &DataType::Utf8); + assert_ne!( + recreated.field_id(), + old_id, + "drop/recreate must allocate a new stable field id" + ); + let err = after + .validate_field_arguments(&call) + .expect_err("old call must not bind by name"); + assert!(matches!(err, Error::InvalidInput { .. })); + let message = err.to_string(); + assert!( + message.contains(&old_id.to_string()), + "diagnostics may name missing field id: {message}" + ); + assert!( + !message.contains("bound_col") && !message.contains("keep_col"), + "diagnostics must not invent or use a column name: {message}" + ); + Ok(()) +} + +#[tokio::test] +async fn validate_field_arguments_rejects_cast_that_allocates_new_field_id() -> Result<()> { + // Native Lance cast_to allocates a new stable field ID. The old bound call + // must fail because that ID is absent. Same-ID exact-type mismatch is proved + // separately via manually constructed snapshots (Remote/other producers). + let tmp = tempdir().unwrap(); + let db = connect(tmp.path().to_str().unwrap()).execute().await?; + let schema = Arc::new(Schema::new(vec![ + Field::new("label_col", DataType::Utf8, true), + Field::new("metric_col", DataType::Int32, false), + ])); + let batch = RecordBatch::try_new( + schema, + vec![ + Arc::new(StringArray::from(vec![Some("x")])), + Arc::new(Int32Array::from(vec![3])), + ], + )?; + let table = db + .create_table("binding_type_change", batch) + .execute() + .await?; + + let before = table.generated_column_binding_snapshot().await?; + let metric = before.field("metric_col").expect("metric_col"); + let old_metric_id = metric.field_id(); + assert_eq!(metric.field().data_type(), &DataType::Int32); + + let id = FunctionId::try_new("fn.exact.binding.type-change")?; + let function = Function::new( + id, + FunctionSignature::try_new( + vec![FunctionParameter::new("metric_value", DataType::Int32)], + sample_output(), + )?, + ); + let call = FunctionCall::try_new( + &function, + vec![( + "metric_value".to_string(), + FunctionArgument::try_field(old_metric_id, DataType::Int32)?, + )], + )?; + before.validate_field_arguments(&call)?; + + table + .alter_columns(&[ColumnAlteration::new("metric_col".into()).cast_to(DataType::Int64)]) + .await?; + + let after = table.generated_column_binding_snapshot().await?; + let casted = after.field("metric_col").expect("metric_col"); + assert_eq!(casted.field().data_type(), &DataType::Int64); + assert_ne!( + casted.field_id(), + old_metric_id, + "Native Lance cast_to must allocate a new stable field id" + ); + let err = after + .validate_field_arguments(&call) + .expect_err("old call must fail because the prior stable field id is absent"); + assert!(matches!(err, Error::InvalidInput { .. })); + let message = err.to_string(); + assert!( + message.contains(&old_metric_id.to_string()), + "diagnostics may name missing field id: {message}" + ); + assert!( + !message.contains("metric_col") && !message.contains("label_col"), + "diagnostics must not invent or use a column name: {message}" + ); + Ok(()) +}