SEZ9 commented on PR #12608:
URL: https://github.com/apache/seatunnel/pull/12608#issuecomment-6137824720

   Thanks @CodeWithPravinMaske. Nothing new on CI beyond my previous note on 
run `38211935642`, and my conclusion is unchanged: no code-side blocker, and a 
committer with write access is still needed for the approval.
   
   While the `dev` merge is pending, here is a recap of the open points from my 
review of `2ca0489fa9`, so they can be addressed in the same push or as 
follow-ups, whichever you prefer:
   
   - **MEDIUM (robustness/functional):** the nullability restore only runs in 
`MySqlSchema#parseSnapshotDdl`. DDL parsed via `parseStreamingDdl` and the 
checkpointed table history bypass it, so a unique-key change applied through 
the binlog, or a restore from checkpoint, would reintroduce the NULL -> 0 
behaviour. Could you route those paths through the same restore step, or 
explain why they are not affected?
   - **LOW (docs/compatibility):** a short note in the MySQL-CDC docs and the 
incompatible-changes / release notes that NULL now flows through for promoted 
unique-key columns, and that sinks previously provisioned to receive `0` may 
need adjusting.
   - **LOW (robustness):** a nullable column listed in 
`table-names-config.primaryKeys` that is also the table's first unique key is 
still emitted as 0 for NULL, silently. A warning log or a docs line would be 
enough.
   - **LOW (robustness/docs):** the `catalogTable == null` guard in 
`restoreNullableColumns` is unreachable from `parseSnapshotDdl` because 
`mergeCatalogTableConfig` already dereferences the same value one line earlier; 
either drop the guard and the "may be null" Javadoc, or move the guard ahead of 
the merge call.
   - **LOW (style):** consider lowering the per-column `log.info` to `debug`, 
or emitting one aggregated `info` per table, since every parallel reader builds 
its own `MySqlSchema`.
   
   If you'd rather handle the LOW items in a follow-up, just say so. I'll take 
another look once `dev` is merged in.
   
   <!-- streview-comment:1719 -->


-- 
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]

Reply via email to