…tch on batch 9)

Codex correctly flagged: omni preserves the user's literal case
on both column.Name and constraint.Columns — no normalization —
so a case-sensitive lookup misses the PK exemption when the user
writes the column and the PK reference at different casings.

Empirical reproduction (pre-fix):
  CREATE TABLE t(A INT, PRIMARY KEY(a))
  → "Column `t`.`A` doesn't have DEFAULT."
  (incorrectly flagged; `A` is exempt as PK column)

The pingcap-typed predecessor coalesced both sides via `.L` (the
lowercased identifier form). The omni port lost that normalization.

Fix: lowercase on both store (constraint.Columns) and lookup
(column.Name) before the map check. Matches pingcap behavior.

Pinned with two fixture cases in column_require_default.yaml:
- CREATE TABLE t(A INT, PRIMARY KEY(a)) → no advice
- CREATE TABLE t(mycol INT, PRIMARY KEY(MyCol)) → no advice

Out-of-scope but worth flagging: mysql/utils_omni.go's
omniPKColumnNames has the same case-sensitive shape; the mysql
analog likely has the same latent miss on case-divergent PK
references. Long-standing pre-omni behavior (per the same
pattern as cumulative #18's mysql parallel) — not in this PR's
scope, no customer report, no ticket filed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…hase 1.5 §1.5.3 batch 9) (#20295)

* refactor(advisor/tidb): migrate column_require_default to omni AST (Phase 1.5 §1.5.3 batch 9)

Closes BYT-9414. The advisor was the last consumer of the
pingcap-typed needDefault helper in utils.go (kept as a placeholder
since batch 7 when omniNeedDefault was pre-migrated). After this
PR, needDefault and its pingcap mysql/types imports are deleted
from utils.go.

Recipe A. Top-level type switch on CreateTableStmt / AlterTableStmt.
Reuses omniNeedDefault from utils.go (batch-7 work), which already
handles AutoIncrement / Generated / column-level PK / BLOB+TEXT /
JSON / geometry exemptions correctly.

Local helpers:
- omniHasDefaultValue: one-line nil check on omni's ColumnDef.DefaultValue
  (omni surfaces DEFAULT directly; pingcap stored it in the Options list).
- tablePKColumnsFromConstraints: scan table-level Constraint[] for
  ConstrPrimaryKey columns. Mirrors pingcap-typed
  getPkColumnsFromConstraints. Column-level PK is handled by
  omniNeedDefault via the column's own Constraints.

columnData struct is shared with advisor_column_auto_increment_must_integer.go
(same package, same {table, column, line} shape — already declared there).

Step 2a (type-enum cross-reference): zero hits. This advisor does
not read mysql.Type* / column.Tp.GetType() — cleanup batch character.

TEXT-family transitive coverage pin added in fixture
(column_require_default.yaml): ALTER TABLE ADD COLUMN TEXT /
TINYTEXT / MEDIUMTEXT / LONGTEXT — all expected as no-advice
(default-exempt). Locks the cumulative #18 transitive contract:
a future refactor of omniNeedDefault cannot silently drop TEXT
names without failing this fixture.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): normalize PK name lookup to lowercase (Codex P2 catch on batch 9)

Codex correctly flagged: omni preserves the user's literal case
on both column.Name and constraint.Columns — no normalization —
so a case-sensitive lookup misses the PK exemption when the user
writes the column and the PK reference at different casings.

Empirical reproduction (pre-fix):
  CREATE TABLE t(A INT, PRIMARY KEY(a))
  → "Column `t`.`A` doesn't have DEFAULT."
  (incorrectly flagged; `A` is exempt as PK column)

The pingcap-typed predecessor coalesced both sides via `.L` (the
lowercased identifier form). The omni port lost that normalization.

Fix: lowercase on both store (constraint.Columns) and lookup
(column.Name) before the map check. Matches pingcap behavior.

Pinned with two fixture cases in column_require_default.yaml:
- CREATE TABLE t(A INT, PRIMARY KEY(a)) → no advice
- CREATE TABLE t(mycol INT, PRIMARY KEY(MyCol)) → no advice

