DanielLeens commented on PR #11479:
URL: https://github.com/apache/seatunnel/pull/11479#issuecomment-5385690465
Thanks @SEZ9 for the deep follow-up round — I want to flag one factual
correction before anything else, since Issue 1 is the item marked blocking.
**Issue 1** describes `row.getOldValue()` being decoded into a separate
`values` array for an UPDATE_BEFORE row, and that array skipping the new
`restorePrimaryKeyColumns` call. I re-fetched
`SeaTunnelRowStreamingRecordDeserializer.java` directly from GitHub at the
current head (`30c35a30a438`) to check this against the real code, and the PUT
branch does not do what Issue 1 describes:
```java
case PUT:
values = decodeObjects(
row.getValue().toByteArray(),
RowKey.decode(row.getKey().toByteArray()).getHandle(),
tableInfo);
CommonHandleDecoder.restorePrimaryKeyColumns(
row.getKey().toByteArray(), values, tableInfo);
if (row.getOldValue() == null || row.getOldValue().isEmpty()) {
// ... RowKind.INSERT, emits `values`
} else {
// ... RowKind.UPDATE_AFTER, emits the SAME already-restored `values`
}
```
There is no second `decodeObjects(row.getOldValue()...)` call anywhere in
this method, and no `UPDATE_BEFORE` row is emitted at all — only `DELETE`,
`INSERT`, and `UPDATE_AFTER`, and all three go through paths that already call
`restorePrimaryKeyColumns` before conversion. I also checked the PR's own diff
(`gh api repos/apache/seatunnel/pulls/11479/files`) to confirm this control
structure — the single `values` array reused for the INSERT-vs-UPDATE_AFTER
branch — is pre-existing and untouched by this PR outside of the two inserted
`restorePrimaryKeyColumns` calls. So the specific "UPDATE_BEFORE still emits
null PK" regression in Issue 1 doesn't apply to the code that's actually on
this head; the after-image used for both INSERT and UPDATE_AFTER is restored.
I haven't independently re-verified Issues 2-8 yet (short common-handle key
ordering, the all-or-nothing overwrite risk under new collations, the missing
key-length/type guard before `skipBytes(11)`, the missing Javadoc, the missing
streaming-UPDATE test, the `getIndices()` null risk, and the per-record
allocation overhead) — several of those look like they could have real merit
and are worth a careful pass on their own. Wanted to get the Issue 1 correction
out first since it's the one currently framed as the merge blocker; happy to
work through the rest with you once the head settles.
--
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]
