DanielLeens commented on PR #11479: URL: https://github.com/apache/seatunnel/pull/11479#issuecomment-6414866158
Thanks @SEZ9 for the careful re-read of `31ce4465a3`. This is the same boundary I raised as Issue 8 in my review on this head, so +1: we are aligned and it is the only remaining non-CI blocker I see. One refinement on the shape of the guard, since both of us have now described it slightly differently. I would suggest **failing fast with context** rather than "skip or log": - In `SeaTunnelRowStreamingRecordDeserializer.decodeDeleteValuesFromHandle` the key-only branch allocates an all-null image and then `CommonHandleDecoder.restorePrimaryKeyColumns` fills every null PK slot straight from the record key. - For a string PK under a new (non-binary) collation, the key holds collation weights, not the original string, so `column.getType().decode(input)` yields bytes that are not the stored value. - Skipping the column would still emit a DELETE with a null PK slot, and only logging would still emit a wrong identity. Both are silent, which is what we want to avoid. Throwing an `IllegalStateException` with table, column and collation (plus key/startTs/commitTs, as the existing pk-handle branch already does) makes the unsupported case visible. Suggested scope for @zhangshenghang: keep key-only recovery for reversible components (integers, decimals, binary-safe types), and fail when a missing PK component is a string whose collation needs restored data. Please add an image-less DELETE regression with a real new-collation string key, and a one-line note about this limit in the EN/ZH docs. The existing test that preserves an original string already present in the row image should stay as is. The head is unchanged since my last review (`31ce4465a3`), and the fork Build for it is green, so I will do a fresh pass as soon as a new commit lands. -- 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]