Out-of-scope but worth flagging: mysql/utils_omni.go's
omniPKColumnNames has the same case-sensitive shape; the mysql
analog likely has the same latent miss on case-divergent PK
references. Long-standing pre-omni behavior (per the same
pattern as cumulative #18's mysql parallel) — not in this PR's
scope, no customer report, no ticket filed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… (Codex round-2 on batch 10)

Codex correctly flagged a residual test gap: batch 10's must_integer
and must_unsigned migrations changed line-position semantics for
multi-line CREATE TABLE — advice now points at the offending column's
line, where pingcap-tidb advice pointed at the CREATE TABLE keyword's
line. Empirically verified:

  CREATE TABLE t(
    a varchar(255) AUTO_INCREMENT   <-- column line: 2
  )

- Pingcap (before this batch): advice startposition.line = 1
- Omni port (this batch): advice startposition.line = 2

Cause: pingcap's pre-existing code captured column.OriginTextPosition()
into columnData.line but then emitted checker.line (statement line) in
the advice — the column line was DEAD CODE. The omni port routes
column line through to the advice via ostmt.AbsoluteLine(column.Loc.Start),
so multi-line CREATE TABLE advice is now more accurate.

Same shape as batch 9 column_require_default's column-line behavior
(also a UX improvement; also pinned in its fixture). I forgot to mirror
the multi-line pin for batch 10; Codex caught it as an unpinned
behavior change at risk of silent drift.

Fix: add one multi-line CREATE TABLE fixture per advisor, expected at
column line (line 2). Future refactor that reverts to statement-line
will fail these fixtures.

Customer-visible behavior change shipping with batch 10: same
deferred-behavior-change framing as cumulative #18's "is text" vs
pingcap's "is blob". UX improvement, not regression — advice now
points at where the actual offending column is, not at the
CREATE TABLE keyword.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…e 1.5 §1.5.3 batch 10) (#20300)

* refactor(advisor/tidb): migrate auto-increment trio to omni AST (Phase 1.5 §1.5.3 batch 10)

Three theme-coupled advisors migrated as one unit (no helper coupling
between them, but theme + sibling parity make ship-as-unit clean):
- advisor_column_auto_increment_must_integer.go
- advisor_column_auto_increment_must_unsigned.go
- advisor_column_auto_increment_initial_value.go

All Recipe A. Pingcap visitors return (in, false) — top-level only.

32 of 51 advisors merged once this lands (29 → 32). 19 remaining +
1 Class III blocked.

## Step 2a fires for the first time

`must_integer`'s pingcap source dispatches on
`case mysql.TypeTiny, mysql.TypeShort, mysql.TypeInt24, mysql.TypeLong, mysql.TypeLonglong:` —
step 2a's grep flagged this in pre-batch protocol.

Cross-reference + empirical parse-test surfaced a new type-byte
unification: **`mysql.TypeTiny` covers TINYINT + BOOL + BOOLEAN**
(BOOL/BOOLEAN are TINYINT aliases in MySQL). Omni splits the alias:
TINYINT stays as `"TINYINT"`, but both BOOL and BOOLEAN pre-normalize
to `"BOOLEAN"` (not "TINYINT"). Same shape as cumulative #18's
BLOB/TEXT charset-distinguished unification.

Without empirical verification, mechanical port of
`omniIsIntegerType` would have flagged `BOOL AUTO_INCREMENT` and
`BOOLEAN AUTO_INCREMENT` as non-integer — regression vs pingcap-tidb.
Fixed in-batch: `omniIsIntegerType` in utils.go includes `"BOOLEAN"`
(diverges from mysql analog's list, which omits it as a long-standing
mysql gap — see cumulative #20). Pinned with three regression-direction
fixtures: `BOOL AUTO_INCREMENT`, `BOOLEAN AUTO_INCREMENT`,
`TINYINT AUTO_INCREMENT` — all expected as no-advice.

**First instance of step 2a working as designed** — caught the class
of bug (charset-style type-byte unification) that batch 8's BLOB/TEXT
miss needed Codex P1 to flag post-merge.

## must_unsigned: Zerofill preservation

Pingcap's `mysql.HasUnsignedFlag(column.Tp.GetFlag())` returns true
when EITHER the unsigned flag OR the zerofill flag is set (ZEROFILL
implies UNSIGNED in MySQL). Omni splits the bits — `Unsigned` and
`Zerofill` are separate booleans. Port checks both via
`Unsigned || Zerofill` (matches mysql analog). Pinned with
`INT ZEROFILL AUTO_INCREMENT` + `INT UNSIGNED ZEROFILL AUTO_INCREMENT`
fixtures (no-advice expected).

## initial_value: preserved pingcap scope (CREATE TABLE only)

Pingcap-typed advisor only matched `*ast.CreateTableStmt` for the
`TableOptionAutoIncrement` check. Mysql analog also handles
`ALTER TABLE ... AUTO_INCREMENT = N` via the ATTableOption arm — that
broader scope is a mysql-side behavior, NOT something the tidb
migration should adopt. Preserve pingcap behavior per invariant #7's
caveat on mysql-analog non-authority. omni's `TableOption.Value` is
string-typed (pingcap's `option.UintValue` was uint64) — parse via
strconv.ParseUint and skip parse failures (same as pingcap's
implicit-skip on type-assert miss).

## Helpers

Added to `utils.go`:
- `omniIsIntegerType(dt *DataType) bool` — covers TINYINT/SMALLINT/
  MEDIUMINT/INT/INTEGER/BIGINT/BOOLEAN (7 names, mapping to pingcap's
  5 type bytes + the TINYINT/BOOLEAN alias split + the cumulative #7
  INTEGER pre-normalization).

`columnData` struct continues to be shared across the three advisors
that use it (require_default + must_integer + must_unsigned) — same
package, same {table, column, line} shape.

## Cumulative #20 added to plan doc

Documents the TINYINT/BOOL/BOOLEAN unification + the "first instance
of step 2a catching the class" framing. Mysql parallel (mysql's
`omniIsIntegerType` omits BOOLEAN) is documented as long-standing
pre-omni gap, NOT regression — consistent with cumulative #19 +
plan invariant #10's regression-vs-long-standing policy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): extract collectColumnViolations helper for column-attribute advisors

SonarCloud flagged batch 10 at 19.3% duplicated lines on new code
(30.6% on advisor_column_auto_increment_must_integer.go, 28.4% on
advisor_column_auto_increment_must_unsigned.go). The flagged
duplication was the checkStmt outer switch + CreateTable column
loop + AlterTable ADD/CHANGE/MODIFY dispatch, repeated verbatim
between the two new advisors. Same shape as batch 8's index-family
extraction (PR #20231 commit dc21c95).

Extract collectColumnViolations(ostmt, isViolation func) []columnData
in utils.go — single predicate parameter, returns the shared
columnData struct, no asymmetric knobs. Each advisor's checkStmt
collapses to a 9-line call site:

  cols := collectColumnViolations(ostmt, func(c *ast.ColumnDef) bool {
      return !autoIncrementColumnIs<X>Omni(c)
  })
  for _, col := range cols {
      c.adviceList = append(c.adviceList, &storepb.Advice{...})
  }

Per-advisor advice formatting + rule-specific predicate stay in
the caller; the helper owns the column-walk shape.

Scope discipline (mirrors batch 8 helper design):
- Used by must_integer + must_unsigned today.
- Extensible to future column-attribute advisors with the same
  "walk columns, apply predicate" shape (column_maximum_character_length,
  column_type_disallow_list).
- NOT appropriate for advisors with table-level filtering
  (column_require_default's table-level PK exemption — dual
  predicate + PK-name set).
- NOT appropriate for advisors reading non-column structures
  (column_auto_increment_initial_value's table options).

Net: -112 lines duplicated scaffolding across 2 advisors,
+77 lines helper (with doc comment explaining scope boundaries).
TestTiDBRules + cross-engine + lint + build all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin multi-line CREATE TABLE column-line semantics (Codex round-2 on batch 10)

Codex correctly flagged a residual test gap: batch 10's must_integer
and must_unsigned migrations changed line-position semantics for
multi-line CREATE TABLE — advice now points at the offending column's
line, where pingcap-tidb advice pointed at the CREATE TABLE keyword's
line. Empirically verified:

  CREATE TABLE t(
    a varchar(255) AUTO_INCREMENT   <-- column line: 2
  )

- Pingcap (before this batch): advice startposition.line = 1
- Omni port (this batch): advice startposition.line = 2

Cause: pingcap's pre-existing code captured column.OriginTextPosition()
into columnData.line but then emitted checker.line (statement line) in
the advice — the column line was DEAD CODE. The omni port routes
column line through to the advice via ostmt.AbsoluteLine(column.Loc.Start),
so multi-line CREATE TABLE advice is now more accurate.

Same shape as batch 9 column_require_default's column-line behavior
(also a UX improvement; also pinned in its fixture). I forgot to mirror
the multi-line pin for batch 10; Codex caught it as an unpinned
behavior change at risk of silent drift.

Fix: add one multi-line CREATE TABLE fixture per advisor, expected at
column line (line 2). Future refactor that reverts to statement-line
will fail these fixtures.

Customer-visible behavior change shipping with batch 10: same
deferred-behavior-change framing as cumulative #18's "is text" vs
pingcap's "is blob". UX improvement, not regression — advice now
points at where the actual offending column is, not at the
CREATE TABLE keyword.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… AST (Phase 1.5 §1.5.3 batch 11) (#20302)

* refactor(advisor/tidb): migrate column_disallow_changing trio to omni AST (Phase 1.5 §1.5.3 batch 11)

Three theme-coupled advisors migrated as one unit. No helper coupling
between them (verified via pre-batch grep) — ship-as-unit is theme-
and reviewer-cadence-driven, not load-bearing.

- advisor_column_disallow_changing.go
- advisor_column_disallow_changing_order.go
- advisor_column_disallow_changing_type.go

All Recipe A. ALTER TABLE only (no CreateTable arm in pingcap-tidb
predecessors; preserved).

35 of 51 advisors merged once this lands (32 → 35). 16 remaining +
1 Class III blocked.

## Single-advice-per-statement cardinality contract preserved

All three pingcap-typed predecessors used a Visitor with `break` after
the first match, emitting at most ONE advice per top-level ALTER TABLE
statement. The omni port replicates this with explicit `break`. **Mysql
analog diverges** — emits per-cmd without breaking, producing N advices
for N matching specs. Cardinality divergence preserved on the tidb side
per invariant #7's "preserve pingcap behavior" rule.

Pinned each advisor with multi-CHANGE/MODIFY fixtures (expected: 1
advice for 2-spec statement).

## changing_type: cumulative #21 — latent pingcap false-positive fixed

Empirical finding from pre-batch protocol step 2a:

Pingcap's `FieldType.String()` rendering appends a ` BINARY` charset
annotation on types stored with binary charset:
  BLOB        → "blob BINARY"
  TINYBLOB    → "tinyblob BINARY"
  VARBINARY(N) → "varbinary(N) BINARY"

The pingcap-typed `disallow_changing_type` rule compared this rendered
string against `column.GetProto().Type` from the catalog (which stores
the bare type name from INFORMATION_SCHEMA — no charset suffix). The
`normalizeColumnType` helper has no "blob" / "blob binary" case, so its
default branch is identity-lowercase. Result:

  "blob" (catalog) ≠ "blob binary" (pingcap render)
  → rule flagged `ALTER TABLE t MODIFY a BLOB` on a BLOB column
    (no-op type change) as a type change.

LATENT FALSE-POSITIVE on the pingcap-typed advisor.

Omni's `DataType.Name` is the bare type name with no charset suffix.
The omni port builds the type string from `Name + Length + Scale +
Unsigned` (omniBuildColumnTypeString helper). Catalog "blob" matches
omni-built "blob" → false-positive silently fixed by this migration.

**Not a regression; a UX improvement.** Same shape as cumulative #18's
"is text" advice content fix.

**Why no fixture pin:** mock test catalog (MockMySQLDatabase.tech_book)
doesn't carry a BLOB column; adding one would have cross-engine impact
(mysql + tidb + mariadb + oceanbase all share it). The fix is locked
conceptually via plan-doc cumulative #21, empirically verified during
pre-batch protocol via parse-through-both-engines test.

## FIRST/AFTER detection for changing_order

Pingcap: `spec.Position.Tp != ast.ColumnPositionNone`.
Omni: `cmd.First || cmd.After != ""` (separate fields; both empty when
no position specified). Verified empirically.

## Helpers

In advisor_column_disallow_changing_type.go (local, not extracted):
- omniBuildColumnTypeString(dt *DataType) string — renders the
  "name[(length[,scale])] [unsigned]" form. Mirrors the mysql analog's
  inline rendering. Local because the rendering specifically targets
  the normalizeColumnType contract used only by this advisor.
- normalizeColumnType — kept structurally identical to the pingcap-
  typed predecessor + mysql analog for fixture parity.

## Cumulative #21 added to plan doc

Documents the BLOB/BINARY charset-suffix latent false-positive fix.
Mysql parallel verified free of the same issue (mysql analog rebuilt
type string from DataType during omni migration, same as my tidb port).

## Mysql cardinality divergence noted but not filed

The three mysql analogs emit per-cmd (no break), which differs from
both pre-omni mysql (likely had break) and pingcap-tidb (had break).
This is a potential mysql regression introduced by the omni migration,
but verifying pre-omni behavior requires git-show + parse test that's
out of scope for this PR. Flagged here for future investigation under
plan invariant #10 if/when a customer reports duplicate advices on
multi-CHANGE statements.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): extract single-shot alter-table helper + fix DECIMAL(N)/ZEROFILL type rendering (Codex round-2 on batch 11)

Two Codex P2 findings + one Sonar duplication signal addressed:

## Codex P2 #1: zero-scale DECIMAL rendering

Empirical reproduction:
  pingcap: DECIMAL(10) → Tp.String() = "decimal(10,0)"   (explicit 0 scale)
  pingcap: DECIMAL(10,0) → Tp.String() = "decimal(10,0)" (canonicalized)
  omni (current): both → DataType{Length=10, Scale=0}
  omni-built (pre-fix): "decimal(10)"   (Scale=0 → no comma form)

Catalog stores "decimal(10,0)" from INFORMATION_SCHEMA — false-positive
on no-op MODIFY when user writes DECIMAL(10) or DECIMAL(10,0).

Fix: omniBuildColumnTypeString now renders explicit scale when
Length > 0 AND the type is decimal-family (decimal/numeric/fixed/
float/double/real). New helper isDecimalFamilyTypeName mirrors the
MySQL canonicalization convention.

## Codex P2 #2: ZEROFILL rendering

Empirical reproduction:
  pingcap: INT ZEROFILL → Tp.String() = "int(11) UNSIGNED ZEROFILL"
  pingcap: INT UNSIGNED ZEROFILL → same
  omni (current): DataType{Unsigned: false-or-true, Zerofill: true}
  omni-built (pre-fix): "int" / "int unsigned"   (zerofill dropped)

Fix: append " zerofill" when Zerofill is set; treat Zerofill as
implying Unsigned in the rendering (pingcap convention; MySQL storage
requires UNSIGNED whenever ZEROFILL is set).

Caveat documented in code: MySQL canonicalizes ZEROFILL display widths
to maximums (int 11 → 10 etc.); the normalizeColumnType map doesn't
cover zerofill cases, so display-width canonicalization differences
may still surface as false-positives on rare ZEROFILL no-op modifies.
Out of scope to fix (ZEROFILL is deprecated in MySQL 8.0.17+).

## Sonar duplication: extract firstAlterCommandMatching

PR #20302 was flagged at 18.8% duplicated lines on new code (59.1% on
advisor_column_disallow_changing.go, 54.5% on advisor_column_disallow_changing_order.go).
The flagged duplication was the same shape across both single-shot
advisors:

  for _, ostmt := range stmts {
      alter, ok := ostmt.Node.(*ast.AlterTableStmt)
      if !ok { continue }
      for _, cmd := range alter.Commands {
          if cmd == nil { continue }
          if <matcher> { append advice; break }
      }
  }

Extracted firstAlterCommandMatching(n, matcher) int in utils.go —
single matcher callback, returns first matching index or -1. Each
single-shot advisor's body collapses to one matcher closure + one
conditional advice emit. Same shape as batch 8's collectIndexFamily*
and batch 10's collectColumnViolations helpers.

advisor_column_disallow_changing_type is structurally different
(catalog lookup, no break) — does NOT use this helper.

## No fixture pins for the rendering fixes

Same constraint as cumulative #21's original framing: the mock test
catalog (MockMySQLDatabase.tech_book + orders) has no decimal(N,0) or
ZEROFILL columns; adding them would have cross-engine impact. The
fixes are locked conceptually via the cumulative #21 plan-doc entry +
the empirical verification recorded in this commit message.

Net: net-zero LOC (helper offsets extraction); duplication eliminated;
two latent false-positive cases fixed; cumulative #21 strengthened.
TestTiDBRules + cross-engine + lint + build all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): restrict zero-scale rendering to exact-decimal types (Codex round-3 on batch 11)

Codex P2 correctly flagged: my omniBuildColumnTypeString from the
previous round treated float/double/real like DECIMAL — always
rendering (length, scale) when Length > 0. Empirical verification
shows MySQL/pingcap behavior differs between exact and approximate
numeric types:

  Exact-precision (DECIMAL / NUMERIC / FIXED):
    DECIMAL       → pingcap "decimal(10,0)"  (defaults applied)
    DECIMAL(10)   → pingcap "decimal(10,0)"  (canonicalized)
    DECIMAL(10,0) → pingcap "decimal(10,0)"
    DECIMAL(10,2) → pingcap "decimal(10,2)"

  Approximate (FLOAT / DOUBLE / REAL):
    FLOAT         → pingcap "float"
    FLOAT(10)     → pingcap "float"          (precision hint DROPPED)
    FLOAT(10,0)   → pingcap "float"          (precision hint DROPPED)
    FLOAT(10,2)   → pingcap "float(10,2)"    (only when scale > 0)
    DOUBLE(10,0)  → pingcap "double"         (same shape)
    REAL          → pingcap "double"         (REAL is aliased to DOUBLE)
    REAL(10,0)    → pingcap "double"
    REAL(10,2)    → pingcap "double(10,2)"

Catalog stores what MySQL info_schema returns — same canonicalization.

Pre-fix behavior with float/double/real in the decimal-family list:
  MODIFY price FLOAT(10) → my port renders "float(10,0)"
  Catalog (real DB):     stores "float"
  Comparison:            DIFFERENT → false-positive

Post-fix (this commit):
- isExactDecimalTypeName covers decimal / numeric / fixed only.
- isApproximateFloatTypeName covers float / double / real — these
  drop both Length and Scale unless Scale > 0 (rendering bare type
  name instead).
- All other types: render (Length) when Length > 0, no scale.

Honest reflection: third rendering shape divergence Codex has caught
on this single advisor (cumulative #21 BLOB BINARY → batch-11 commit-1
DECIMAL(10,0) + ZEROFILL → this commit FLOAT/DOUBLE/REAL). The common
pattern is that pingcap's `Tp.String()` has many type-specific
canonicalization rules that my builder needs to mirror per-family,
not generalize across. The "approximate vs exact" split is the
canonical MySQL distinction and should be the structural framing
for future migration of any rule that renders type-strings for
comparison.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): preserve ENUM/SET value literals in type rendering (Codex round-4 on batch 11)

Codex P2 round-4 correctly flagged: omniBuildColumnTypeString dropped
EnumValues entirely. Empirical reproduction:

  Pingcap ENUM('x','y','z')  → Tp.String() = "enum('x','y','z')"
  Pingcap SET('a','b','c')   → Tp.String() = "set('a','b','c')"
  Omni                       → DataType{Name="ENUM", EnumValues=[x y z]}
  Omni-built (pre-fix)       → "enum"  (values dropped entirely)

Catalog stores the literal value list via MySQL info_schema's
COLUMN_TYPE column (verified per backend/plugin/db/tidb/sync.go).

Pre-fix false-positive:
  catalog: "enum('a','b')"
  user:    ALTER TABLE t MODIFY status ENUM('a','b')   (no-op)
  omni-built: "enum"
  comparison: "enum('a','b')" ≠ "enum" → flagged as type change

Fix:
- isEnumOrSetTypeName(lower) covers "enum" and "set".
- formatEnumValueList(values []string) renders the canonical
  `'v1','v2',…` form with single-quote escaping (' → '') per SQL
  convention. Matches pingcap's Tp.String() output AND info_schema's
  COLUMN_TYPE rendering.
- ENUM/SET case takes precedence over length/scale rendering in the
  switch (the value list is the canonical body for these types).

Honest pattern recognition: this is the FOURTH round of Codex catches
on omniBuildColumnTypeString in a single batch:
  #1 (cumulative #21):  BLOB/TINYBLOB/VARBINARY " BINARY" suffix
  #2 (round-2 commit):  DECIMAL(N) zero-scale + ZEROFILL drop
  #3 (round-3 commit):  FLOAT/DOUBLE/REAL precision-hint drop
  #4 (this commit):     ENUM/SET value literal drop

The common failure mode: my builder enumerates the DataType fields I
already knew about (Name + Length + Scale + Unsigned + Zerofill) but
silently drops fields I didn't anticipate (Charset annotations,
EnumValues). The discipline I should have started with:

  enumerate ALL DataType fields (Name, Length, Scale, Unsigned,
  Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) and ask
  per-field: does pingcap's Tp.String() render it? Does the catalog
  store it? If yes to either, my builder must render it.

That's the field-by-field audit that would have caught all four
rounds in one pass. Adding to the pre-batch protocol for any future
advisor that uses Tp.String() comparison against a catalog string.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize BOOLEAN ↔ tinyint(1) in normalizeColumnType (Codex round-5 on batch 11)

Codex P2 round-5 correctly flagged: omniBuildColumnTypeString renders
DataType.Name verbatim ("boolean"), but TiDB/MySQL stores BOOLEAN
columns as tinyint(1) (verified via backend/plugin/schema/tidb/get_database_metadata.go:437-438
and backend/plugin/schema/mysql/get_database_metadata.go:259-261).

Empirical reproduction:

  Pingcap BOOL/BOOLEAN     → Tp.String() = "tinyint(1)"  (canonicalized)
  Pingcap TINYINT(1)       → Tp.String() = "tinyint(1)"  (same)
  Pingcap TINYINT (bare)   → Tp.String() = "tinyint(4)"  (default width)
  Omni BOOL/BOOLEAN        → DataType{Name="BOOLEAN", Length=0}
  Omni TINYINT(1)          → DataType{Name="TINYINT", Length=1}

Catalog stores "tinyint(1)" for boolean columns.

Pre-fix false-positive:
  catalog:    "tinyint(1)"
  user:       ALTER TABLE t MODIFY x BOOLEAN   (no-op against a boolean column)
  omni-built: "boolean"
  normalize:  default branch → identity → "boolean"
  comparison: "boolean" ≠ "tinyint(1)" → flagged as type change

Fix: add `"boolean"` and `"bool"` cases to normalizeColumnType, both
mapping to `"tinyint(1)"`. Placed in normalizeColumnType rather than
omniBuildColumnTypeString because the normalize layer is where the
existing canonicalization conventions live (integer default widths
already follow this pattern: `int` ↔ `int(11)`, `tinyint` ↔ `tinyint(4)`).

Both sides of the comparison go through normalize, so the canonicalization
is symmetric — works regardless of which side originally rendered which
form.

Codex round-5 of 5 on omniBuildColumnTypeString in this PR:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias normalization     → this commit

The field-by-field DataType audit (Name, Length, Scale, Unsigned,
Zerofill, Charset, Collate, EnumValues, ArrayDim, SRID) that I
identified in round-4 as the missing pre-batch discipline would NOT
have caught round-5 — this is a TYPE-NAME canonicalization concern,
not a field-rendering concern. The pre-batch discipline addition for
this class: when porting a rule that compares type-strings against
the catalog, audit normalizeColumnType for alias coverage by
cross-referencing MySQL's documented type aliases (BOOL/BOOLEAN ↔
TINYINT(1), DEC ↔ DECIMAL, REAL ↔ DOUBLE, INTEGER ↔ INT, etc.) and
backend/plugin/schema/*/get_database_metadata.go's explicit
normalization map.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize bare DECIMAL/NUMERIC/FIXED to decimal(10,0) (Codex round-6 on batch 11)

Codex P2 round-6 correctly flagged: omniBuildColumnTypeString renders
bare DECIMAL (no parens) as "decimal" (Length=0 → default case),
while pingcap Tp.String() and MySQL INFORMATION_SCHEMA both
canonicalize to "decimal(10,0)" (MySQL defaults M=10, D=0).

Pre-fix false-positive:
  catalog:    "decimal(10,0)"
  user:       ALTER TABLE t MODIFY amount DECIMAL   (no-op against decimal(10,0))
  omni:       Name="DECIMAL" Length=0 Scale=0
  omni-built: "decimal"
  normalize:  default branch → identity → "decimal"
  comparison: "decimal" ≠ "decimal(10,0)" → flagged as type change

Empirical (from prior verification): pingcap renders bare DECIMAL,
DECIMAL(10), and DECIMAL(10,0) all as "decimal(10,0)" — MySQL applies
the default precision regardless of which syntactic form the user
typed.

Fix: add `"decimal"`, `"numeric"`, `"fixed"` cases to
normalizeColumnType, all mapping to `"decimal(10,0)"`. NUMERIC and
FIXED are MySQL aliases for DECIMAL — included defensively for the
same canonicalization story. Same layer as the round-5 BOOLEAN
canonicalization and the integer default-width entries — symmetric
across both sides of the comparison.

Codex round-6 of 6 (so far) on the type-string comparison path:
  #1 BLOB/TINYBLOB/VARBINARY " BINARY" suffix     → cumulative #21
  #2 DECIMAL zero-scale + ZEROFILL drop           → round-2 commit
  #3 FLOAT/DOUBLE/REAL precision-hint drop        → round-3 commit
  #4 ENUM/SET value-literal drop                  → round-4 commit
  #5 BOOLEAN ↔ tinyint(1) alias                   → round-5 commit
  #6 bare DECIMAL ↔ decimal(10,0) default         → this commit

Refined pre-batch protocol going forward (third axis added):

  For any rule that compares type-strings against catalog, audit:
    (a) omniBuildColumnTypeString — every DataType field that
        pingcap renders or catalog stores. Field-by-field check
        across Name, Length, Scale, Unsigned, Zerofill, Charset,
        Collate, EnumValues, ArrayDim, SRID.
    (b) normalizeColumnType — every MySQL type alias and default-
        application. Cross-reference backend/plugin/schema/*/
        get_database_metadata.go's normalization map.
    (c) Bare-form defaults — for every type with implicit precision
        defaults (DECIMAL → (10,0), INT → (11), TINYINT → (4), etc.),
        verify the bare-form is canonicalized at the normalize layer.

