{{ message }}
Repository navigation
refactor(advisor/tidb): migrate naming index/UK/FK convention to omni AST (Phase 1.5 §1.5.3 batch 4) - #20204
Merged
Conversation
… AST (Phase 1.5 §1.5.3 batch 4) Migrate 3 advisors to omni AST via Recipe A (top-level type-switch): - advisor_naming_index_convention.go - advisor_naming_unique_key_convention.go - advisor_naming_foreign_key_convention.go Pre-batch coupling scan promoted 2 cross-file helpers to utils.go: - getTemplateRegexp (function) - indexMetaData (struct) Both are AST-agnostic Go (no pingcap types in their signatures), so the move is clean. No Linear cleanup ticket needed (unlike canNull/BYT-9362 or tableNewColumn/BYT-9395, which stayed pingcap-typed for un-migrated consumers). Also added OmniStmt.TrimmedText() and OmniStmt.FirstTokenLine() helpers to utils.go. Same helpers also defined on PR #20199 (batch 3); whichever batch merges second drops the duplicate definition (trivial conflict). NEW shape divergence caught and pinned: omni splits ALTER TABLE ADD INDEX (ATAddIndex) vs ADD CONSTRAINT name INDEX (ATAddConstraint) where pingcap collapsed both into AlterTableAddConstraint. Same split applies to UNIQUE: ADD UNIQUE INDEX goes to ATAddIndex, ADD CONSTRAINT name UNIQUE goes to ATAddConstraint. Switch arms now cover both, mirroring mysql omni rule. Fixture pins for the discovered divergence: - naming_index_idx.yaml: ALTER TABLE ADD CONSTRAINT name INDEX (pass + violation pair). - naming_index_uk.yaml: ALTER TABLE ADD UNIQUE INDEX, ALTER TABLE ADD UNIQUE KEY (4 cases: 2 forms x pass+violation each). These also pin the existing ConstrUnique unification (cumulative table item #2). Behavior preserved (NOT aligned with mysql divergence): CREATE FULLTEXT/SPATIAL INDEX statements continue to be checked by the index naming convention advisor (matching pingcap behavior). The mysql omni rule excludes them; aligning tidb with mysql on this is a separate intentional behavior change deferred from this AST migration. Migration progress: 17 of 51 advisors on omni AST. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…up naming trio
Sonar flagged 21.9% duplication across the 3 new naming-convention
advisors (33.6% / 32.2% / 11.0% per file). The duplication was real:
~140 LOC of structural overlap across the trio in the Check() preamble
(payload validation, template-token check, maxLength default) and the
advice-emission loop (regex match + length overflow advices).
Extracted runNamingConventionRule(checkCtx, cfg, collect) in utils.go.
Each advisor's Check() drops to ~17 lines: a config struct (mismatch
code, type noun, internal-error title) plus a closure that maps statement
nodes to []*indexMetaData. Collector functions (collect{Index,UK,FK}*)
unchanged — those carry the rule-specific logic that genuinely differs.
Net LOC: +93 helper / -167 dedup = -102 LOC. Sonar duplication on the
trio should drop to ~0%.
Mysql analogs left similar duplication inline; that precedent pre-dates
Sonar duplication gating on new code. Since the trio is new code, the
gate applies; the cumulative cost — naming_table, naming_column,
naming_auto_increment_column are queued in the handoff doc and would
each replicate the same ~50 LOC scaffold without the helper — justifies
the extraction now.
Verification:
- TestTiDBRules green — advice content is byte-identical to the
pre-extraction inline form (helper builds the same "<noun> in table
..." and "<noun> `<name>` in table ..." strings via fmt.Sprintf with
cfg.typeNoun substitution). Existing fixtures are the safety net.
- go test ./backend/plugin/advisor/tidb/ ./backend/plugin/advisor/mysql/
green (cross-engine non-regression).
- golangci-lint 0 issues.
- Server build clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce0904a4cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
rebelice
approved these changes
May 7, 2026
Codex P2 round-1 catch on PR #20204. The line argument ostmt.AbsoluteLine(constraint.Loc.Start) was evaluated BEFORE buildFKMetaData could nil-check, so a nil entry in n.Constraints would panic with a nil pointer dereference instead of being skipped. The helper's internal `if constraint == nil` was vestigial because of Go's argument-evaluation order. Sibling collectIndexCreateTable / collectUKCreateTable already nil-checked at loop top; collectFKCreateTable was the outlier. collectFKAlterTable was safe by accident — it precomputes stmtLine outside the loop. Fix: nil-check at loop top, mirroring siblings exactly. Added TestCollectFKCreateTableNilConstraintGuard with a synthetic AST containing a nil constraint slot. Omni's parser doesn't produce nils from valid SQL so no YAML fixture can reach this branch — the unit test pins the contract for future refactors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r-batch-4 # Conflicts: # backend/plugin/advisor/tidb/utils.go
Code review pushback on PR #20204 — the runNamingConventionRule extraction's byte-identity claim only stood for the regex-mismatch and length-overflow paths the YAML fixtures exercise. The internal-error advice path (fires when getTemplateRegexp's regexp.Compile fails on the post-substitution template) had no test coverage; an accidental swap of cfg.internalErrorTitle vs cfg.typeNoun, or a regression in Status/Code fields, would go unnoticed. Added TestRunNamingConventionRuleEmitsInternalErrorAdvice — uses an unbalanced-paren format that passes payload-token validation but fails regexp.Compile after no-op substitution. Asserts the advice's Title comes from cfg.internalErrorTitle (not cfg.typeNoun or rule.Type), Code is code.Internal, Status mirrors the rule level, and Content shape matches the pre-extraction inline form. Also added clarifying comments per the same review: - runNamingConventionRule docstring: indexMetaData coupling tradeoff (4-field LCD shared by all callers; future divergence may require widening or replacing the closure shape). - omniIndexColumns docstring: silent skip of expression-based parts (functional indexes contribute no name to column-list substitution; matches pingcap behavior). - Index/UK Check methods: explain the closure-capture pattern for originalMetadata (keeps helper signature uniform across rules that don't need metadata). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1.5 §1.5.3 batch 4 of the TiDB advisor migration to omni AST.
Migrates 3 naming-convention advisors via Recipe A (top-level type-switch).
Migration progress: 17 of 51 advisors on omni AST.
Migrated advisors
advisor_naming_index_convention.gocase ATAddConstraint, ATAddIndexto cover both ALTER syntactic formsadvisor_naming_unique_key_convention.goConstrUniquearm covers pingcap's 3-way Uniq/UniqKey/UniqIndex splitadvisor_naming_foreign_key_convention.goconstraint.RefTable/constraint.RefColumns(omni flat fields)Pre-batch checklist
Coupling scan
Cross-file helpers in the trio:
getTemplateRegexpadvisor_naming_index_convention.go:244utils.goindexMetaData(struct)advisor_naming_index_convention.go:132utils.goBoth helpers are AST-agnostic Go (no pingcap types). Clean move — no Linear cleanup ticket needed (unlike
canNull/BYT-9362 ortableNewColumn/BYT-9395, which stay pingcap-typed for un-migrated consumers).Shape audit findings (NEW divergence — proposed cumulative table item #8)
naming_*_convention(batch 4)ALTER TABLE ADD INDEXandALTER TABLE ADD CONSTRAINT name INDEXboth produceAlterTableAddConstraint(single enum)ATAddIndex(no CONSTRAINT keyword) vsATAddConstraint(with CONSTRAINT keyword)case ATAddConstraintonly would miss bareADD INDEX. Mirrored mysql omnicase ATAddConstraint, ATAddIndexto cover both. Same applies to UNIQUE forms.Existing cumulative table items consulted (from plan §1.5.0 invariant #7):
ConstrUniqueunification): omni unifies pingcap'sConstraintUniq/ConstraintUniqKey/ConstraintUniqIndexunder singleConstrUnique. UK advisor's switch becomes single arm; covers all 3 syntactic forms.Spot-check on constraint-name handling per session-baseline review note:
constraint.Nameis the same field name in both pingcap and omni (verified atomni/tidb/ast/parsenodes.go:459). No divergence.Fixture pins
New fixtures pin the discovered divergences:
naming_index_idx.yaml:ALTER TABLE ... ADD CONSTRAINT name INDEX(pass + violation pair) — pinsATAddConstraint+ConstrIndexpath.naming_index_uk.yaml:ALTER TABLE ... ADD UNIQUE INDEXandADD UNIQUE KEY(4 cases: 2 forms × pass+violation each) — pinsATAddIndex+ConstrUniquepath AND theConstrUniqueunification on the ALTER side.If omni regresses on either split, these fixtures fail loudly.
Behavior preserved (NOT aligned with mysql)
CREATE FULLTEXT INDEX/CREATE SPATIAL INDEXcontinue to be checked by the index naming convention advisor (matching pingcap behavior ofKeyType != IndexKeyTypeUnique). The mysql omni rule additionally excludes Fulltext/Spatial; aligning tidb with mysql on this is a separate intentional behavior change deferred from this AST migration.Dedup follow-up (commit
ce0904a4)Sonar's first-run review flagged 21.9% duplication across the 3 new files (33.6% / 32.2% / 11.0%). The duplication was real: ~140 LOC of structural overlap in the
Check()preamble (payload validation, template-token check, maxLength default) and the advice-emission loop (regex match + length overflow advices).Extracted
runNamingConventionRule(checkCtx, cfg, collect)inutils.go. EachCheck()drops to ~17 lines: anamingRuleConfigstruct (mismatch code, type noun, internal-error title) plus a closure mapping statement nodes to[]*indexMetaData. Collector functions are unchanged.Net LOC: +93 helper / -167 dedup = -102 LOC. Sonar dup on the trio should drop to ~0%.
Mysql divergence — explicit: the mysql omni naming-convention rules (
rule_naming_index_convention.goetc.) left similar duplication inline. That precedent pre-dates Sonar duplication gating on new code. Since the trio is new code, the gate applies; we extract here. The cumulative cost of NOT extracting —naming_auto_increment_columnis queued in the handoff doc and would replicate the same ~50 LOC scaffold; any future naming-style rules also benefit — justifies the deviation from mysql precedent. Reviewers who prefer mysql-mirrored structure can push back; the extraction is reversible.Verification:
TestTiDBRulesgreen — advice content is byte-identical to the pre-extraction inline form (helper builds the same"<noun> in table ..."and"<noun> \` in table ..."strings viafmt.Sprintfwithcfg.typeNoun` substitution). Existing fixtures are the safety net.API mappings used
From the plan §1.5.0 mapping table:
ast.ConstraintUniq/UniqKey/UniqIndexast.ConstrUnique(single)ast.ConstraintIndexast.ConstrIndexast.ConstraintForeignKeyast.ConstrForeignKeyconstraint.Tpconstraint.Typeconstraint.Keys[].Column.Name.String()constraint.Columns(simple) /omniIndexColumns(constraint.IndexColumns)(with expressions)constraint.Refer.Table.Name.String()constraint.RefTable.Nameconstraint.Refer.IndexPartSpecifications[].Column.Name.String()constraint.RefColumnsnode.KeyType == ast.IndexKeyTypeUniquen.Unique(bool)node.IndexPartSpecifications(CreateIndexStmt)n.Columnsast.AlterTableAddConstraintast.ATAddConstraintANDast.ATAddIndex(omni split)ast.AlterTableRenameIndexast.ATRenameIndexspec.FromKey.String()/spec.ToKey.String()cmd.Name/cmd.NewNamenode.OriginTextPosition()(whole statement)ostmt.FirstTokenLine()constraint.OriginTextPosition()(sub-node)ostmt.AbsoluteLine(constraint.Loc.Start)Helper additions to utils.go
OmniStmt.TrimmedText()—strings.TrimSpace(s.Text). Used for embedding statement text in advice content.OmniStmt.FirstTokenLine()— 1-based absolute line of first non-whitespace char. Pingcap-parity for whole-statement advice positions (per cumulative item #6).runNamingConventionRule()— shared scaffold for the index/UK/FK rules (see Dedup follow-up above).namingRuleConfig— config struct for the scaffold.TrimmedTextandFirstTokenLineare also defined on PR #20199 (batch 3). Whichever batch merges second drops the duplicate definition. Trivial conflict resolution.What's NOT in this batch
naming_table/naming_columnadvisors — already migrated in batch 1 (PR refactor(advisor/tidb): migrate naming-table/column/drop rules to omni AST (Phase 1.5 §1.5.3 batch 1) #20195).naming_auto_increment_column— separate advisor, listed in handoff for a future batch.Test plan
go test -count=1 -run "^TestTiDBRules$" ./backend/plugin/advisor/tidb/— greengo test -count=1 ./backend/plugin/advisor/tidb/ ./backend/plugin/advisor/mysql/— green (cross-engine non-regression)go test -count=1 ./backend/plugin/parser/tidb/— greengolangci-lint run --allow-parallel-runners ./backend/plugin/advisor/tidb/...— 0 issuesgo build -ldflags "-w -s" -p=16 -o /tmp/bb-batch4 ./backend/bin/server/main.go— clean🤖 Generated with Claude Code