DanielLeens commented on PR #11577: URL: https://github.com/apache/seatunnel/pull/11577#issuecomment-5421792917
@SEZ9 Thanks for the fresh pass. I want to flag a mismatch before we go further, because two of the four items you're listing as blocking (Issue 2, and Issues 1/3 together) are the exact two things I re-verified line-by-line against this same head (`4dc7808cbb12`) in my Aug 24 review, and I don't think the current source supports re-opening them. **Issue 2 (identifier escaping/DDL injection)** — `BigQuerySchemaChangeManager.toAddColumnAction()` calls `quoteIdentifier(column.getName())` at `BigQuerySchemaChangeManager.java:284`, which wraps the name in backticks via `validateIdentifier()` at `BigQuerySchemaChangeManager.java:452-463`. `validateIdentifier` throws `BigQueryConnectorException` if the identifier contains a backtick, `\n`, or `\r`, so it can't break out of the quoted identifier or smuggle a second statement. The table id is validated/quoted the same way in the constructor (`:85-89`). This guard is present in the diff at the current head, not missing — `git log -S validateIdentifier` shows it's been there since the PR's first commit. **Issues 1 & 3 (uncoordinated DDL across parallel subtasks hitting BigQuery's per-table quota)** — `applySchemaChange()` at `BigQuerySchemaChangeManager.java:99-144` already guards this: before issuing DDL it checks `hasMissingColumns()` (`:115`), so a subtask that loses the race to a peer's already-applied DDL returns without calling `bigQuery.query()` again; and on `BigQueryException`/`JobException` from the DDL call, `isRetryableDdlFailure()` (`:178`) recognizes HTTP 429 and rate-limit reasons, and `waitBeforeRetry()` (`:134`) backs off and retries instead of failing the job outright. `BigQuerySchemaChangeManagerTest` exercises exactly this with `testConcurrentHandlersRecoverFromTableUpdateQuota` and `testRetryDdlAfterQuotaFailureWhileColumnIsStillMissing` (both present at the current head). I re-pulled and re-read `BigQuerySchemaChangeManager.java` directly from the current head just now before writing this, rather than relying on my Aug 24 notes, and the lines above are exactly where they were. Could you take a direct look at `BigQuerySchemaChangeManager.java:99-192, 281-287, 448-463` (rather than the doc wording in `docs/en/connectors/sink/BigQuery.md`, which is what this round's citations point at) and let me know if I'm missing a code path where an identifier or a DDL call bypasses these guards? If not, I'd like to close Issues 1-3 as already-resolved and keep the four non-blocking items (Issue 4's doc note, the `emulator_grpc_host` loopback restriction, the CDC E2E coverage gap, and a dedicated hostile-identifier unit test) as the remaining punch list — all four of which I agree are still worth doing. -- 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]
