jerry-024 commented on PR #269:
URL: https://github.com/apache/paimon-rust/pull/269#issuecomment-4278722469
## Review: Must-fix blockers before merge
Five items must be fixed before this lands. The overall design (CoW writer +
`validate_deleted_files` + retry loop) is sound and concurrent-DELETE conflict
detection is correct, but the items below are either result-correctness bugs or
hard runtime failures on legal SQL.
### 1. `build_source_partition_set` — partitioned MERGE INTO is incorrect
`crates/integrations/datafusion/src/merge_into.rs:1292-1309`
```rust
let cols = partition_keys
.iter()
.map(|k| format!("{s_alias}.\"{k}\""))
.collect::<Vec<_>>().join(", ");
let sql = format!("SELECT DISTINCT {cols} FROM {source_ref} AS {s_alias}");
```
This assumes the source table has columns named identically to the target's
partition keys. Two failure modes:
- Source lacks those columns → legal MERGE fails at runtime with `column not
found`.
- Source has columns with matching names but different meaning, or the ON
condition maps a differently-named source column (`t.dt = s.event_date`) →
`partition_set` silently omits partitions the MERGE actually touches, rows that
should UPDATE are treated as NOT MATCHED and re-inserted.
**Minimum fix:** fall back to `Ok(None)` (full-partition scan). A proper fix
parses the ON condition to map source→target partition columns.
### 2. Multi-clause `WHEN MATCHED` / `WHEN NOT MATCHED` ordering is wrong
`merge_into.rs:400-413` (matched), `816-826` (inserts)
```rust
if let Some(ref pred) = mc.predicate {
conditions.push(pred.clone());
consumed_predicates.push(pred.clone()); // only pushed when Some
}
```
When a clause has no predicate (unconditional), nothing is added to
`consumed_predicates`, so later clauses re-match the same rows. SQL semantics
require only the first matching clause to fire per row. Combined with the
strict checks in `cow_writer.rs:358-394`, this makes legal SQL like:
```sql
WHEN MATCHED THEN UPDATE SET v = s.v
WHEN MATCHED AND s.flag = 1 THEN UPDATE SET v = s.v2
```
fail with `duplicate UPDATE operations` or `both DELETE and UPDATE
operations` at commit time.
**Minimum fix:** when predicate is `None`, push `\"TRUE\"` into
`consumed_predicates`, or reject any same-kind clause following an
unconditional one (they are unreachable by SQL semantics). Apply the same fix
on the INSERT path.
### 3. Identifier quoting is inconsistent — corrupts quoted column names and
leaves an injection surface
`merge_into.rs`, `update.rs` — every site where a column or alias is
interpolated into SQL
Two codepaths disagree:
- `update_columns`: raw `id.value` (unescaped), wrapped as
`\"\\\"{col}\\\"\"`
- `ins.columns`: `c.to_string()` (already escaped by sqlparser), re-wrapped
as `\"{field}\"` — double-quoted
For a legitimate column name containing `\"`, the same statement emits
inconsistent SQL. For a user with DDL privileges, a column named `x\" FROM t;
DROP TABLE t; --` produces a genuinely injected `SELECT ... AS \"__upd_x\" FROM
t; DROP TABLE t; --\"`.
**Fix:** single helper
```rust
fn quote_identifier(name: &str) -> String {
Ident { value: name.to_string(), quote_style: Some('\"') }.to_string()
}
```
Route *every* identifier interpolation through it. Remove the double-wrap in
`insert_select_clause`.
### 4. `UPDATE t AS x` / `DELETE FROM t AS x` parses but fails at runtime
`update.rs:184-187, 231-237` and `delete.rs:76-80, 109-116`
`update.table.to_string()` keeps the alias in `table_ref`, but the inner
query selects from the generated `__cow_target_N` (not aliased), so `WHERE x.c
= 1` resolves against a name that doesn't exist and the statement fails with
`column x.c not found`. Same pattern in DELETE (`extract_delete_table_ref`
drops the alias but the user's WHERE still references it).
This is not a \"not yet supported\" message — it is a confusing runtime
error on legal SQL.
**Minimum acceptable fix:** reject aliased UPDATE/DELETE at the entry points
with an explicit `DataFusionError::Plan(\"table alias in UPDATE/DELETE is not
yet supported\")`. A full fix registers the temp table under the user's alias
or rewrites the WHERE.
### 5. `MemTable` leaks in `SessionContext` on async cancellation
`delete.rs:91-98`, `update.rs:193-200`, `merge_into.rs` register/deregister
pattern
```rust
let (has_data, cow_table_name) = register_cow_target_table(ctx, table,
&writer).await?;
...
let _ = ctx.deregister_table(&cow_table_name);
```
Sequential deregister, not RAII. If the future is cancelled (timeout, panic
mid-SQL, ctx drop) between register and deregister, the `MemTable` — which
holds all target rows — stays in the `SessionContext`. `COW_TABLE_COUNTER` is
monotonically increasing so names never collide; the leak is silent and
unbounded across DMLs on a long-lived context.
**Fix:** wrap `cow_table_name` in a Drop guard that calls
`ctx.deregister_table` synchronously in `Drop`.
---
### Non-blocking but worth a follow-up
- `merge_into.rs` empty `partition_set` (`WHERE pt='nonexistent'`) is
treated as \"no filter\" and materializes the full table into a `MemTable` —
OOM risk on large tables. Short-circuit to 0 affected rows.
- `is_delete_conflict` substring-matches `\"Delete conflict\"`; prefer a
typed `paimon::Error::Conflict` variant.
- `rewrite_condition` does text-level replace on SQL and can corrupt ON
conditions containing string literals that match the table name; use an AST
`VisitMut`.
- No concurrency cap on parallel file rewrites in `cow_writer.rs` — FD
exhaustion on very large DELETEs; use `stream::iter(...).buffered(N)`.
- Orphan parquet files from failed commits: acceptable by design since
Paimon relies on a separate orphan-file cleanup action, same as Java. No fix
needed here.
### Test coverage gaps worth adding
- NULL values in partition keys
- Column names containing `\"` (covers #3)
- Aliased UPDATE/DELETE (covers #4)
- Multiple WHEN MATCHED / WHEN NOT MATCHED with mixed
predicate/unconditional clauses (covers #2)
- Partitioned MERGE where source uses different column names for partition
keys (covers #1)
- Retry exhaustion path (5 retries depleting)
---
Design is good. Fix #1, #2, #3, #4, #5 and this is ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
