DanielLeens commented on PR #10934:
URL: https://github.com/apache/seatunnel/pull/10934#issuecomment-4528183028
Thanks for the update. I re-reviewed the latest head from scratch and
rechecked the Dameng create-table path again.
# What This PR Fixes
- User pain: Dameng auto-create-table fails when column comments are
present, because the base JDBC catalog path needs separate statements for
`CREATE TABLE` and each `COMMENT ON COLUMN`.
- Fix approach: the PR now follows the Oracle-style pattern and lets
`DamengCreateTableSqlBuilder.build(...)` return `List<String>` directly, so the
catalog executes the table DDL and comment statements separately without
reparsing SQL text.
- One-line summary: this resolves the semicolon-in-comment bug at the right
layer, and the latest head is much safer than the previous split-by-semicolon
version.
Simple example:
- Before this update, a comment like `a;b` could be broken by the
catalog-side `split(";")` workaround.
- After this update, the builder emits one `CREATE TABLE` statement plus one
full `COMMENT ON COLUMN` statement per commented column, so the semicolon stays
inside the comment literal instead of being treated as a separator.
# Runtime Chain Rechecked
```text
Catalog create-table path
-> AbstractJdbcCatalog.createTableInternal(...)
[seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/AbstractJdbcCatalog.java:485-493]
-> DamengCatalog.getCreateTableSqls(...)
[seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/dm/DamengCatalog.java:115-118]
-> new DamengCreateTableSqlBuilder(...).build(tablePath)
-> sqls[0] = CREATE TABLE ...
-> sqls[1..n] = COMMENT ON COLUMN ...
-> executeInternal(dbUrl, sql) for each returned statement
```
# Key Findings
- The normal create-table path definitely hits this change.
- Moving the multi-statement split into the builder is the correct fix. The
catalog no longer has to guess where statement boundaries are.
- The new tests in `DamengCreateTableSqlBuilderTest.java` now cover the
statement-list contract, including the semicolon-in-comment case that
previously broke the workaround.
- I do not see a remaining source-level blocker in the current head.
# Findings
No blocking issue found in the current head.
# Merge Decision
### Conclusion: can merge
1. Blocking items
- None.
2. Suggested follow-up
- The current `Build` check shows `CANCELLED`, so please rerun it on the
latest head and make sure the fresh run is green before merging.
Overall, this is a solid follow-up to the previous review. Thanks for taking
the builder-return-list direction; that is the right shape for this path.
--
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]
