Gabriel39 commented on PR #66498:
URL: https://github.com/apache/doris/pull/66498#issuecomment-5199557884
Thanks for adding Paimon row-level DML support. I reviewed the current head
`ac1681ac` and found the following correctness issues and test gaps that should
be addressed before merge.
## Correctness issues
### [P1] DELETE can silently succeed without deleting rows when
`ignore-delete=true`
`PaimonDmlCommandUtils.checkDelete()` accepts every `DEDUPLICATE` table
unconditionally, without checking `CoreOptions.IGNORE_DELETE`.
In the Paimon 1.3.1 dependency used by this branch,
`DeduplicateMergeFunction` deliberately discards retract records when
`ignore-delete=true`. This PR emits `RowKind.DELETE`, so both a standalone
DELETE and a MERGE DELETE branch can commit successfully while leaving the
existing row unchanged.
Please reject DELETE/MERGE DELETE during analysis when delete records are
configured to be ignored, or otherwise provide semantics that guarantee the row
is removed.
### [P1] UPDATE cannot set a nullable column to NULL on
`merge-engine=partial-update`
`checkUpdate()` explicitly permits `PARTIAL_UPDATE`, and the new write path
sends a complete `UPDATE_AFTER` row containing the explicit NULL value.
However, Paimon's partial-update merge function only updates non-null
fields. As a result:
```sql
UPDATE t SET nullable_col = NULL WHERE id = 1;
```
can report success while retaining the old value. The same issue applies to
a MERGE UPDATE assignment.
Please either reject SQL UPDATE/MERGE UPDATE for partial-update tables until
explicit NULL can be represented correctly, or add a connector-specific
encoding/write path that distinguishes "field not supplied" from "set field to
NULL".
### [P2] Schema-change retry reuses the old immutable write target
The UPDATE/DELETE/MERGE commands create and pin a `PaimonWriteTarget` before
constructing `InsertIntoTableCommand`. `InsertIntoTableCommand` retries by
replanning the same already-bound `LogicalPaimonTableSink`; it does not reload
the Paimon table or rebuild the write target.
The new `CollectRelation` handling can help detect a target schema change,
but it does not refresh the pinned columns/table serialized to the writer. A
concurrent remote schema change can therefore cause repeated retry failures or
a mismatch between the analyzed output schema and the writer table generation.
Please rebuild the row-level DML plan/write target on retry, and add a
concurrent schema-change test.
### [P2] MERGE does not detect multiple source rows matching one target row
The implementation directly joins source and target and emits one changelog
record per joined row. There is no cardinality check for multiple source rows
matching the same target primary key. For deduplicate tables this can produce
last-write-wins behavior whose result depends on execution/write order, instead
of rejecting an ambiguous MERGE.
Please define and enforce the expected cardinality semantics and add a
duplicate-source-key test.
## Existing regression/coverage failures
- `external_table_p0/paimon/test_paimon_write_boundary` still asserts that
UPDATE, DELETE, and MERGE are unsupported. It now fails in External Regression
and must be updated to validate the new behavior.
- BE incremental coverage is currently `0/2` for the changed C++ lines.
- FE incremental coverage is only `9/378 (2.38%)`.
- The new regression verifies only a default deduplicate table with fixed
buckets plus one UPDATE, one DELETE, and one mixed MERGE.
## Missing test scenarios
Please add coverage for at least:
1. `ignore-delete=true`: standalone DELETE and MERGE DELETE must not
silently succeed.
2. `merge-engine=partial-update`:
- update a non-null value;
- explicitly set a nullable column to NULL;
- MERGE UPDATE with an explicit NULL;
- delete behavior with `partial-update.remove-record-on-delete`;
- sequence-group removal options.
3. `merge-engine=aggregate` with and without
`aggregation.remove-record-on-delete`.
4. MERGE with duplicate source keys matching one target key.
5. All unsupported-engine negative paths for UPDATE, DELETE, and MERGE, not
only UPDATE on `first-row`.
6. Schema changes between initial analysis, retry, writer binding, and
commit.
7. Multiple conditional MATCHED/NOT MATCHED clauses and branch-priority
behavior.
8. Dynamic bucket modes, sequence fields, partitioned tables, and
reordered/mixed-case columns.
9. UPDATE/DELETE with CTEs, joins/USING, aliases, ORDER BY/LIMIT where
supported, and zero matched rows.
10. Failure atomicity: writer/commit failure after mixed
INSERT/UPDATE/DELETE records must not partially apply the MERGE.
11. Large MERGE/UPDATE/DELETE workloads covering join spill, Paimon writer
spill/backpressure, memory limits, cancellation, and OOM protection.
12. JNI/Thrift compatibility for the changed Java `open` signature and the
new `CHANGELOG` write mode, including a clear mixed-version/rolling-upgrade
behavior.
Given the two silent data-correctness failures above and the directly
related red regression, I do not think this is ready to merge yet.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