Axes (a) caught rounds 2-4. (b) caught round 5. (c) caught round 6.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin type-rendering rounds 1-7 via pure-function unit tests; close round-7 risk with BIT/BINARY/YEAR canonicalization

Peer review on PR #20302 flagged three concerns. Addressing all three.

## #1: Pure-function unit tests (peer recommendation #1)

Six rounds of Codex catches across the PR all carried the same
"no fixture pin due to shared mock catalog cross-engine impact"
deferral. Peer correctly identified this as a test-infrastructure
limitation, not a fundamental constraint — the fixes are pure
(DataType, string) → string transforms and can be tested package-
locally without the advisor/catalog framework.

Added advisor_column_disallow_changing_type_test.go with three
suites:

  - TestOmniBuildColumnTypeString: 31 cases covering rounds 1-7
    (BLOB family, exact-decimal, ZEROFILL, FLOAT/DOUBLE/REAL,
    ENUM/SET escape, bare-form defaults, regulars).

  - TestNormalizeColumnType: 18 cases covering all canonicalization
    rules (BOOLEAN ↔ tinyint(1), bare DECIMAL ↔ decimal(10,0),
    bare BIT/BINARY/YEAR ↔ defaults, integer default widths,
    identity branch).

  - TestTypeStringRoundTripAgainstNormalize: 24 cases asserting the
    load-bearing invariant for columnTypeChanged — that
    normalize(omni-built) == normalize(catalog) across every
    no-op-modify scenario we've fixed. Plus negative cases (actual
    type changes) confirming the invariant correctly rejects them.

