Skip to content

Commit 2306a4b

Browse files
fix(core): reject a DELETE or an UPDATE whose WHERE clause cannot reach the provider (#24657)
## Which issue does this PR close? #24654 ## Rationale for this change See ticket. ## What changes are included in this PR? A `DELETE` or an `UPDATE` whose `WHERE` clause holds an `IN` or an `EXISTS` subquery changed every row of the target table, and reported the whole table as affected. The optimizer rewrites the subquery into a semi join, so the condition leaves the `Filter` nodes that `extract_dml_filters()` reads. The provider then received an empty filter list, which is the encoding for "no WHERE clause", and applied the statement to all rows. An always-false `WHERE` clause reached the provider the same way. The simplifier folds the predicate into an empty relation, so again no filter survived, and a `DELETE FROM t WHERE false` emptied the table. Add `classify_dml_input()`, which walks the input plan of a `DELETE` or an `UPDATE` before the provider hook runs: - an empty relation means that no row matches, so the statement reports a count of 0 and the hook is not called; - a join, a predicate on another table, or any other node that restricts or multiplies rows raises a "not implemented" error, and the hook is not called. The hook stays untouched in every rejected case, so a provider that writes to durable storage cannot lose rows. ## Are these changes tested? Only with the tests provided in this patch, which are based on the assumptions made in the ticket description. ## Are there any user-facing changes? ?
1 parent 5b8fcf1 commit 2306a4b

4 files changed

Lines changed: 729 additions & 40 deletions

File tree

‎datafusion/core/src/physical_planner.rs‎

Lines changed: 193 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ use crate::physical_plan::{
5757
};
5858
use crate::schema_equivalence::schema_satisfied_by;
5959

60-
use arrow::array::{RecordBatch, builder::StringBuilder};
60+
use arrow::array::{ArrayRef, RecordBatch, UInt64Array, builder::StringBuilder};
6161
use arrow::compute::SortOptions;
6262
use arrow::datatypes::Schema;
6363
use arrow_schema::Field;
@@ -794,17 +794,29 @@ impl DefaultPhysicalPlanner {
794794
target,
795795
op: WriteOp::Delete,
796796
input,
797-
..
797+
output_schema,
798798
}) => {
799799
if let Some(provider) = target.downcast_ref::<DefaultTableSource>() {
800-
let filters = extract_dml_filters(input, table_name)?;
801-
provider
802-
.table_provider
803-
.delete_from(session_state, filters)
804-
.await
805-
.map_err(|e| {
806-
e.context(format!("DELETE operation on table '{table_name}'"))
807-
})?
800+
let allowed_refs = collect_dml_target_refs(input, table_name)?;
801+
match classify_dml_input(input, table_name, &allowed_refs, "DELETE")?
802+
{
803+
DmlInput::NoRows => {
804+
zero_rows_affected_exec(Arc::clone(output_schema.inner()))?
805+
}
806+
DmlInput::Filters => {
807+
let filters =
808+
extract_dml_filters(input, table_name, &allowed_refs)?;
809+
provider
810+
.table_provider
811+
.delete_from(session_state, filters)
812+
.await
813+
.map_err(|e| {
814+
e.context(format!(
815+
"DELETE operation on table '{table_name}'"
816+
))
817+
})?
818+
}
819+
}
808820
} else {
809821
return exec_err!(
810822
"Table source can't be downcasted to DefaultTableSource"
@@ -816,21 +828,33 @@ impl DefaultPhysicalPlanner {
816828
target,
817829
op: WriteOp::Update,
818830
input,
819-
..
831+
output_schema,
820832
}) => {
821833
if let Some(provider) = target.downcast_ref::<DefaultTableSource>() {
822-
// For UPDATE, the assignments are encoded in the projection of input
823-
// We pass the filters and let the provider handle the projection
824-
let filters = extract_dml_filters(input, table_name)?;
825-
// Extract assignments from the projection in input plan
826-
let assignments = extract_update_assignments(input)?;
827-
provider
828-
.table_provider
829-
.update(session_state, assignments, filters)
830-
.await
831-
.map_err(|e| {
832-
e.context(format!("UPDATE operation on table '{table_name}'"))
833-
})?
834+
let allowed_refs = collect_dml_target_refs(input, table_name)?;
835+
match classify_dml_input(input, table_name, &allowed_refs, "UPDATE")?
836+
{
837+
DmlInput::NoRows => {
838+
zero_rows_affected_exec(Arc::clone(output_schema.inner()))?
839+
}
840+
DmlInput::Filters => {
841+
// For UPDATE, the assignments are encoded in the projection of input
842+
// We pass the filters and let the provider handle the projection
843+
let filters =
844+
extract_dml_filters(input, table_name, &allowed_refs)?;
845+
// Extract assignments from the projection in input plan
846+
let assignments = extract_update_assignments(input)?;
847+
provider
848+
.table_provider
849+
.update(session_state, assignments, filters)
850+
.await
851+
.map_err(|e| {
852+
e.context(format!(
853+
"UPDATE operation on table '{table_name}'"
854+
))
855+
})?
856+
}
857+
}
834858
} else {
835859
return exec_err!(
836860
"Table source can't be downcasted to DefaultTableSource"
@@ -2252,6 +2276,149 @@ fn get_physical_expr_pair(
22522276
Ok((physical_expr, physical_name))
22532277
}
22542278

2279+
/// How a DELETE or an UPDATE reaches its target table.
2280+
///
2281+
/// The `filters` argument of [`TableProvider::delete_from`] and
2282+
/// [`TableProvider::update`] is the only channel that carries the `WHERE` clause
2283+
/// to the provider, and an empty vector means "no `WHERE` clause, so every row".
2284+
/// A plan whose row restriction cannot travel through that channel must
2285+
/// therefore never reach the provider.
2286+
///
2287+
/// [`TableProvider::delete_from`]: datafusion_catalog::TableProvider::delete_from
2288+
/// [`TableProvider::update`]: datafusion_catalog::TableProvider::update
2289+
enum DmlInput {
2290+
/// Every row restriction of the statement reaches the provider as a filter.
2291+
Filters,
2292+
/// No row matches, so the statement affects no rows and the provider is not
2293+
/// called at all.
2294+
NoRows,
2295+
}
2296+
2297+
/// Collect the table references that a predicate of a DELETE or an UPDATE may
2298+
/// name: the target table itself, and the alias of every scan of the target
2299+
/// table in the input plan.
2300+
///
2301+
/// Both [`classify_dml_input`] and [`extract_dml_filters`] need this set, so the
2302+
/// caller collects it once and passes it to each of them.
2303+
fn collect_dml_target_refs(
2304+
input: &Arc<LogicalPlan>,
2305+
target: &TableReference,
2306+
) -> Result<Vec<TableReference>> {
2307+
let mut allowed_refs = vec![target.clone()];
2308+
input.apply(|node| {
2309+
if let LogicalPlan::SubqueryAlias(alias) = node
2310+
// Check if this alias points to the target table
2311+
&& let LogicalPlan::TableScan(scan) = alias.input.as_ref()
2312+
&& scan.table_name.resolved_eq(target)
2313+
{
2314+
allowed_refs.push(TableReference::bare(alias.alias.to_string()));
2315+
}
2316+
Ok(TreeNodeRecursion::Continue)
2317+
})?;
2318+
Ok(allowed_refs)
2319+
}
2320+
2321+
/// Check that the input plan of a DELETE or an UPDATE can reach the table
2322+
/// provider without losing part of its `WHERE` clause.
2323+
///
2324+
/// The optimizer rewrites an `IN` or an `EXISTS` subquery into a semi join, and
2325+
/// it folds an always-false predicate into an empty relation. In both cases the
2326+
/// condition leaves the `Filter` nodes that [`extract_dml_filters`] reads, and
2327+
/// the provider would see an empty filter list and change every row.
2328+
///
2329+
/// # Parameters
2330+
/// - `input`: the input plan of the DELETE or the UPDATE
2331+
/// - `target`: the target table of the statement
2332+
/// - `allowed_refs`: the target table and its aliases, from [`collect_dml_target_refs`]
2333+
/// - `op`: `"DELETE"` or `"UPDATE"`, used in the error message
2334+
///
2335+
/// # Returns
2336+
/// * [`DmlInput::Filters`] when the provider may be called
2337+
/// * [`DmlInput::NoRows`] when the statement matches no row
2338+
/// * a "not implemented" error when part of the `WHERE` clause cannot reach the provider.
2339+
fn classify_dml_input(
2340+
input: &Arc<LogicalPlan>,
2341+
target: &TableReference,
2342+
allowed_refs: &[TableReference],
2343+
op: &str,
2344+
) -> Result<DmlInput> {
2345+
let mut result = DmlInput::Filters;
2346+
input.apply(|node| {
2347+
match node {
2348+
// An empty relation means the optimizer proved that no row matches,
2349+
// so the statement affects no rows.
2350+
LogicalPlan::EmptyRelation(empty) if !empty.produce_one_row => {
2351+
result = DmlInput::NoRows;
2352+
return Ok(TreeNodeRecursion::Stop);
2353+
}
2354+
// A join carries the condition in its `on` clause, where
2355+
// `extract_dml_filters` cannot read it. The optimizer builds one for
2356+
// an `IN` or an `EXISTS` subquery.
2357+
LogicalPlan::Join(join) => {
2358+
return not_impl_err!(
2359+
"{op} on table '{target}' with an IN or an EXISTS subquery in its \
2360+
WHERE clause is not supported: the optimizer rewrites the subquery \
2361+
into a {} join, and the condition does not reach the table provider",
2362+
join.join_type
2363+
);
2364+
}
2365+
LogicalPlan::Filter(filter) => {
2366+
// A predicate on another table restricts the rows of the target
2367+
// table, and the provider cannot evaluate it.
2368+
for predicate in split_conjunction(&filter.predicate) {
2369+
if !predicate_is_on_target_multi(predicate, allowed_refs)? {
2370+
return not_impl_err!(
2371+
"{op} on table '{target}' with a WHERE clause that \
2372+
references another table is not supported"
2373+
);
2374+
}
2375+
}
2376+
}
2377+
// Plans that pass every row of the target table through, or that
2378+
// hold no row restriction of their own.
2379+
LogicalPlan::TableScan(_)
2380+
| LogicalPlan::Projection(_)
2381+
| LogicalPlan::SubqueryAlias(_)
2382+
| LogicalPlan::Sort(_)
2383+
| LogicalPlan::Repartition(_)
2384+
// A `Limit` carries no predicate, so it reaches the provider as no
2385+
// filter at all and `DELETE FROM t LIMIT n` deletes every matching
2386+
// row. `UPDATE ... LIMIT` is already rejected by the SQL planner.
2387+
// That gap is separate from this one, and it is tracked separately.
2388+
| LogicalPlan::Limit(_)
2389+
// A subquery expression that survives to this point fails later,
2390+
// when the provider compiles the filter it belongs to.
2391+
| LogicalPlan::Subquery(_) => {}
2392+
// Everything else either restricts or multiplies the rows of the
2393+
// target table in a way that no filter list can express.
2394+
other => {
2395+
return not_impl_err!(
2396+
"{op} on table '{target}' is not supported: the statement plan \
2397+
contains \"{}\", and its effect on the rows cannot reach the table \
2398+
provider as a filter",
2399+
other.display()
2400+
);
2401+
}
2402+
}
2403+
Ok(TreeNodeRecursion::Continue)
2404+
})?;
2405+
2406+
Ok(result)
2407+
}
2408+
2409+
/// Build a plan that reports no rows affected, for a DELETE or an UPDATE that
2410+
/// matches no row. `schema` is the output schema of the statement, one `count`
2411+
/// column of type `UInt64`.
2412+
fn zero_rows_affected_exec(schema: Arc<Schema>) -> Result<Arc<dyn ExecutionPlan>> {
2413+
let count = Arc::new(UInt64Array::from(vec![0_u64])) as ArrayRef;
2414+
let batch = RecordBatch::try_new(Arc::clone(&schema), vec![count])?;
2415+
Ok(MemorySourceConfig::try_new_exec(
2416+
&[vec![batch]],
2417+
schema,
2418+
None,
2419+
)?)
2420+
}
2421+
22552422
/// Extract filter predicates from a DML input plan (DELETE/UPDATE).
22562423
///
22572424
/// Walks the logical plan tree and collects Filter predicates and any filters
@@ -2268,6 +2435,7 @@ fn get_physical_expr_pair(
22682435
/// # Parameters
22692436
/// - `input`: The logical plan tree to extract filters from (typically a DELETE or UPDATE plan)
22702437
/// - `target`: The target table reference to scope filter extraction (prevents multi-table filter leakage)
2438+
/// - `allowed_refs`: The target table and its aliases, from [`collect_dml_target_refs`]
22712439
///
22722440
/// # Returns
22732441
/// A vector of unqualified filter expressions that can be passed to the TableProvider for execution.
@@ -2277,28 +2445,16 @@ fn get_physical_expr_pair(
22772445
fn extract_dml_filters(
22782446
input: &Arc<LogicalPlan>,
22792447
target: &TableReference,
2448+
allowed_refs: &[TableReference],
22802449
) -> Result<Vec<Expr>> {
22812450
let mut filters = Vec::new();
2282-
let mut allowed_refs = vec![target.clone()];
2283-
2284-
// First pass: collect any alias references to the target table
2285-
input.apply(|node| {
2286-
if let LogicalPlan::SubqueryAlias(alias) = node
2287-
// Check if this alias points to the target table
2288-
&& let LogicalPlan::TableScan(scan) = alias.input.as_ref()
2289-
&& scan.table_name.resolved_eq(target)
2290-
{
2291-
allowed_refs.push(TableReference::bare(alias.alias.to_string()));
2292-
}
2293-
Ok(TreeNodeRecursion::Continue)
2294-
})?;
22952451

22962452
input.apply(|node| {
22972453
match node {
22982454
LogicalPlan::Filter(filter) => {
22992455
// Split AND predicates into individual expressions
23002456
for predicate in split_conjunction(&filter.predicate) {
2301-
if predicate_is_on_target_multi(predicate, &allowed_refs)? {
2457+
if predicate_is_on_target_multi(predicate, allowed_refs)? {
23022458
filters.push(predicate.clone());
23032459
}
23042460
}

0 commit comments

Comments
 (0)