leonardBang commented on PR #4540:
URL: https://github.com/apache/flink-cdc/pull/4540#issuecomment-5810055010
## Review Summary
Second-round review on `4ae9e39c5`. All three findings from the previous
round are addressed, and the fixes hold up against the code and the new tests:
- `verifyExpansion` now renders all three kinds of unresolved difference
through the shared `describeDifferences(plan)` helper, so a no-op DDL no longer
reports an empty `[]`.
- `analyze()` now calls `validateTableKeys()` and records key differences as
incompatibilities, so `CHECK`/`EXPAND` fail instead of silently passing a table
that identifies rows differently.
- Missing columns now go through `normalizeType(...)` as a phase-1 gate, so
an unrepresentable type is reported rather than surfacing as a DDL failure in
phase 2.
The behaviour tightening you flagged is fine, and keeping `TRY_EXPAND`
non-fatal on key mismatch is consistent with its best-effort contract.
### Remaining issue
**`validateTableKeys()` does not account for sinks that store partition
columns inside the primary key.** `PaimonMetadataApplier#applyCreateTable`
merges every partition key into `primaryKeys` before `catalog.createTable(...)`
- `PaimonMetadataApplierTest` asserts that a pipeline schema with
`primaryKey("col1")` + `partitionKey("dt")` produces a table whose
`primaryKeys()` is `[col1, dt]` - and `getExistingTableSchema` reads that
augmented list back. The strict set comparison therefore reports a
pipeline-created partitioned table as key-incompatible on the next run.
`CHECK`/`EXPAND` fail a valid job, and `TRY_EXPAND` takes the new skip path and
issues no DDL at all, so the option quietly does nothing for partitioned
tables. Filed inline with a suggested normalisation.
### Checklist
- [ ] Normalise partition columns out of the primary key comparison
- [ ] Add a partitioned-target unit case, and ideally a partitioned table in
the Paimon e2e
## Leonard Verdict
**Ready to merge:** With fixes
**Reason:** The three previous gaps are properly closed with matching tests.
The remaining item is a false-positive risk in the new key check on the most
common Paimon layout (partitioned primary-key tables), which can either fail
valid jobs or silently disable expansion; it should be normalised before merge.
--
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]