~120 LOC of tests covering every Codex round's empirical receipt.
Locks the contract in CI; future refactors of the type-rendering
path that re-introduce any class of bug will fail these tests.

## #2: Round-7 risk check (peer recommendation #2)

Ran the harness across edge type families (BIT, JSON, GEOMETRY,
spatial, BINARY/VARBINARY, COLLATE columns, DATETIME with fractional
seconds, YEAR, integer display widths). Surfaced THREE real round-7
candidates pre-merge:

  - bare BIT     → pingcap "bit(1)",     omni-built "bit"     (mismatch)
  - bare BINARY  → pingcap "binary(1)",  omni-built "binary"  (mismatch)
  - bare YEAR    → pingcap "year(-1)",   omni-built "year"    (mismatch, plus pingcap render is itself a `-1` sentinel artifact; catalog stores year(4))

Plus two expected-divergence cases (cumulative #21 shape — pingcap-
side ` BINARY` / ` CHARACTER SET` annotation artifacts that omni
correctly omits, matching catalog).

Fixed all three at the normalize layer (same as BOOLEAN round-5 +
DECIMAL round-6 pattern):

  case "bit":    return "bit(1)"
  case "binary": return "binary(1)"
  case "year":   return "year(4)"

Symmetric across both comparison sides; bounded by the empirical
verification harness; locked by the unit tests above.

