You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "reswqa (via GitHub)" <gi...@apache.org> on 2026/10/16 09:18:02 UTC
[PR] fix(core): preserve partial-update deletion state [paimon-rust]
reswqa opened a new pull request, #1139:
URL: https://github.com/apache/paimon-rust/pull/1139
### Purpose
Partial-update merges can incorrectly revive a deleted record when processing an update to another sequence group, an older update to the delete-controlling group, or subsequent retractions.
For example, after a `DELETE` from a group configured in `partial-update.remove-record-on-sequence-group`, an `INSERT` affecting only another group must not make the record visible again.
Deleted results were also materialized from the last raw retract instead of the accumulated merge state. This can lose field updates and aggregate results across merge boundaries, or retain incorrect row-kind metadata.
This PR aligns these behaviors with Java's partial-update merge semantics.
### Brief change log
- Preserve deletion state across subsequent retractions and unrelated or stale sequence-group updates.
- Clear sequence-group deletion state only when a configured delete-controlling group accepts an INSERT or UPDATE_AFTER.
- Materialize deleted records from merged fields and aggregate results, preserving the final RowKind and latest internal sequence number.
- Correct the existing `last_non_null_value` regression: an older update may contribute to the aggregate state without reviving the deleted record.
### Tests
- Add regression coverage for deletion and restoration, single-field and composite sequence groups, all commit-boundary combinations, projections, aggregate-state retention, and `ignore-delete`.
- Add a unit test verifying the DELETE payload, final RowKind, and latest internal sequence number after a subsequent UPDATE_BEFORE.
- `cargo test -p paimon --locked --offline`
- Independently run equivalent cases against Java master (`5d019f5a6d`) using a temporary parity suite
### API and Format
No public API or storage format changes.
### Documentation
No documentation changes. This fixes existing partial-update behavior.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(core): preserve partial-update deletion state [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #1139:
URL: https://github.com/apache/paimon-rust/pull/1139
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(core): preserve partial-update deletion state [paimon-rust]
Posted by "JeonDaehong (via GitHub)" <gi...@apache.org>.
JeonDaehong commented on code in PR #1139:
URL: https://github.com/apache/paimon-rust/pull/1139#discussion_r4301108503
##########
crates/paimon/src/table/sort_merge.rs:
##########
@@ -715,37 +683,31 @@ impl PartialUpdateMergeFunction {
continue;
}
+ if group
+ .sequence_indices
+ .iter()
+ .any(|index| self.sequence_group_partial_delete.contains(index))
+ {
+ current_delete_row = false;
+ }
group_sequence_rows[group_idx] = Some((row.batch_idx, row.row_idx));
for &output_col_idx in &group.sequence_indices {
selected_by_col[output_col_idx] = Some((row.batch_idx, row.row_idx));
}
}
}
- if current_delete_row {
- return match last_retract_source {
- Some((batch_idx, row_idx)) => Ok(MergeResult::MaterializedDeleteRow(
- Self::materialize_source_row(
- batch_idx,
- row_idx,
- batch_buffer,
- source_output_col_indices,
- output_schema,
- )?,
- )),
- None => Ok(MergeResult::Omit),
- };
- }
- if !saw_add && last_retract_source.is_none() {
+ if !saw_add && !saw_retract {
return Ok(MergeResult::Omit);
}
+ let deleted = current_delete_row || !saw_add;
Review Comment:
Nothing covers the `|| !saw_add` part. If I drop it everything still passes, but two UPDATE_BEFORE rows on `seq_right` in the same commit then show up as a visible row `[1, None, None, 4, None, None]`. Java returns DELETE there since `meetInsert` is false. Something like this would catch it:
```rust
let a = event(RowKind::UpdateBefore, (None, None), (Some(3), Some(30)), None);
let b = event(RowKind::UpdateBefore, (None, None), (Some(4), Some(40)), None);
write(&table, &[a, b]).await;
assert_projections(&table, None, 0).await;
```
##########
crates/paimon/src/table/sort_merge.rs:
##########
@@ -761,7 +723,7 @@ impl PartialUpdateMergeFunction {
None => new_null_array(field.data_type(), 1),
},
};
- if saw_add && !field.is_nullable() && column.is_null(0) {
+ if (saw_add || current_delete_row) && !field.is_nullable() && column.is_null(0) {
Review Comment:
nit: same here, reverting to `saw_add` alone doesn't fail anything. A NOT NULL value column with a null DELETE payload would cover it (Java throws in `initRow`).
##########
crates/paimon/src/table/sort_merge.rs:
##########
@@ -546,7 +513,7 @@ impl PartialUpdateMergeFunction {
continue;
}
latest_row = Some(row);
- if !saw_add && last_retract_source.is_none() {
+ if !saw_add && !saw_retract {
Review Comment:
Unrelated to this PR, but since this comment mentions it: Java calls `initRow` on the first retract before it checks `ignoreDelete`, and here we `continue` at L512 first. With `ignore-delete=true` and `[DELETE(value="old"), INSERT(value=null)]` Java gives `"old"`, Rust gives `null`. Only matters for files that already have retract rows in them (0.7 and older, per the Java comment), so probably a follow-up.
##########
crates/paimon/src/table/sort_merge.rs:
##########
@@ -715,37 +683,31 @@ impl PartialUpdateMergeFunction {
continue;
}
+ if group
+ .sequence_indices
+ .iter()
+ .any(|index| self.sequence_group_partial_delete.contains(index))
Review Comment:
This matches `updateWithSequenceGroup` in Java. Reverting it (or the guard at L618) breaks 5 / 3 of the new tests, good.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org