Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 26 additions & 17 deletions rust/ffi/dataset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ use arrow_array::types::UInt64Type;
use datafusion::logical_expr::Expr;
use datafusion::physical_plan::SendableRecordBatchStream;
use datafusion::scalar::ScalarValue;
use datafusion_sql::unparser::expr_to_sql;
use datafusion_sql::unparser::dialect::CustomDialectBuilder;
use datafusion_sql::unparser::Unparser;
use futures::TryStreamExt;
use lance::dataset::builder::DatasetBuilder;
use lance::dataset::statistics::DatasetStatisticsExt;
Expand All @@ -28,6 +29,28 @@ use super::util::{
FfiResult,
};

/// Convert an optional filter expression into a predicate SQL string for
/// Lance's string-based `delete` API. Identifiers are always backtick-quoted so
/// column names that collide with SQL keywords (e.g. `name`, `value`, `key`,
/// `desc`) survive the unparse/re-parse round-trip. Lance's filter parser uses
/// backtick identifier quoting; double quotes would be treated as string
/// literals.
fn delete_filter_expr_to_sql(filter: Option<Expr>) -> FfiResult<String> {
match filter {
Some(expr) => {
let dialect = CustomDialectBuilder::new()
.with_identifier_quote_style('`')
.build();
let unparser = Unparser::new(&dialect);
let sql = unparser.expr_to_sql(&expr).map_err(|err| {
FfiError::new(ErrorCode::DatasetDelete, format!("predicate sql: {err}"))
})?;
Ok(sql.to_string())
}
None => Ok("true".to_string()),
}
}

#[repr(C)]
#[derive(Clone, Copy, Debug)]
pub struct LanceFieldStats {
Expand Down Expand Up @@ -699,14 +722,7 @@ fn delete_transaction_with_storage_options_inner(
"delete filter_ir",
)?
};
let predicate = match filter {
Some(expr) => expr_to_sql(&expr)
.map_err(|err| {
FfiError::new(ErrorCode::DatasetDelete, format!("predicate sql: {err}"))
})?
.to_string(),
None => "true".to_string(),
};
let predicate = delete_filter_expr_to_sql(filter)?;
let session = unsafe { optional_session_handle(session)? };