The round-7 risk check finished with all remaining cases either
matching or known-fix-direction. Strong confidence the type-string
comparison path is settled.

## #3: Mysql cardinality claim verified (peer recommendation #3)

Previous PR description's batch-11 commit body claimed:
  "potential mysql regression introduced by the mysql omni
   migration ... verifying pre-omni behavior requires git-show
   + parse test that's out of scope for this PR"

This was an unverified-regression assertion — exactly the class
plan invariant #10 was designed to prevent.

Verified empirically (~3 minutes via git-show 699ff60^):
pre-omni mysql also emitted per-cmd, no break. The "for each item
in AlterListItem ... AddAdvice(...) ..." loop is structurally
identical to the post-omni "for each cmd ... AddAdvice(...)"
loop. Per-cmd cardinality is LONG-STANDING pre-omni behavior
preserved by the omni migration, NOT a regression.

Plan-doc cumulative table entry not needed (no novel pattern;
matches existing #19 framing). PR description correction will
follow in the merge body.

## Net for PR #20302

  - 6 Codex rounds of type-rendering fixes (rounds 1-6)
  - Round-7 risk check found 3 more pre-merge fixes (BIT/BINARY/YEAR)
  - 49 unit-test cases pinning every round's contract
  - mysql cardinality claim corrected per empirical verification

The type-string comparison path on this advisor is now bounded by
unit tests in CI. Future regressions of the rounds-1-7 classes
have a test guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): move bare-form canonicalization into builder for composability with UNSIGNED/ZEROFILL modifiers (Codex round-8 on batch 11)

Codex P2 (two comments, one underlying issue) + peer review correctly
identified the same gap: bare-form types with modifiers (UNSIGNED,
ZEROFILL) bypassed both the builder's family-specific bare-form
handling AND the normalize layer's canonicalization map.

