DanielLeens commented on PR #10934:
URL: https://github.com/apache/seatunnel/pull/10934#issuecomment-4525100049
Thanks for working on this. I reviewed the latest head
`95f960d1a3a5c008c031fe7d19033f3192c3d689` locally against `upstream/dev`, and
I traced the actual Dameng create-table path through the catalog layer. I did
not run Maven locally in this pass; this is a source-level review only.
# What problem this PR solves
- User pain point
Dameng create-table can fail when column comments are present, because the
builder currently returns `CREATE TABLE ...` plus `COMMENT ON COLUMN ...` as
one combined SQL string, while the base JDBC catalog path executes it as a
single statement.
- Fix approach
This PR overrides `getCreateTableSqls()` in `DamengCatalog` and tries to
split the combined SQL into multiple statements before `createTableInternal()`
executes them one by one.
- One sentence
The direction is correct, but the current `split(";")` implementation is
too coarse and breaks valid column comments that themselves contain semicolons.
# Runtime chain I checked
```text
Catalog create-table path
-> AbstractJdbcCatalog.createTableInternal()
[AbstractJdbcCatalog.java:485-493]
-> DamengCatalog.getCreateTableSqls() [DamengCatalog.java:116-124]
-> getCreateTableSql()
-> DamengCreateTableSqlBuilder.build()
[DamengCreateTableSqlBuilder.java:91-106]
-> emits CREATE TABLE ...;
-> appends COMMENT ON COLUMN ... IS '<column comment>';
-> executeInternal(dbUrl, sql) for each returned statement
```
# Core logic review
Before this PR, Dameng inherited the default behavior:
```java
protected List<String> getCreateTableSqls(
TablePath tablePath, CatalogTable table, boolean createIndex) {
return Collections.singletonList(getCreateTableSql(tablePath, table,
createIndex));
}
```
Now the latest head splits the builder output by `;`:
```java
@Override
protected List<String> getCreateTableSqls(
TablePath tablePath, CatalogTable table, boolean createIndex) {
String sql = getCreateTableSql(tablePath, table, createIndex);
return Arrays.stream(sql.split(";"))
.map(String::trim)
.filter(s -> !s.isEmpty())
.map(s -> s + ";")
.collect(Collectors.toList());
}
```
Key findings:
1. The normal create-table path definitely hits this change.
2. The PR is fixing the right layer: Dameng does need multi-statement
execution here.
3. But the latest head assumes every `;` in the builder output is a
statement separator.
4. That assumption is not true once a real column comment contains a
semicolon.
# Findings
Issue 1: `split(";")` also splits valid semicolons inside column comments,
so the fix is still incorrect on a real comment boundary
- Location:
`seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/dm/DamengCatalog.java:116`
- Problem description:
`AbstractJdbcCatalog.createTableInternal()` fully trusts the statement
list returned by `getCreateTableSqls()`. But
`DamengCreateTableSqlBuilder.build()` appends real column comment text directly
into `COMMENT ON COLUMN ... IS '...';`. That means the latest `split(";")`
logic cannot distinguish the end of a SQL statement from a semicolon that is
part of the comment literal itself.
- Potential risk:
If a user has a column comment like `a;b`, the current implementation will
split one valid `COMMENT ON COLUMN` statement into two broken fragments, and
the Dameng auto-create-table path will still fail on the exact path this PR is
trying to repair.
- Best improvement suggestion:
Option A: follow the Oracle / OceanBase-Oracle pattern and let the Dameng
builder return `List<String>` directly, so the catalog never reparses the SQL
text.
Option B: if you want a minimal patch, split according to the known
builder structure instead of applying a blind `;` split to the whole string.
- Severity: High
- Already raised by others: No
Issue 2: there is still no regression test for the new multi-statement
Dameng contract
- Location:
`seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/catalog/dm/DamengCreateTableSqlBuilderTest.java:99`
- Problem description:
The existing tests only check the single builder string. They do not
verify the new `getCreateTableSqls()` behavior, and they do not lock down how
the create-table SQL should be split when comments are present.
- Potential risk:
A later builder-format change can silently break execution again without
CI catching it.
- Best improvement suggestion:
Please add at least one regression test that asserts the exact statement
list returned for Dameng create-table, including a comment-bearing case.
- Severity: Medium
- Already raised by others: No
# Compatibility
- API/config/defaults: no change
- Protocol/serialization: no change
- Historical behavior: still compatible from the outside, but not safe for
comment text that contains semicolons
# Tests / coverage
- No new UT/E2E test code was added in this PR.
- So the main gap here is missing regression coverage, not flaky-test
structure.
# CI status
- GitHub currently reports:
- `mergeable=MERGEABLE`
- `mergeStateStatus=BLOCKED`
- `Build=IN_PROGRESS`
- So CI is not green yet, but issue 1 is already a source-level blocker
independently of the running Build.
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1: the current SQL splitting logic is unsafe for valid semicolons
inside column comments, so the Dameng create-table fix is not robust yet.
2. Suggested but non-blocking follow-up
- Issue 2: add a regression test for the new multi-statement create-table
contract.
Overall, I agree with the direction of the fix. But for the current head, I
do not think we should merge yet, because the splitting layer is still too
fragile for real-world comment content.
--
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]
