JingsongLi commented on code in PR #10702:
URL: https://github.com/apache/paimon/pull/10702#discussion_r4345317213
##########
paimon-core/src/main/java/org/apache/paimon/mergetree/compact/LookupChangelogMergeFunctionWrapper.java:
##########
@@ -152,18 +183,27 @@ private void setChangelog(@Nullable KeyValue before,
KeyValue after) {
}
} else {
if (!after.isAdd()) {
- reusedResult.addChangelog(replaceBefore(RowKind.DELETE,
before));
+ reusedResult.addChangelog(
+ replaceBeforeWithSequenceOverwrite(RowKind.DELETE,
before, after));
} else if (valueEqualiser == null
|| !valueEqualiser.equals(before.value(), after.value())) {
reusedResult
- .addChangelog(replaceBefore(RowKind.UPDATE_BEFORE,
before))
+ .addChangelog(
+ replaceBeforeWithSequenceOverwrite(
+ RowKind.UPDATE_BEFORE, before, after))
.addChangelog(replaceAfter(RowKind.UPDATE_AFTER,
after));
}
}
}
- private KeyValue replaceBefore(RowKind valueKind, KeyValue from) {
- return replace(reusedBefore, valueKind, from);
+ private KeyValue replaceBeforeWithSequenceOverwrite(
+ RowKind valueKind, KeyValue before, KeyValue after) {
+ if (reusedOverwriteRow != null) {
+ reusedOverwriteRow.replace(before.value(), after.value());
Review Comment:
[P2] Preserve metadata from the incoming event, not the merged after image
`after` is the result of `mergeFunction.getResult()`, so its fields need not
contain the incoming event values. With the supported `partial-update` merge
engine, stored `(data=10, event_ts=50)` plus incoming `(data=20,
event_ts=NULL)` produces a merged row with `event_ts=50`; this code
consequently also emits `-U(data=10, event_ts=50)` instead of the promised
incoming NULL. The table state should retain 50 under partial-update semantics,
but the preserved event metadata must be captured separately. Please retain the
relevant raw event values, or reject merge engines for which this contract
cannot be provided, and cover this case.
##########
paimon-core/src/main/java/org/apache/paimon/mergetree/compact/MergeTreeCompactManagerFactory.java:
##########
@@ -364,11 +364,36 @@ private MergeTreeCompactRewriter createRewriter(
} else {
processorFactory =
PersistValueProcessor.factory(valueType);
}
+ List<String> preserveColumns =
options.changelogPreserveFieldOnRetract();
+ int[] preserveFieldIndices = null;
+ if (!preserveColumns.isEmpty()) {
+ List<String> fieldNames = valueType.getFieldNames();
+ preserveFieldIndices =
+ preserveColumns.stream()
+ .mapToInt(
+ name -> {
+ int idx =
fieldNames.indexOf(name);
Review Comment:
[P2] Keep preservation references valid across column renames
For the documented configuration where the same column is used by
`sequence.field` and `changelog-producer.preserve-field-on-retract`, renaming
that column succeeds but only `sequence.field` is rewritten by
`SchemaManagerUtils.applyRenameColumnsToOptions`. The preservation option
retains the old name. Reopening the table and writing the next row reaches this
lookup and throws `IllegalArgumentException` (column not found), stopping
writes after a successful ALTER. Include this new option in the existing
schema-rename rewrite/validation and test an actual write after the rename.
--
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]