Empirical reproduction (pingcap renders the canonical default
precision EVEN for bare forms with modifiers):

  DECIMAL UNSIGNED         → "decimal(10,0) UNSIGNED"
  DECIMAL ZEROFILL         → "decimal(10,0) UNSIGNED ZEROFILL"
  DECIMAL UNSIGNED ZEROFILL→ "decimal(10,0) UNSIGNED ZEROFILL"
  INT ZEROFILL             → "int(11) UNSIGNED ZEROFILL"
  TINYINT ZEROFILL         → "tinyint(4) UNSIGNED ZEROFILL"

Catalog stores the same canonical form. My pre-fix builder emitted
"decimal unsigned" / "int unsigned zerofill" — false-positive on
no-op `MODIFY x DECIMAL UNSIGNED` against a `decimal(10,0) unsigned`
column. normalizeColumnType had entries for bare names ("decimal",
"int") but not for bare + modifier ("decimal unsigned",
"int unsigned zerofill") — the Cartesian product is unbounded as a
normalize map.

## Structural fix: canonicalize at builder, not normalize

Per peer recommendation, the fix moves canonicalization to the
builder. Round-3's FLOAT/DOUBLE/REAL path was already builder-based;
making the bare-form canonicalization builder-based too means the
canonical-base + modifier suffix composes naturally:

  buildBaseTypeForm: emits canonical body (decimal(10,0), int(11),
                     tinyint(4), bit(1), binary(1), year(4),
                     tinyint(1) for BOOLEAN — via the new
                     canonicalBareTypeForm map). FLOAT/DOUBLE/REAL
                     keep their bare-form (no MySQL default).
  Modifier suffixes:  appended once at the end (unsigned + zerofill).

Now any bare-form × {UNSIGNED, ZEROFILL, UNSIGNED ZEROFILL}
combination composes correctly without combinatorial normalize
entries. New normalize entries from rounds 5/6/7 (BOOLEAN ↔ tinyint(1),
bare DECIMAL ↔ decimal(10,0), bare BIT/BINARY/YEAR ↔ defaults) stay
as belt-and-suspenders — they handle the case where catalog ever
stores a bare form, even though catalog typically canonicalizes.

## Discipline refinement: axis (c) was incomplete

Post-round-6 the discipline had three axes:
  (a) Field-by-field DataType audit         (caught rounds 2, 3, 4)
  (b) Type-name alias canonicalization      (caught round 5)
  (c) Bare-form defaults                    (caught round 6)

Round 8 reveals (c) was only enumerating bare-form, not bare-form ×
{applicable modifiers}. Revised:

  (c-revised) For each canonical-bare-form type family, enumerate
              the Cartesian product with applicable modifiers
              (UNSIGNED, ZEROFILL, UNSIGNED ZEROFILL for integer +
              exact-decimal families; verify no modifiers for
              BIT/BINARY/YEAR/BOOLEAN; verify FLOAT-family does
              NOT apply default precision under modifiers).

## Tests

Updated TestOmniBuildColumnTypeString:
  - Replaced old round-2 "INT ZEROFILL → int unsigned zerofill"
    case (which was pinning the broken bare-form behavior) with
    the canonical "int(11) unsigned zerofill" output.
  - Added 18 new cases covering the Cartesian product:
    DECIMAL/NUMERIC × {UNSIGNED, ZEROFILL, UNSIGNED+ZEROFILL},
    TINYINT/SMALLINT/MEDIUMINT/INT/BIGINT × {ZEROFILL, UNSIGNED+ZEROFILL,
    UNSIGNED}, FLOAT/DOUBLE × {UNSIGNED} (verifies float-family
    keeps bare form even under modifier).

Updated TestTypeStringRoundTripAgainstNormalize:
  - 6 new round-8 no-op-modify scenarios with bare-form × modifier.
    Catalog stores canonical (decimal(10,0) unsigned etc.); builder
    now produces matching form. normalize(both) yields equal strings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(advisor/tidb): canonicalize NUMERIC alias to decimal in builder (Codex round-9 on batch 11)

Codex P2 round-9 correctly flagged: buildBaseTypeForm renders the
exact-decimal family verbatim from DataType.Name. NUMERIC(8,2)
emits "numeric(8,2)"; catalog stores "decimal(8,2)" (per
backend/plugin/schema/tidb/get_database_metadata.go line ~440
which does `strings.Replace(fullType, "numeric", "decimal", 1)`).

Empirical reproduction:

  Pingcap NUMERIC(8,2)   → Tp.String() = "decimal(8,2)"  (canonicalized)
  Pingcap FIXED(8,2)     → Tp.String() = "decimal(8,2)"  (canonicalized)
  Pingcap DEC(8,2)       → Tp.String() = "decimal(8,2)"  (canonicalized)
  Omni NUMERIC(8,2)      → Name="NUMERIC" Length=8 Scale=2   (NOT canonicalized)
  Omni FIXED(8,2)        → Name="DECIMAL" Length=8 Scale=2   (pre-canonicalized)
  Omni DEC(8,2)          → Name="DECIMAL" Length=8 Scale=2   (pre-canonicalized)
  Omni INTEGER(8)        → Name="INT" Length=8               (pre-canonicalized)

So omni pre-normalizes FIXED / DEC / INTEGER but NOT NUMERIC. My
builder previously emitted whatever Name carried — NUMERIC(8,2)
produced "numeric(8,2)" verbatim.

Fix: render the canonical name "decimal" for the exact-decimal
family in buildBaseTypeForm. Same builder-layer pattern as round-8
(canonicalize at the source rather than relying on normalize map
entries). Defensively keeps FIXED in the isExactDecimalTypeName
list even though omni currently pre-normalizes it — future grammar
changes won't introduce a regression.

## Tests

Updated TestOmniBuildColumnTypeString:
  - Replaced "NUMERIC(8) → numeric(8,0)" case (which was pinning
    the broken non-canonicalized output) with "NUMERIC(8) → decimal(8,0)".
  - Added new cases: NUMERIC(8,2), FIXED(8,2) (defensive), and
    NUMERIC(10,2) UNSIGNED.

Updated TestTypeStringRoundTripAgainstNormalize:
  - 2 new round-9 no-op-modify scenarios: NUMERIC(8,2) against
    catalog "decimal(8,2)" and bare NUMERIC against catalog
    "decimal(10,0)".

## Discipline refinement (axis-b extension)

Round 9 reveals axis (b) — type-name alias canonicalization — also
applies to TYPED-form aliases, not just bare-form aliases. Round 5
covered BOOLEAN ↔ tinyint(1) (bare alias). Round 9 covers
NUMERIC(M,D) ↔ DECIMAL(M,D) (typed alias preserving M,D).

Revised (b): For each type alias MySQL canonicalizes, verify the
canonicalization in BOTH the bare form AND every typed form
(typed length/scale/precision combinations). Bare form lives in
canonicalBareTypeForm; typed form lives in buildBaseTypeForm.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…lative #22 (Phase 1.5 §1.5.3 batch 13) (#20312)

* refactor(advisor/tidb): migrate type-byte pair to omni AST + add cumulative #22 (Phase 1.5 §1.5.3 batch 13)

