DanielLeens commented on PR #10959: URL: https://github.com/apache/seatunnel/pull/10959#issuecomment-5207533011
Hi @SEZ9, thanks for taking a fresh look at this one. On the "possible public API change" you flagged for `DamengCreateTableSqlBuilder.java`: I traced it against the current head `deafab3c0e04` and the caller chain, and I'd like to add some context that changes the risk picture. - The signature change you spotted is real: `public String build(TablePath tablePath)` becomes `public List<String> build(TablePath tablePath)` in this PR. - However, `DamengCreateTableSqlBuilder` is an internal implementation class under `.../jdbc/catalog/dm`, not an SPI or documented extension point. It has exactly one caller in the whole codebase — `DamengCatalog.getCreateTableSqls(...)` — which lives in the same module and is updated together with the builder in this very diff. There's no downstream connector or external consumer depending on this method's return type. - More importantly, the identical `String -> List<String>` change already landed on `dev` via #10934 (merged 2026-05-27), which fixed the same Dameng "create table with column comments" DDL failure this PR targets. `dev`'s current `DamengCreateTableSqlBuilder.build()` already returns `List<String>` and has been running there without any reported compatibility fallout. Given that, I'd treat this as a non-blocking observation rather than a High-severity backward-compatibility risk — the same "API change" already happened safely on `dev` through the accepted fix. The bigger point for this thread though: this PR is now superseded. As I noted on 2026-07-18, #10934 already merged an equivalent (and more complete, including comment-escaping) fix into `dev`. Keeping both implementations around would just leave duplicate/conflicting logic for the same code path, so I'd suggest closing #10959 in favor of #10934 rather than continuing to iterate on the return-type question here. @happybrant no action needed on your side — this isn't a rejection of your work, the Dameng DDL issue you found is real and it's already fixed on `dev` thanks to your report. Really appreciate you flagging it. -- 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]
