Skip to content

[Fix][Connector-V2] Fix dameng create table failed bug #10959

Open
happybrant wants to merge 1 commit into
apache:devfrom
happybrant:fix/dmCreateTable
Open

[Fix][Connector-V2] Fix dameng create table failed bug #10959
happybrant wants to merge 1 commit into
apache:devfrom
happybrant:fix/dmCreateTable

Conversation

@happybrant
Copy link
Copy Markdown

fix #10955

Copy link
Copy Markdown
Contributor

@DanielLeens DanielLeens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. I reviewed the full diff on the current head and traced the real Dameng catalog create-table path end to end.

What this PR fixes

  • User pain: Dameng table creation can fail when column comments are emitted as part of one large DDL string.
  • Fix approach: split the CREATE TABLE statement and each COMMENT ON COLUMN into separate SQL statements.
  • One-line summary: the direction is correct, but the current revision still leaves one real runtime gap in the comment SQL path and one deterministic test mismatch.

Runtime chain I checked

catalog create-table path
  -> AbstractJdbcCatalog.createTable(...) [AbstractJdbcCatalog.java:443-472]
      -> AbstractJdbcCatalog.createTableInternal(...) [AbstractJdbcCatalog.java:485-497]
          -> DamengCatalog.getCreateTableSqls(...) [DamengCatalog.java:115-117]
              -> DamengCreateTableSqlBuilder.build(...) [DamengCreateTableSqlBuilder.java:56-109]
                  -> SQL[0] = CREATE TABLE ...
                  -> SQL[1..n] = COMMENT ON COLUMN ...
          -> executeInternal(dbUrl, sql)  // executes every statement one by one

comment SQL generation
  -> DamengCreateTableSqlBuilder.buildColumnCommentSql(...) [DamengCreateTableSqlBuilder.java:157-168]
      -> current head appends column.getComment() directly into IS '...'
      -> comments containing a single quote still produce invalid SQL

Findings

  • The normal catalog create-table path really does hit the new split-SQL contract.
  • Splitting the statements is the right fix for the original Dameng DDL execution issue.
  • However, the current head still leaves the comment-content escaping problem unresolved.
  • The updated unit test is not flaky from a timing/resource perspective, but one new assertion is currently inconsistent with the builder output on the createIndex=false path.

Merge conclusion

Conclusion: can merge after fixes

  1. Blocking items
  • Issue 1: DamengCreateTableSqlBuilder.java:157-168 still appends column.getComment() directly into IS '...' without escaping '. So if a column comment is something like Bob's column, the new COMMENT ON COLUMN statement still breaks on the real create-table path. The same JDBC catalog family already escapes this pattern in builders such as OracleCreateTableSqlBuilder.java:144-154. Please escape single quotes here as well and add a regression test for that case.
  • Issue 2: DamengCreateTableSqlBuilderTest.java:133-147 now expects the createIndex=false create-table SQL to end with );, but DamengCreateTableSqlBuilder.build(...) currently returns the first statement with ) only. That is a deterministic mismatch rather than a flaky test pattern, so this assertion needs to be corrected before merge.
  1. Suggested non-blocking follow-up
  • No extra non-blocking requests from my side beyond covering the escaped-comment case in the unit test.

Overall, the split-SQL approach is the right foundation. Once the comment escaping and the test mismatch are closed, this should be in much better shape for merge.

@davidzollo davidzollo added the First-time contributor First-time contributor label May 26, 2026
@davidzollo
Copy link
Copy Markdown
Contributor

image

@DanielLeens
Copy link
Copy Markdown
Contributor

Thanks @davidzollo. I saw the screenshot. On the current unchanged head deafab3c0e04cb3e2817d215ddd6ad6e98930af3, the two blockers from the latest Daniel review are still the ones I would keep: the column-comment SQL still needs single-quote escaping in DamengCreateTableSqlBuilder.buildColumnCommentSql(...), and the test expectation still needs to match the actual createIndex=false output. Once a new commit lands for those two points, I will re-review the new head from scratch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

First-time contributor First-time contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [connector-jdbc] Dameng create table with comment failed

3 participants