Two advisors with step-2a-flagged type-byte dispatches, validating
the three-axis discipline from batch 11 on first real use:
- advisor_column_current_time_count_limit (DATETIME/TIMESTAMP)
- advisor_column_maximum_character_length (TypeString CHAR/BINARY)

39 of 51 advisors merged once this lands (37 → 39). 12 remaining +
1 Class III blocked.

## column_current_time_count_limit

Recipe A with cross-statement state accumulator. Pingcap dispatched
on `mysql.TypeDatetime, mysql.TypeTimestamp` — distinct type bytes,
no unification. Omni surfaces both as their own DataType.Name
("DATETIME"/"TIMESTAMP"), so the type-name predicate is a straight
mapping via the new omniIsTimeType helper.

CURRENT_TIMESTAMP detection: pingcap walked column.Options[] looking
for ColumnOptionDefaultValue/OnUpdate with *ast.FuncCallExpr whose
FnName.L matched the synonyms. Omni surfaces these as separate
top-level fields on ColumnDef:
  - col.DefaultValue ExprNode → DEFAULT expression
  - col.OnUpdate ExprNode → ON UPDATE expression
Both fields can carry a *omniast.FuncCallExpr with Name (case-
preserved from user input). New omniIsDefaultCurrentTime /
omniIsOnUpdateCurrentTime / omniIsCurrentTimeFuncCall helpers
(in utils.go) preserve pingcap's case-insensitive synonym
matching via strings.ToUpper.

Line attribution: preserved pingcap's statement-line behavior
(`ostmt.AbsoluteLine(n.Loc.Start)`). Mysql analog diverges —
uses column-line (`r.LocToLine(col.Loc)`) — but the advice content
is table-level ("Table t has N CURRENT_TIMESTAMP columns") so
statement-line is the natural choice. Preserve pingcap.

## column_maximum_character_length — cumulative #22 territory

