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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
