nzw921rx commented on PR #11025: URL: https://github.com/apache/seatunnel/pull/11025#issuecomment-5305836888
> @CloverDew Thank you for your contribution. I have found some issues that need to be fixed: > > ### Why the green E2E result does not validate this feature > Both MySQL CDC E2E jobs passed for the final PR commit: > > * [[JDK 8](https://github.com/CloverDew/seatunnel/actions/runs/31555429214/job/94316168568)](https://github.com/CloverDew/seatunnel/actions/runs/31555429214/job/94316168568) > * [[JDK 11](https://github.com/CloverDew/seatunnel/actions/runs/31555429214/job/94316168831)](https://github.com/CloverDew/seatunnel/actions/runs/31555429214/job/94316168831) > > However, the current E2E assertions do not validate the CDC schema-change event contract. They execute the DDL against the source MySQL database, query the source database’s own `information_schema` to verify its comment, and then verify that ordinary data records continue reaching the sink. > > Therefore, the tests can remain green even when a comment event is: > > * emitted with the wrong table identifier; > * ignored or dropped before reaching the sink; > * represented as the wrong schema-change event type; > * applied as a structural column modification instead of a comment-only no-op. > > The job logs confirm that this is happening: > > ``` > AlterColumnCommentEvent: 0 occurrences > > AlterTableModifyColumnEvent > columnLength=2048 > sourceType=VARCHAR(512) > typeChanged=true > > AlterTableCommentEvent > tableIdentifier=.null.products > changeAfter=null > > Not found table products > ``` > > The green E2E result is consequently a false-positive for the feature introduced by this PR. > > ### 1. The new table-comment path propagates a pre-existing parser-context problem into the emitted event > The following sequence already existed before this PR: > > ```java > ddlParser.setCurrentDatabase(tablePath.getDatabaseName()); > ddlParser.setCurrentSchema(tablePath.getSchemaName()); > ``` > > Debezium 1.9.8 stores both values in the same `currentSchema` field. For MySQL, `tablePath.getSchemaName()` is `null`, so the second call clears the database context. > > This underlying parser-context problem predates the PR. However, before this change, column events were returned inside an `AlterTableColumnsEvent` whose top-level `TableIdentifier` was reconstructed from the `SourceRecord` table path. The new table-comment path instead returns the parser-created event directly: > > ```java > if (parsedEvents.size() == 1 > && parsedEvents.get(0) instanceof AlterTableCommentEvent) { > return parsedEvents.get(0); > } > ``` > > Compare the [[base implementation](https://github.com/apache/seatunnel/blob/0a5ceee2018e44659593fc16ad57f154a4483210/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/schema/AbstractSchemaChangeResolver.java#L77-L103)]( > > https://github.com/apache/seatunnel/blob/0a5ceee2018e44659593fc16ad57f154a4483210/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/schema/AbstractSchemaChangeResolver.java#L77-L103 > > ) with the [[new direct-return path](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/schema/AbstractSchemaChangeResolver.java#L81-L108)]( > https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/schema/AbstractSchemaChangeResolver.java#L81-L108 > > ). > This makes `.null.products` the top-level event identifier. As a result: > > * the event cannot be matched to `.shop.products`; > * the source-side `CatalogTable` is not updated; > * `changeAfter` remains `null`; > * a later comment change still obtains the wrong `oldComment`; > * multi-table Zeta processing may drop the event as belonging to an unknown table. > > The parsed event must be normalized to the `SourceRecord`/catalog `TablePath` before it is returned. > > ### 2. The new column-comment classification does not work with the existing MySQL type normalization > The test DDL only adds a comment to an existing `VARCHAR(512)` column: > > ```sql > ALTER TABLE products > MODIFY COLUMN description VARCHAR(512) > COMMENT 'Product description text'; > ``` > > Nevertheless, both real E2E jobs emit an `AlterTableModifyColumnEvent`; no `AlterColumnCommentEvent` is produced. > > The MySQL conversion had already expanded character lengths through `charTo4ByteLength` before this PR. The new [`[isSameColumnExceptComment](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/schema/AbstractSchemaChangeResolver.java#L254-L289)`](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/base/schema/AbstractSchemaChangeResolver.java#L254-L289) compares the raw column representations, including `columnLength`, so semantically identical `VARCHAR(512)` definitions fail the comparison. > > This is not a regression of a previously working comment-event feature—the feature did not previously exist. It means the main column-comment capability claimed by this PR is not actually working in the real CDC path. > > The current behavior also allows sinks to receive a structural `ModifyColumnEvent`. JDBC executes an actual `ALTER TABLE ... MODIFY COLUMN`, while sinks such as Elasticsearch may reject the event entirely. > > The comparison must use canonicalized column metadata, and the E2E test must assert that a pure comment change produces `AlterColumnCommentEvent`. > > ### 3. Producing the intended column-comment event exposes missing Zeta and Databend integration > The current E2E does not reach this path because issue 2 prevents `AlterColumnCommentEvent` from being produced. > > After issue 2 is corrected, the event will be nested inside `AlterTableColumnsEvent`. Zeta’s [`[SeaTunnelSourceCollector](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/SeaTunnelSourceCollector.java#L270-L291)`](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/SeaTunnelSourceCollector.java#L270-L291) still uses the deprecated [`[DataTypeChangeEventDispatcher](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-api/src/main/java/org/apache/seatunnel/api/table/schema/handler/DataTypeChangeEventDispatcher.java#L59-L79)`](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-api/src/main/java/org/apache/seatunnel/api/table/schema/handler/DataTypeChang eEventDispatcher.java#L59-L79). > > The outer `AlterTableColumnsEvent` is dispatched to [`[AlterTableEventHandler](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-api/src/main/java/org/apache/seatunnel/api/table/schema/handler/AlterTableEventHandler.java#L60-L86)`](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-api/src/main/java/org/apache/seatunnel/api/table/schema/handler/AlterTableEventHandler.java#L60-L86), but that handler has no branch for the nested `AlterColumnCommentEvent` and will throw `UnsupportedOperationException`. > > These dispatcher classes predate the PR, but the new event type did not. Adding the event requires integrating it with the engine path that consumes schema-change events. > > Databend has a separate problem introduced in this PR. [`[SchemaChangeManager](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-connectors-v2/connector-databend/src/main/java/org/apache/seatunnel/connectors/seatunnel/databend/schema/SchemaChangeManager.java#L63-L140)`](https://github.com/apache/seatunnel/blob/12eba6fb116824f85cc7eb63c7b7b77c48bee612/seatunnel-connectors-v2/connector-databend/src/main/java/org/apache/seatunnel/connectors/seatunnel/databend/schema/SchemaChangeManager.java#L63-L140) checks `AlterTableColumnEvent` before checking `AlterColumnCommentEvent`. Since `AlterColumnCommentEvent` extends `AlterTableColumnEvent`, it enters the generic branch first, and the nested handler eventually throws instead of ignoring the comment event. > > Therefore, fixing only the producer-side classification will turn the current false-green E2E into deterministic Zeta and Databend failures. > > ### 4. This PR directly regresses MySQL DDL parser error handling > Before this PR, the custom parser called: > > ```java > super(); > ``` > > The Debezium 1.9.8 default is equivalent to: > > ```java > super(true, false, false, ...); > ``` > > where `throwErrorsFromTreeWalk=true`. > > This PR changes it to: > > ```java > super(false, false, true, ...); > ``` > > The change enables comment parsing but also disables throwing errors collected during the parse-tree walk. SeaTunnel does not retrieve or propagate `getParsingExceptionsFromWalker()`, so partially parsed DDL may now silently produce incomplete schema-change events. > > This is a direct behavioral regression introduced by the PR. The intended constructor call should preserve the previous error behavior: > > ```java > super(true, false, true, ...); > ``` > > ### Required follow-up > These findings should not be addressed as isolated small fixes because they mask one another. In particular, correcting the column classification without updating the consumers will expose the deterministic Zeta and Databend failures. > > A complete corrective change should verify: > > 1. Table-comment events use `.shop.products`, not `.null.products`. > 2. Consecutive table-comment changes contain the correct `oldComment` and non-null `changeAfter`. > 3. A pure column-comment change produces `AlterColumnCommentEvent`. > 4. A genuine structural type change still produces `AlterTableModifyColumnEvent`. > 5. Comment events pass through Zeta and the affected sink dispatch paths without failure. > 6. Parser tree-walk errors continue to be propagated. > 7. E2E assertions inspect the emitted event type, table path, and downstream result rather than only the source database state and ordinary row continuity. @CloverDew Hello, could you submit a PR to fix this issue? Thanks 🙏 No need to revert this PR; I have already corrected the 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]