Step 2a flagged `mysql.TypeString` dispatch. Empirical
cross-reference against the three-axis discipline identified
TypeString as the same family-pair unification as TypeBlob (#18)
and TypeTiny (#20):

  Pingcap mysql.TypeString covers:
    - CHAR (non-binary charset)
    - BINARY (binary charset)

  Omni splits to distinct DataType.Name:
    - CHAR → "CHAR"
    - BINARY → "BINARY"

The pingcap-typed rule fired on BOTH (TypeString match was charset-
agnostic). A mechanical port matching only "CHAR" would silently
drop BINARY coverage. Mysql analog (`getCharLengthFromOmni` in
utils_omni.go) only matches "CHAR" — preserves a pre-omni mysql
ANTLR gap (CHAR_SYMBOL distinct from BINARY_SYMBOL); long-standing,
not regression. No mysql ticket per invariant #10.

Fix:
- omniIsCharOrBinaryType (utils.go): matches both names.
- omniCharLength (utils.go): returns length, defaulting to 1 for
  bare CHAR/BINARY (MySQL default; pingcap's GetFlen() returned
  this canonically). Returns 0 for non-CHAR/BINARY types (callers
  gate on the predicate first).

Cardinality: pingcap-tidb broke after first match → ONE advice per
top-level statement. Mysql analog emits per-column. Preserve pingcap
via early-return-on-first-match in checkStmtForCharLength. Pinned
with multi-column fixture (CHAR(225) + BINARY(225)) expecting 1
advice.

## Cumulative #22 added to plan doc

Documents TypeString CHAR/BINARY unification with the empirical
mysql-pre-omni-ANTLR-gap framing. Step 2a checklist updated to
include TypeString alongside TypeBlob (#18) and TypeTiny (#20).

Second consecutive step-2a-preempted batch (batch 10 caught TINYINT/
BOOLEAN, batch 13 caught CHAR/BINARY). The three-axis discipline
from batch 11 is paying off — both bugs were the same family-pair
unification class, both caught pre-merge via the same grep + cross-
reference protocol.

## Pre-batch protocol receipts

- [x] Both pingcap visitor bodies read side-by-side. Both return
  `(in, false)` from Enter → Recipe A.
- [x] Coupling scan: 5 new helpers in utils.go (time + char/binary
  pair); no cross-file coupling between advisors.
- [x] Step 2a: 2 type-byte hits flagged; cross-referenced; TypeDatetime/
  TypeTimestamp are distinct (no unification concern), TypeString is
  unified per cumulative #22 (new entry added).
- [x] Three-axis discipline applied:
   (a) Field-by-field DataType audit: covered Length, Name; CHAR
       length default verified via omniCharLength tests
   (b) Type-name alias canonicalization: TypeString → CHAR/BINARY
       split documented in cumulative #22
   (c) Bare-form × modifier Cartesian product: N/A (this rule doesn't
       compare type strings against a catalog, just reads Length)
- [x] Shape audit against mysql analogs. Mysql diverges on:
   - line attribution (column-line vs statement-line) — preserved pingcap
   - cardinality (per-col vs single-advice-per-stmt) — preserved pingcap
   - BINARY coverage (CHAR-only in mysql) — restored full pingcap-tidb behavior
- [x] Unit tests written in commit-1 (batch 11 discipline):
   - TestOmniIsTimeType (8 cases)
   - TestOmniIsCurrentTimeFuncCall (8 cases — synonyms + negatives)
   - TestOmniIsCharOrBinaryType (8 cases — CHAR/BINARY positive +
     VARCHAR/VARBINARY/etc. negative)
   - TestOmniCharLength (8 cases — default-1, explicit lengths,
     non-CHAR/BINARY)
- [x] Fixture pins for BINARY coverage + cardinality preservation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): include actual length in COLUMN_MAXIMUM_CHARACTER_LENGTH advice (batch 13 follow-up)

Previously the advice only mentioned the maximum ("...is bigger than 20...");
now it includes the offending column's actual length ("...is 225, bigger than
20..."). UX improvement for users diagnosing CHAR/BINARY length violations.

Also clears SonarCloud godre:S8193 ("unnecessary variable declaration") on
the three `if charLength := omniCharLength(...); charLength > maximum`
sites — charLength is now consumed in the advice content, not just the
predicate.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ovement on insert_disallow_order_by_rand

Peer review caught a silent behavior change at the UNION boundary
that the initial batch 14 PR description elided.

Pre-omni pingcap-tidb's `InsertStmt.Select` is a `ResultSetNode`
interface. For `INSERT INTO t SELECT ... UNION SELECT ... ORDER BY
RAND()` the concrete type is `*ast.SetOprStmt`, NOT
`*ast.SelectStmt`. The pre-omni Enter's `insert.Select.(*ast.SelectStmt)`
cast failed and the whole check was silently skipped — pre-omni
rule did NOT flag UNION-result ORDER BY RAND despite the rule's
name and stated intent.

Omni's `InsertStmt.Select` is `*SelectStmt` (concrete pointer)
regardless of SetOp value; UNION'd inputs surface as
`SelectStmt{SetOp: SetOpUnion, Left: ..., Right: ..., OrderBy: [...]}`.
Direct access to `ins.Select.OrderBy` lands on the outer-UNION ORDER
BY list. Rule now fires — silent UX improvement matching the rule's
actual intent. Same UX-improvement shape as cumulative #18 ("is text"
advice content fix) and #21 (latent BLOB MODIFY false-positive).

Verified empirically by parsing the same input through both pingcap-
and omni-tidb parsers and inspecting `%T` / `SetOp` / `OrderBy[0]`.

Updates:
- Advisor comment now accurately describes outer-vs-inner UNION
  boundary distinction (outer-UNION OrderBy newly covered; per-arm
  parenthesized OrderBy remains uncovered — omni rejects that syntax
  in INSERT position, separate Phase 2 grammar gap).
- New positive fixture pin asserting the rule fires on the
  un-parenthesized UNION form.

NEW pre-batch audit axis added in plan-doc cumulative #24: when
pingcap source uses a narrow type-assertion on an interface-typed
field (e.g. `.(*ast.SelectStmt)` on `ResultSetNode`), the omni port
may newly reach inputs the old code accidentally filtered out — the
omni equivalent field is often concrete-typed, so direct access
bypasses the implicit filter. Audit every type-assertion: ask "does
this assertion narrow the input set?"

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…i AST + cumulative #23 (Phase 1.5 §1.5.3 batch 14) (#20318)

* fix(advisor/tidb): preserve grouped ADD COLUMN last-wins on column_maximum_character_length (cumulative #23)

Pingcap-tidb's pre-omni column_maximum_character_length had asymmetric
loop control between two call-sites in the same Enter method:
- CreateTableStmt arm: inner column loop with `break` → first-wins
- AlterTableStmt ATAddColumns arm: inner column loop with NO break →
  last-wins (each subsequent violation overwrites the previous one;
  outer `break` fires only after the inner loop completes)

Externally observable on grouped form:
  ALTER TABLE t ADD COLUMN (a CHAR(225), b CHAR(226))
  before: advice mentions `b`
  after batch 13 migration: advice mentions `a` (regression)

Initial batch 13 omni port (PR #20312) normalized to first-wins
across both arms — preserved single-advice-per-statement cardinality
but flipped which column got reported for grouped ALTER ADD COLUMN.
Codex P3 caught post-merge.

Fix: lastViolation tracker on the ATAddColumn inner loop, emit once
after — same prescription as cumulative #15's "track lastViolation
local, emit once after the loop" applied to a different call-site
within the same advisor file. ATChangeColumn / ATModifyColumn are
single-shot (no inner loop) and unchanged.

Pinned with new grouped fixture: ALTER TABLE tech_book ADD COLUMN
(a char(225), b char(226)) expects advice mentioning `b`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(advisor/tidb): migrate partition + order-by-rand pair to omni AST (Phase 1.5 §1.5.3 batch 14)

Two clean-shape advisors with Recipe A (top-level type-switch + direct
field access). Step 2a clean — no type-byte territory.

- advisor_table_disallow_partition: CreateTableStmt.Partitions != nil
  + AlterTableStmt commands scanned for ATPartitionBy. Pingcap's
  AlterTablePartition enum (the REPARTITION form: ALTER TABLE t
  PARTITION BY ...) maps directly to omni's ATPartitionBy.
  Partition-management forms (ADD PARTITION, DROP PARTITION,
  TRUNCATE PARTITION, etc.) are NOT covered by either pingcap-tidb
  or the omni port — long-standing pre-omni scope preserved per
  invariant #10. Mysql analog has identical scope.

- advisor_insert_disallow_order_by_rand: InsertStmt.Select.OrderBy
  scanned for RAND() function calls. Preserves pingcap-tidb's
  break-on-first-match cardinality (one advice per statement, NOT
  per-item like the mysql analog) and the no-SetOp-recursion scope
  (UNION'd inserts skipped, matching long-standing pingcap-tidb
  behavior).

New helper omniIsRandFuncCall in utils.go: case-insensitive RAND()
detection via EqualFold on FuncCallExpr.Name (pingcap canonicalized
to lowercase via FnName.L; omni preserves user case). Unit tests in
utils_test.go cover uppercase/lowercase/titlecase, seed-arg variant,
nil expr, and negative cases (RANDOM, NOT_RAND).

Migration count: 37 → 39 omni-migrated; 10 remaining on pingcap
(8 migratable + 1 Class III blocked + utils.go bridge).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): expand fixture pins for partition + order-by-rand (Phase 1.5 §1.5.3 batch 14)

table_disallow_partition.yaml:
- HASH partition positive — verifies coverage across partition
  methods (not just RANGE).
- ALTER ... ADD PARTITION negative — pins the long-standing
  pre-omni scope (rule does NOT cover partition-management forms).
- ALTER ... DROP PARTITION negative — same scope pin.

statement_insert_disallow_order_by_rand.yaml:
- Uppercase RAND() positive — case-insensitivity contract on
  omniIsRandFuncCall (pingcap lowercased via FnName.L; omni
  preserves user case; EqualFold restores parity).
- INSERT ... SELECT ... ORDER BY id negative — distinguishes "any
  ORDER BY" from "ORDER BY RAND() specifically".
- INSERT ... SET form negative — cumulative #1 territory; ins.Select
  is nil and the rule short-circuits before checking OrderBy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(advisor/tidb): pin cumulative #24 — UNION outer-ORDER-BY UX improvement on insert_disallow_order_by_rand

Peer review caught a silent behavior change at the UNION boundary
that the initial batch 14 PR description elided.

Pre-omni pingcap-tidb's `InsertStmt.Select` is a `ResultSetNode`
interface. For `INSERT INTO t SELECT ... UNION SELECT ... ORDER BY
RAND()` the concrete type is `*ast.SetOprStmt`, NOT
`*ast.SelectStmt`. The pre-omni Enter's `insert.Select.(*ast.SelectStmt)`
cast failed and the whole check was silently skipped — pre-omni
rule did NOT flag UNION-result ORDER BY RAND despite the rule's
name and stated intent.

Omni's `InsertStmt.Select` is `*SelectStmt` (concrete pointer)
regardless of SetOp value; UNION'd inputs surface as
`SelectStmt{SetOp: SetOpUnion, Left: ..., Right: ..., OrderBy: [...]}`.
Direct access to `ins.Select.OrderBy` lands on the outer-UNION ORDER
BY list. Rule now fires — silent UX improvement matching the rule's
actual intent. Same UX-improvement shape as cumulative #18 ("is text"
advice content fix) and #21 (latent BLOB MODIFY false-positive).

Verified empirically by parsing the same input through both pingcap-
and omni-tidb parsers and inspecting `%T` / `SetOp` / `OrderBy[0]`.

Updates:
- Advisor comment now accurately describes outer-vs-inner UNION
  boundary distinction (outer-UNION OrderBy newly covered; per-arm
  parenthesized OrderBy remains uncovered — omni rejects that syntax
  in INSERT position, separate Phase 2 grammar gap).
- New positive fixture pin asserting the rule fires on the
  un-parenthesized UNION form.

NEW pre-batch audit axis added in plan-doc cumulative #24: when
pingcap source uses a narrow type-assertion on an interface-typed
field (e.g. `.(*ast.SelectStmt)` on `ResultSetNode`), the omni port
may newly reach inputs the old code accidentally filtered out — the
omni equivalent field is often concrete-typed, so direct access
bypasses the implicit filter. Audit every type-assertion: ask "does
this assertion narrow the input set?"

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>