Repository navigation
fix(frontend): accept parenthesized ALTER ADD COLUMN lists - #29963
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
0f1318e to
4a3a767
Compare
|
mo-bot review · Commit: Assessment: approve Reviewed the complete six-file delta. Parenthesized ADD COLUMN lists preserve ordering and reuse existing authorization, validation, COPY execution and cleanup. No actionable defects remain within the stated syntax-compatibility scope. Model: Session: Validation:
Limitations:
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 4a3a767ec62ea936811cd65f52d29cb6618066c2
Assessment: approve
Reviewed the complete six-file delta. Parenthesized ADD COLUMN lists preserve ordering and reuse existing authorization, validation, COPY execution and cleanup. No actionable defects remain within the stated syntax-compatibility scope.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
Validation:
-
Regenerated mysql_sql.go using the repository-owned generator: byte-for-byte match, zero shift/reduce or reduce/reduce conflicts.
-
Traced legacy and grouped ADD syntax through AST ownership, duplicate/type/default validation, planning errors, transaction handling and COPY execution.
-
Reviewed parser, planner and SQL regression coverage for the reported Debezium statement, mixed options, malformed lists, duplicate rejection, schema preservation, backfill and subsequent writes.
-
Reused frozen exact-head successful Ubuntu UT, UT coverage, arm64 SCA, shared-build, proxy BVT and pessimistic multi-CN BVT results, plus the author's exact-head focused parser/planner test report.
-
git diff --check passed; tracked source remains unchanged. Detailed evidence is recorded in attempt-1/review-audit.json.
Limitations:
-
Local validation exercised parser generation only; native/server tests were not rerun. Frozen CI evidence contains terminal statuses rather than individual test logs.
-
The linked issue's full text was not supplied; its contract was assessed from the PR description and reproduced SQL. Full Debezium/Kafka recovery remains outside this assessment.
-
Parser overhead was reviewed structurally but not benchmarked; no claim of zero performance regression is made.
31be889 to
f859569
Compare
f859569 to
e193516
Compare
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact published head 4918392 against merge base e511193. Read issue #29851, all prior reviews/conversation (no inline threads), the increment since previously reviewed 7736872, and the entire four-file change map. No blocking finding.
Grouped ADD definitions are flattened in source order into the same AlterAddCol/ColumnPositionNone owner as scalar syntax. Mixed scalar/grouped ALTER actions remain composable through the existing alter_option_list recursion, and column_def retains nested DECIMAL/default-expression commas. Identifier, duplicate/type/default checks, per-table column limits, authorization, COPY planning and transactional cleanup remain on the existing path; there is no alternate execution or lifecycle owner. Parenthesized indexes/constraints are deliberately not accepted. The allocator adapter restores the existing test helper's required bounded-range arguments and propagates range-discovery errors; it changes no production logservice behavior. This is a syntax correction using the existing AST contract, not a new design-gated subsystem.
Independent validation on the reviewed head:
- Repository-owned parser generator run twice; mysql_sql.go stayed byte-identical, SHA256 28606b80bd1429a10ef6add533da323e5469e460cd7be70dffc7c20c952e850a, with no reported grammar conflicts. Generated state/table reindexing is mechanical.
- Built this detached worktree's native dependencies, then ran the supported CGo test wrapper. Full MySQL parser package passed (199 root tests, including a temporary extra probe). The probe covers ADD with/without COLUMN, mixed scalar/grouped actions, several groups, nested type/default commas and separate index/algorithm/lock controls; empty/trailing/double-comma/missing-type lists and grouped constraints are rejected.
- Existing TestAlterTableAddColumns planner control passed.
- Temporary public MySQL-protocol regression passed two rounds on the same real CN (0.14s/0.11s scenario bodies): the exact reported LONGTEXT NULL statement, a two-column DECIMAL/default list, preserved rows and backfill, Unicode subsequent UPDATE/INSERT, duplicate-column rejection with no partial schema/data change, and verified empty fixture catalog after each round. This is direct SQL validation, not a full Debezium/Kafka recovery certification.
- Reused exact-head CI 38348271128 terminal success for build/UT/SCA/coverage and both BVT modes; inspected all eight successful planner race checkpoints. No new shared state was introduced requiring an invented concurrency stress root.
- git diff --check passed. The initial direct parser-test attempt failed for missing native malloc prerequisites; it was replaced by the successful native build and wrapper runs, not counted as a code failure.
The published head is presently DIRTY against newer main. This code approval is scoped to 491; it does not approve the unpublished 331 synchronization candidate or waive merge conflict resolution/fresh validation after rebase. The current-main #30108/#30109 delivery issue is outside this exact published diff. APPROVE; state/head rechecked immediately before submission.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Merge Queue Status
This pull request spent 2 hours 37 minutes 58 seconds in the queue, including 1 hour 31 minutes 52 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
Closes #29851
What this PR does / why we need it:
Accept
ALTER TABLE ... ADD [COLUMN] (column_definition, ...)and lower each definition to the existing orderedAlterAddColpath. This supports the Debezium JDBC statement reported in #29851 while preserving the existing scalar ALTER productions. Parenthesized indexes and constraints are outside this change.This PR integrates official main commit
dd5bdb7a896398a161a93734a8073fe1ccc031b3to resolve the parser conflict while retaining its parser changes. Relative to that base, the PR changes only the MySQL grammar, generated parser, and focused parser tests. The generated parser was produced by the repository generator; its tables were not hand-edited.Diff against the integrated main tree: grammar +23/-1, generated parser +7,978/-7,786, parser tests +32. The generated diff preserves all 1,974 existing action bodies/stack offsets and adds four actions; generation reported zero shift/reduce or reduce/reduce conflicts.
Validation
git diff --checkpassed.TestAlterTableAddColumns, and the treeTestIndexOptionOpTypeLiteralpassed via the repository CGo test wrapper.LONGTEXT NULLparenthesized ADD and a two-columnVARCHAR/DECIMAL DEFAULTADD. The existing row scanned back with both new nullable columns NULL and the decimal default1.25; cleanup passed. The service used a private port 6002 endpoint.-s -w(omit symbol table and DWARF), and the resulting binary passed the SQL probe. The unmodified Make/strip target itself is not claimed as completed.5903b6d0739ad90db358d1c5cad527e4c5e8c23dcompleted successfully in MatrixOne ALL CI: preflight, shared build, SCA arm64, Ubuntu UT, PR size, coverage eligibility, coverage, proxy BVT, pessimistic BVT, coverage aggregate, and CI Required all passed. Non-applicable jobs were skipped; Iceberg Connector E2E was skipped and is not claimed as passed.Invalid commit message/section not foundbecause the extracted summary heading lacked the exact trailing colon required by the repository template and Mergify configuration. Both template headings now match exactly; the rule status should be refreshed from this body update.Checks not run
The full Debezium connector/Kafka recovery flow and complete distributed BVT matrix were not run locally.