DanielLeens commented on PR #11659: URL: https://github.com/apache/seatunnel/pull/11659#issuecomment-5205087504
Reviewed at head `2794225` (branch `improve-cli-routing-cdc-knowledge`). ## Relationship to #11658 This PR and #11658 branch from the same base commit and independently carry the identical `_validate_routing_pairs`/`_extract_connector_blocks_raw` fix (byte-identical diff in `agents.py` and the same 5 regression tests) — not accidental duplication, the description explains it's because this PR's new skill content depends on that fix and #11658 wasn't merged yet. Both show `mergeable: MERGEABLE` against `dev` today; since the shared hunks are textually identical, whichever merges second will just show an empty diff for that portion once rebased — no conflict either order. Would suggest merging #11658 first for a cleaner history, but not required. ## Routing-validation fix (shared with #11658) — looks correct Traced through all 5 new tests by hand against the updated code: - Adding `"transform"` to the section tuple in [`_extract_connector_blocks_raw`](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/agents.py#L291) correctly closes the gap; the existing brace-counting walk already handles nested blocks like `schema { fields {...} } }` fine, so no new parsing edge case here. - Generalizing `source.`/`sink.` to `f"{section}.{connector_name}"` in [`_validate_routing_pairs`](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/agents.py#L343-L394) is a nice side-effect fix — also corrects diagnostic misattribution for transform-originated errors, and there's a dedicated test for it. - Making the `_validate_routing_pairs` call unconditional ([agents.py#L502-L505](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/agents.py#L502-L505)) correctly closes the "silently skipped without pyhocon" hole from #11657. Not a regression, just flagging for awareness: `_extract_connector_blocks_raw`'s section-finding regex only finds the *first* occurrence of each section header per config, and the label regex only matches quoted `plugin_output`/`plugin_input` values (unquoted HOCON labels silently skip routing validation). Both pre-existing, unchanged by this PR. ## New domain-knowledge content — two factual issues found Since the whole point of this PR is teaching the model *correct* facts to fix generation errors, I checked the new claims against the actual connector source rather than trusting the prose. Most of it holds up, but two things don't: **1. The PostgreSQL-CDC `table-names` format guidance is wrong.** [cdc_realtime.md#L67-L69](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/skills/cdc_realtime.md#L67-L69) and the comparison table row at [L82](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/skills/cdc_realtime.md#L82) assert entries must be 3-part `database.schema.table` and explicitly say "not `schema.table`". The actual parser, [`PostgresSourceConfigFactory.java#L86-L103`](https://github.com/apache/seatunnel/blob/bf9be04805115fc856344741414b097a21497a88/seatunnel-connectors-v2/connector-cdc/connector-cdc-postgres/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/postgres/config/PostgresSourceConfigFactory.java#L86-L103), accepts *both* 2-part (`schema.table`, used as-is) and 3-part (`database.schema.table` — the leading database segment is silently **dropped**, only `schema.tabl e` is forwarded to Debezium); the method's own exception message calls the canonical form `schemaName.tableName` (2-part). So the skill steers the model away from the connector's own canonical format, and implies the database segment does real filtering when it's actually discarded — real database scoping happens through the separate `database-names` option (and even that only uses [the first element](https://github.com/apache/seatunnel/blob/bf9be04805115fc856344741414b097a21497a88/seatunnel-connectors-v2/connector-cdc/connector-cdc-postgres/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/postgres/config/PostgresSourceConfigFactory.java#L69) for the JDBC connection, despite being list-typed). Suggest correcting both spots to say both forms are accepted, 2-part is canonical, and the database segment (if given) is ignored. **2. The generic CDC template fix is incomplete.** The second commit's own message says it exists to fix "the generic CDC Pattern template... still presented singular `database-name`/`table-name`... contradicting the corrected PostgreSQL-specific guidance." But it only adds a caveat comment above the template ([cdc_realtime.md#L119-L124](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/skills/cdc_realtime.md#L119-L124)) — the template body itself still reads `database-name = "<database>"` / `table-name = "<table>"`. Checked: **neither singular key exists for MySQL-CDC either.** [`MySqlIncrementalSourceFactory.optionRule()`](https://github.com/apache/seatunnel/blob/bf9be04805115fc856344741414b097a21497a88/seatunnel-connectors-v2/connector-cdc/connector-cdc-mysql/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/mysql/source/MySqlIncrementalSourceFactory.java#L62-L64) registers only `TABLE_NAMES`/`DATABASE_NAME S` (plural — inherited from the same shared [`CatalogOptions.TABLE_NAMES`](https://github.com/apache/seatunnel/blob/bf9be04805115fc856344741414b097a21497a88/seatunnel-api/src/main/java/org/apache/seatunnel/api/options/table/CatalogOptions.java#L40-L45) interface Postgres-CDC uses), no singular fallback anywhere in `connector-cdc-base` or `connector-cdc-mysql`. So the "generic" template a model copies for *any* CDC connector uses keys that don't exist for either connector this skill documents. Since this PR already touches these exact lines, seems like the natural place to finish the fix — swap the template to the plural form instead of just disclaiming it above. Smaller, same area: `database-names` is list-typed but only the first element is actually used for the connection (see link above) — the skill's phrasing "lists the databases to monitor" ([L71](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/skills/cdc_realtime.md#L71)) slightly overstates that. ## FieldMapper example looks internally inconsistent [`multi_pipeline.md#L111-L113`](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/skills/multi_pipeline.md#L111-L113): ``` field_mapper = { order_id = id amount = total } ``` [`FieldMapperTransform.java#L104-L113`](https://github.com/apache/seatunnel/blob/bf9be04805115fc856344741414b097a21497a88/seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/fieldmapper/FieldMapperTransform.java#L104-L113): the map key must be an *existing input field name* (looked up via `inputFieldNames.indexOf(key)`, and the constructor throws if a key isn't found in the input schema), and the value becomes the *new output column name*. The `amount = total` entry follows that direction (existing terse column → friendly output name), but `order_id = id` reads the opposite way — if the source table's real PK column is `id` (the natural read given the sibling entry), this entry would throw at runtime for referencing a nonexistent input field `order_id`. Given the PR's own finding that weaker models imitate example patterns literally, worth fixing the direction (`id = order_id`) so the example stays internally consistent. ## Possible skill-router interaction (not confirmed, worth checking) [`skills.py#L483`](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/skills.py#L483) caps injected skills at `_MAX_SKILLS = 2`, and `multi_pipeline` gets a forced score floor of 3 whenever the plan has 2+ pipeline slots ([skills.py#L500-L503](https://github.com/apache/seatunnel/blob/279422525829722a6310bcafa506073bda14ad08/seatunnel-cli/seatunnel_cli/skills.py#L500-L503)). `conditional_routing` shares the `"split"` trigger with the existing `transform_chain` skill, and its other triggers are fairly generic ("based on", "separate", "by status"). If a 1-source/2-sink split request ends up represented as 2 pipeline slots by the planner, `multi_pipeline`'s forced floor plus a low organic score for `conditional_routing` could push the new skill out of the top-2 cut — the exact scenario it's meant to fix. This might explain the one regression in the PR's own benchmark table (`t3_mp_mixed_transform_pipelines` 3/3→2/3), curre ntly attributed to single-task variance, which is plausible with only 3 trials but worth checking directly — logging `SkillRouter.match()`'s actual selection for that trial would confirm or rule it out. ## Test coverage note The 5 new regression tests for the routing fix are well-targeted (chain, parallel-split, dangling-input-still-rejected, transform-misattribution, duplicate-across-transform-location). But none of `benchmark/tasks/tier{1,2,3}*.json` exercise `Postgres-CDC` as an actual source — the PostgreSQL-CDC prerequisite content, including the `table-names` issue above, was never exercised end-to-end by the project's own benchmark harness. An `l3: run` Postgres-CDC benchmark task would likely have caught this. ## What checked out fine Worth noting since I went looking for problems: the core mechanism `conditional_routing.md` relies on — one `plugin_output` fanning out to multiple `plugin_input` consumers — is a real, tested Zeta engine topology (`LogicalDagGenerator` builds one edge per consumer from a set of target vertices, and it's already exercised by existing E2E configs). The "Zeta SQL supports WHERE only, no GROUP BY/JOIN/ORDER BY" claim is accurate (`ZetaSQLEngine` explicitly throws for all three). `slot.name`, `decoding.plugin.name`, the plural-only `database-names`, and the `debezium { }` passthrough map all check out against source too. Also: the PR description's PostgreSQL-CDC summary ("schema-qualified `table-name` (`schema.table`, not MySQL-style `db.table`)") doesn't match what's actually shipped (plural `table-names`, and — per the finding above — `schema.table` is in fact accepted). Looks like the description predates the self-correcting second commit; worth syncing before merge since it'll likely become the squash-merge commit message. --- Net: the routing-validation fix and the wide-DAG/conditional-routing wiring content look solid and are backed by real, tested engine behavior. The PostgreSQL-CDC `table-names` guidance should be corrected before merge though, since it actively points away from the connector's own canonical format — which cuts against this PR's own stated goal of fixing exactly this kind of generation error. The FieldMapper example and the incomplete template fix are worth a pass too. -- 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]