let (maybe_txn, deleted_rows) = match runtime::block_on(async {
Expand Down Expand Up @@ -854,14 +870,7 @@ fn dataset_delete_inner(
"delete filter_ir",
)?
};
let predicate = match filter {
Some(expr) => expr_to_sql(&expr)
.map_err(|err| {
FfiError::new(ErrorCode::DatasetDelete, format!("predicate sql: {err}"))
})?
.to_string(),
None => "true".to_string(),
};
let predicate = delete_filter_expr_to_sql(filter)?;

let mut ds = (*handle.dataset).clone();

Expand Down
13 changes: 6 additions & 7 deletions src/lance_delete.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,12 @@ static bool TryBuildLanceDeleteFilterIR(LogicalDelete &op,

vector<string> parts;

vector<ColumnIndex> column_indexes = get->GetColumnIds();
TableFunctionInitInput init_input(get->bind_data.get(),
std::move(column_indexes),
get->projection_ids, &get->table_filters);
auto table_filters =
BuildLanceTableFilterIRParts(names, types, init_input, false);
parts = std::move(table_filters.parts);
if (!TryBuildLanceTableFilterIRParts(names, types, get->table_filters,
parts)) {
out_error = "unsupported DELETE predicate for Lance: pushed-down table "
"filter could not be translated to Lance filter IR";
return false;
}

vector<const Expression *> predicates;
if (!TryCollectDeleteFilterPredicates(*op.children[0], predicates,
Expand Down
8 changes: 8 additions & 0 deletions src/lance_filter_ir.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,14 @@ bool TryBuildLanceTableFilterIRParts(const vector<string> &names,
if (!filter) {
continue;
}
// OPTIONAL_FILTER / DYNAMIC_FILTER are pruning hints that are not required
// for correctness: DuckDB pushes them as PUSHED_DOWN_PARTIALLY and keeps
// the exact predicate as a residual LogicalFilter (collected separately for
// DELETE). Skip them here rather than failing translation.
if (filter->filter_type == TableFilterType::OPTIONAL_FILTER ||

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 skips optional filters only when the optional node is the top-level table-filter entry. DuckDB can instead produce CONJUNCTION_AND(required comparison, OPTIONAL_FILTER(IN ...)); recursive translation reaches the optional child, returns false, and rejects a valid DELETE. Use a tri-state result (encoded / skip / unsupported) so AND can omit safely optional children while preserving required conjuncts, while unsupported required nodes still fail closed.

Reproducer run against this head
ATTACH 'mixed_filter_repro.lance' AS ns (TYPE LANCE);
CREATE TABLE ns.main.t(s VARCHAR);
INSERT INTO ns.main.t VALUES ('a'), ('b'), ('c'), ('d');
DELETE FROM ns.main.t WHERE s >= 'c' AND s IN ('b', 'd');
SELECT * FROM ns.main.t ORDER BY s;

Expected: DELETE succeeds and the query returns a, b, c.
Observed: DELETE reports Not implemented Error: unsupported DELETE predicate for Lance: pushed-down table filter could not be translated to Lance filter IR, the debug CLI exits 134 during Tokio session cleanup, and a fresh read returns all four rows.

filter->filter_type == TableFilterType::DYNAMIC_FILTER) {
continue;
}
if (col_id == COLUMN_IDENTIFIER_ROW_ID ||
col_id == COLUMN_IDENTIFIER_EMPTY) {
return false;
Expand Down
24 changes: 24 additions & 0 deletions test/sql/dml_delete_keyword_column.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# name: test/sql/dml_delete_keyword_column.test
# description: DELETE predicates on columns whose names are SQL keywords still match rows
# group: [sql]

require lance

statement ok
COPY (
SELECT 'x'::VARCHAR AS name, 1::BIGINT AS id
UNION ALL
SELECT 'y'::VARCHAR AS name, 2::BIGINT AS id
) TO 'test/.tmp/lance_delete_keyword.lance' (FORMAT lance, mode 'overwrite');

statement ok
ATTACH 'test/.tmp' AS del_kw_ns (TYPE LANCE);

# 'name' is a SQL keyword; the predicate must still match and delete the row.
statement ok
DELETE FROM del_kw_ns.main.lance_delete_keyword WHERE name = 'x';

query TI
SELECT name, id FROM del_kw_ns.main.lance_delete_keyword ORDER BY id;
----
y 2
29 changes: 29 additions & 0 deletions test/sql/dml_delete_non_first_column_filter.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# name: test/sql/dml_delete_non_first_column_filter.test
# description: DELETE with a predicate on a non-first column deletes only matching rows
# group: [sql]

require lance

statement ok
COPY (
SELECT 1::BIGINT AS id, 'a'::VARCHAR AS s, 0::BIGINT AS flag
UNION ALL
SELECT 2::BIGINT AS id, 'b'::VARCHAR AS s, 1::BIGINT AS flag
UNION ALL
SELECT 3::BIGINT AS id, 'c'::VARCHAR AS s, 0::BIGINT AS flag
UNION ALL
SELECT 4::BIGINT AS id, 'd'::VARCHAR AS s, 1::BIGINT AS flag
) TO 'test/.tmp/lance_delete_non_first.lance' (FORMAT lance, mode 'overwrite');

statement ok
ATTACH 'test/.tmp' AS del_col_ns (TYPE LANCE);

# Predicate on the non-first column: only the flag=1 rows may be removed.
statement ok
DELETE FROM del_col_ns.main.lance_delete_non_first WHERE flag = 1;

query IT
SELECT id, s FROM del_col_ns.main.lance_delete_non_first ORDER BY id;
----
1 a
3 c