…T (Phase 1.5 §1.5.3 batch 19 — last migratable advisor)

The final IIb migration: 285→329 LOC expression-tree walker with
catalog-aware unique-WHERE detection. Mechanical port preserving
pre-omni tidb behavior exactly (invariant #7).

Helper translation table:

| Pre-omni | Omni |
|----------|------|
| ast.DDLNode (marker iface) | omniIsDDLStmt (24 concrete types) |
| *ast.UpdateStmt / *ast.DeleteStmt | *omniast.UpdateStmt / *omniast.DeleteStmt |
| update.TableRefs (*TableRefsClause) | update.Tables []TableExpr |
| *ast.BinaryOperationExpr / opcode.* | *omniast.BinaryExpr / BinOp* |
| *ast.ColumnNameExpr | *omniast.ColumnRef |
| ast.ValueExpr (marker iface) | omniIsConstantLit (8 literal types) |
| *ast.TableSource | (no direct equivalent — TableExpr interface) |
| *ast.TableName | *omniast.TableRef |
| *ast.Join | *omniast.JoinClause |
| *ast.SetOprStmt | *omniast.SelectStmt{SetOp != SetOpNone} |
| *ast.SubqueryExpr | *omniast.SubqueryExpr |
| node.Name.L (lowercase) | strings.ToLower(node.Column) |
| stmtNode.OriginTextPosition() | ostmt.FirstTokenLine() |

Audit axes applied:

- #7 (preserve pre-omni): mechanical port; pre-omni semantics
  preserved including the niche "single-table-with-unique-WHERE
  short-circuit" for >maxMixedDMLCount DML batches.
- #19 (case-sensitivity): pre-omni used .L for case-insensitive
  matching. Omni preserves user case via direct strings; explicit
  strings.ToLower / strings.EqualFold at lookup sites.
- #26 (UNION-root): pre-omni extractResultSetNode returned nil for
  *ast.SetOprStmt and *ast.SelectStmt arms. Omni's UNION-rooted
  derived tables (SelectStmt-with-SetOp) are reached only via
  SubqueryExpr at the TableExpr layer; the SubqueryExpr arm
  returns nil (preserved).
- #29 (filter-effect mismatch): isConstantLit enumerates 8 omni
  literal types — pre-omni's ast.ValueExpr interface check.
  Unit-tested for completeness.

NEW unit tests (utils_test.go):
- TestOmniIsConstantLit: 8 literal types + ColumnRef/BinaryExpr/nil
  negatives
- TestOmniIsDDLStmt: 12 DDL types + UPDATE/DELETE/INSERT/SELECT/nil
  negatives

These serve as the primary regression surface for the migration —
the advisor's main behavior is gated on EnablePriorBackup which the
standard test fixture driver gates on. The orphan fixture
test/statement_prior_backup_check.yaml is left untouched (rule was
renamed STATEMENT_→BUILTIN_ at some point; fixture file wasn't
renamed; never wired into tidb_rules_test.go). Future ticket to
properly test-cover this advisor.

Divergence from mysql analog: mysql's prior_backup_check has been
modernized to use mysqlparser.ExtractTables + per-table DML-mixing
detection (~120 LOC). The tidb pre-omni logic is the OLDER shape
(unique-WHERE short-circuit + single mixed-DDL emission). This
migration preserves the older tidb logic exactly per invariant #7.
Future feature ticket: align tidb prior_backup_check with mysql's
modernized version (out of Phase 1.5 scope).

Migration count: 46 → 47 omni-migrated. **All migratable
advisors complete.** 2 remaining: 1 Class III (dml_dry_run,
blocked on omni BATCH grammar) + utils.go bridge. Next: PR 1.5.N+1
dispatcher flip per plan §1.5.N+1.

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

…ligned modernized logic (batch 19 reshape)

Pre-merge peer review challenged the initial mechanical port:

> "If we're implementing it, why don't just go straight for the
>  modernized version if it's more correct?"

Original framing (invariant #7 by-the-book) was overly conservative:
the pre-omni tidb logic wasn't being tested (orphan fixture, not in
test runner), so "preserving" untested behavior preserves unknowns.
Modernizing to mysql's shape is the right call when:
  1. Phase 1.5 closes after this batch — deferring leaves tidb stale.
  2. Mysql's per-table DML-type mixing is more semantically accurate
     for backup feasibility than the pre-omni count-cap heuristic.
  3. The migration is the right place to align; a future-ticket-punt
     is an anti-pattern.

Reshape changes:

- Replaced unique-WHERE machinery (omniUpdateForOneTableWithUnique,
  omniHasUniqueInWhereClause, omniExtractColumnsInEqualCondition,
  omniIsConstantLit) with per-table DML-type mixing detection
  matching mysql's modernized shape.
- Added size cap check (StatementsTotalSize > MaxSheetCheckSize)
  matching mysql.
- Fixed omniIsDDLStmt enumeration per peer's verified pingcap
  DDLNode set (22 implementers in tidb v8.5.5; 18 mappable to omni
  today, 4 deferred Tier-4 grammar). Initial PR had 14 false-
  positives (User/Role/Function/Trigger/Event/Tablespace/Server
  stmts + DropView + AlterDatabase — pingcap doesn't classify these
  as DDL) and 5 false-negatives (OptimizeTable, RepairTable,
  ResourceGroup trio).

Behavior matrix (vs pre-omni tidb):

  Input                                    pre-omni       new
  -----------------------------------     ----------     ----------
  UPDATE t × 6 (no unique-WHERE)          fires          skips
  UPDATE t WHERE id=5 × 10                skips          skips
  UPDATE t; DELETE FROM t                 skips          FIRES (new)
  Statements > MaxSheetCheckSize          skips          FIRES (new)
  CREATE TABLE; UPDATE                    fires          fires
  Missing bbdataarchive db                fires          fires

Test coverage closes peer's third concern:

- New TestTiDBPriorBackupCheckAdvisor (6 cases) modeled on mysql's
  TestMariaDBPriorBackupCheckAdvisor pattern: standalone test
  passing ListDatabaseNamesFunc + EnablePriorBackup context
  directly. Covers all 4 advice paths (DDL mixing, missing backup
  db, per-table DML mixing, size cap exceeded) + 2 negative cases
  (single UPDATE, UPDATE+DELETE on different tables).
- TestOmniIsDDLStmt expanded from 16 to 36 cases — adds 18 positive
  (full pingcap DDLNode set restricted to omni) + 18 negative
  (DropView, AlterDatabase, User/Role/Function/Trigger/Event/
  Tablespace/Server stmts + DML + nil). The NEGATIVE cases are
  load-bearing — without them a future refactor could re-add
  over-inclusions silently.

Removed:
- Orphan fixture file test/statement_prior_backup_check.yaml. The
  rule type was renamed STATEMENT_→BUILTIN_ at some point; fixture
  file name didn't follow; content was stale (mentioned "Found N
  DELETE, M UPDATE" — neither pre-omni nor mysql analog phrasing).
  Test coverage now comes from the dedicated TestTiDBPriorBackupCheckAdvisor
  function (mysql pattern), not the fixture driver.

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

… in behavior matrix (cumulative #30)

Peer review caught: the reshape commit changed the advice code for
"Need database bbdataarchive ... does not exist" from
code.BuiltinPriorBackupCheck → code.DatabaseNotExists (mysql-aligned),
but the file-level behavior matrix listed this row as "pre: fires →
new: fires" without noting the code change. The numeric code is a
public-API surface — downstream consumers (UI filtering, automation
parsing) may key off the value; a contract change at this boundary
deserves explicit acknowledgment.

This commit makes the delta explicit in the file-level docstring.
No runtime change. Plan-doc cumulative #30 (added in same batch)
captures the full receipt set including the new invariant #7
caveat that permits this kind of mysql-aligned modernization when
pre-omni behavior is untested.

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

…t DB (Codex P1 + P2, cumulative #30 Codex-fix-1b / fix-2 revised)

Two follow-on Codex catches on the previous Codex-fix-1 / fix-2 work:

P1 (Codex-fix-1b): aliased single-table UPDATE bypass.

`omniCollectTableAliases` stored TWO lookup entries per aliased
TableRef (alias + bare name → same base). So `UPDATE tech_book AS t
SET id = 1` produced `len(aliasMap) == 2`. The unqualified-column
branch in `omniExtractUpdateTargets` gated on `len(aliasMap) == 1`,
which failed for aliased single-table UPDATEs → UPDATE target was
dropped → following DELETE on same table not flagged as mixed-DML.
False-negative on the safety gate.

Fix: track distinct base tables separately from lookup-map entries.
Restructured the helper return type from
`map[string]priorBackupTable` to a new `updateTableAliasMap` struct
with two fields: `lookup` (alias-or-name → base; for qualified
resolution) and `distinctBases` (deduplicated FROM-clause base
tables; for single-target detection). The "single-target" gate now
checks `len(m.distinctBases) == 1` — correct regardless of alias
multiplicity.

P2 (Codex-fix-2 revised): qualified vs unqualified split when
CurrentDatabase is empty.

Previous Codex-fix-2 used `checkCtx.CurrentDatabase` as the default
database for unqualified references. But CurrentDatabase is NOT
populated in some review paths — backend/runner/plancheck/
statement_advise_executor.go:168-180 constructs advisor.Context
without setting it. So qualified `db1.tech_book` vs unqualified
`tech_book` split into different buckets (`db1.tech_book` vs
`.tech_book`) and mixed-DML on the same table was missed.

Fix: use `checkCtx.DBSchema.GetName()` as the primary default
(mysql analog pattern via `prepareTransformation(checkCtx.DBSchema.Name, ...)`).
The schema being checked IS the implicit database for unqualified
references; DBSchema is reliably populated across review paths
including plancheck. Fall back to CurrentDatabase, then empty
string, in that order.

Resolution moved to extraction time (omniExtractDMLTables +
omniBuildTableAliasMap both take defaultDB), so priorBackupTable
records have populated database fields from the start. The
grouping-key loop simplified: no longer needs runtime fallback.

Regression-direction pins added:

- "aliased single-table UPDATE with unqualified SET (Codex-fix-1b)":
  `UPDATE tech_book AS t SET id = 1; DELETE FROM tech_book ...`
  → MUST fire mixed-DML (single base table despite 2 lookup keys).

- "qualified vs unqualified same-table mixing (Codex-fix-2 revised)":
  `UPDATE test.tech_book ...; DELETE FROM tech_book ...`
  → MUST fire (DBSchema.Name default resolves unqualified ref).

Test count: 8 → 10 cases.

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

…t (Codex P1 ×2, cumulative #30 Codex-fix-1c / fix-1d)

Two new Codex P1 catches following the algorithm-corrections wave:

P1 (Codex-fix-1c): DROP VIEW + ALTER DATABASE classified as non-DDL.

Peer's earlier DDLNode analysis counted compile-time
`_ DDLNode = &XxxStmt{}` declarations in pingcap's ast/ddl.go and
concluded DropViewStmt + AlterDatabaseStmt were absent. Empirical
verification (parse + type-assert .(ast.DDLNode)) reveals:

  - `DROP VIEW v` parses as *ast.DropTableStmt (already in our DDL
    list) in pingcap, but omni splits to *ast.DropViewStmt — my
    exclusion of DropViewStmt was the actual regression.
  - `ALTER DATABASE foo COLLATE = ...` has isDDLNode=true via the
    ddlNode struct embedding (ast/base.go:81-88) which the
    compile-time grep missed.

Both added to omniIsDDLStmt; unit test moves both from negative to
positive lists with #30 Codex-fix-1c comment lineage.

P1 (Codex-fix-1d): schema-qualified UPDATE target misattribution.

For `UPDATE db1.tech_book JOIN db2.tech_book ON ... SET
db1.tech_book.id = 1; DELETE FROM db1.tech_book ...`, my
omniBuildTableAliasMap keyed lookup by bare-name only. Two
TableRefs with Name="tech_book" overwrote each other in the lookup
map (db2.tech_book wins). The `SET db1.tech_book.id = 1` resolution
ignored Column.Schema and matched the bare name → got db2.tech_book
(WRONG). Following DELETE on db1.tech_book recorded as DELETE-only;
UPDATE went to db2 → no mixing detected → false-negative on the
safety gate.

Fix: restructured updateTableAliasMap into four resolution paths:

  - bySchemaName: "schema.name" key → base. Used for Column.Schema
    + Column.Table qualified SET LHS.
  - byAlias: alias → base. Aliases are query-unique; unambiguous.
  - byBareName: bare-name → list of bases. Multiple bases on the
    same bare name → ambiguous reference, skip.
  - distinctBases: deduplicated FROM-clause bases for single-target
    fallback (existing Codex-fix-1b semantic preserved).

Resolution dispatch in omniExtractUpdateTargets:
  1. Column.Schema != "" → bySchemaName (disambiguates).
  2. Column.Schema == "" but Column.Table != "" → byAlias first
     (unambiguous), then byBareName (single match only).
  3. Both empty → single-target fallback via distinctBases.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
test count: 10 → 13):

  - "DROP VIEW + UPDATE fires mixed DDL+DML (Codex-fix-1c)"
  - "ALTER DATABASE + UPDATE fires mixed DDL+DML (Codex-fix-1c)"
  - "schema-qualified UPDATE target disambiguation (Codex-fix-1d)":
    full Codex repro, asserts mixed-DML fires on db1.tech_book.

TestOmniIsDDLStmt: 36 → 36 cases (DropViewStmt + AlterDatabaseStmt
moved from negative to positive; net count unchanged).

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

… in multi-table UPDATE (Codex P2, cumulative #30 Codex-fix-1e)

The 7th Codex round on this advisor; closes the last remaining
false-negative on the per-table DML-mixing safety gate.

The case: `UPDATE t1 JOIN t2 ... SET status = 1` where `status`
belongs to only one of the joined tables. Without schema-aware
resolution, my prior code (post-Codex-fix-1b/1d) dropped multi-
target unqualified-SET attribution → no UPDATE target recorded
→ following DELETE on the implicit-target table wasn't flagged
as mixed-DML.

The shipping-the-full-modernization decision (cf. cumulative #30
invariant #7 caveat — "modernization is allowed when properly
gated") commits us to mysql-analog correctness. mysql gets this
right via mysqlparser.ExtractTables (parser-level column→table
resolver with full MySQL grammar knowledge). Tidb omni doesn't
have an equivalent helper; this fix uses the advisor's catalog
metadata (checkCtx.DBSchema) instead.

New helper omniResolveUnqualifiedSETColumn walks dbMetadata for
each candidate base in distinctBases and returns the single base
whose schema owns the column. Multiple matches (column exists on
multiple joined tables) → skip (MySQL itself errors on ambiguous
unqualified refs at execution time; safety gate stays silent
rather than guess). Zero matches → skip (column not in catalog;
either truly missing or unknown schema → can't resolve).

Resolution dispatch in omniExtractUpdateTargets (unqualified
branch) now:
  1. Single-target UPDATE (len(distinctBases) == 1) → use that
     base (existing Codex-fix-1b semantic).
  2. Multi-target UPDATE → omniResolveUnqualifiedSETColumn walks
     catalog → if exactly one base owns the column, use it.
  3. Otherwise → skip (preserved conservative default).

Schema-match policy: bases pointing at databases other than
dbMetadata.Name are skipped — we have no catalog info for them.
Common single-database multi-table UPDATEs are fully covered;
cross-database UPDATEs with unqualified SET fall through to skip
(rare; out of scope without multi-database catalog access).

Regression-direction pin (TestTiDBPriorBackupCheckAdvisor case
count: 13 → 14):
  - "unqualified SET in multi-table UPDATE resolves via catalog
    (Codex-fix-1e)": `UPDATE tech_book INNER JOIN orders SET
    customer_name = 'x'; DELETE FROM orders WHERE order_id = 5`.
    customer_name is in MockMySQLDatabase under orders only →
    catalog walk resolves → UPDATE attributed to orders → DELETE
    on orders → mixed-DML fires.

Phase 2 ticket (out of scope here): factor out a reusable
tidb-side ExtractTables analog so future advisors can share the
column-to-table resolution machinery instead of each advisor
walking dbMetadata directly.

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

…-grammar coverage (Codex P1, cumulative #30 Codex-fix-1f)

Codex caught: omni's parser rejects Tier-4-deferred DDL grammar
(CREATE/ALTER/DROP SEQUENCE, FLASHBACK DATABASE/TABLE). Per
invariant #2, `getTiDBOmniNodes` soft-fails on omni-rejected stmts.
For builtin_prior_backup_check — a safety gate — this silently
drops DDL classification on these valid tidb statements:

  CREATE SEQUENCE seq1 START 1; UPDATE bar SET x=1;
  FLASHBACK TABLE foo; DELETE FROM bar WHERE id=5;

Pre-omni: pingcap parses both, classifies as DDL via DDLNode
interface → mixed-DDL+DML advice fires.
Post-omni: omni rejects SEQUENCE/FLASHBACK → soft-fail skips →
advisor sees only UPDATE/DELETE → no DDL detected → safety gate
silently misses.

Empirical verification (parse-test through omni): all five inputs
(CREATE/ALTER/DROP SEQUENCE, FLASHBACK DATABASE, FLASHBACK TABLE)
fail omni parsing with syntax error.

Fix: DDL detection switches to the `getTiDBNodes` (pingcap) path.
Pingcap's DDLNode interface (via the ddlNode struct embedding in
ast/base.go:81-88) catches all 22 implementers including Tier-4-
deferred grammar. The advisor still uses `getTiDBOmniNodes` for
per-table DML mixing detection (where omni AST shape is required);
DDL detection sidesteps omni's grammar gaps.

Architectural note: this dual-path approach mirrors mysql's analog,
which uses mysqlparser.ExtractTables alongside the omni AST.
Specialized advisors that need both full-grammar coverage AND
omni-specific AST shape will need both paths until omni grammar
parity catches up.

Side effects:

- omniIsDDLStmt helper REMOVED. Eight rounds of Codex catches on
  omni-only DDL enumeration (cumulative #30 Codex-fix-1c through 1e)
  demonstrated the brittle-enumeration anti-pattern; pingcap's
  authoritative DDLNode interface eliminates the maintenance burden.
- TestOmniIsDDLStmt REMOVED. The helper it tested is gone. End-to-end
  coverage of the DDL-detection path moves to TestTiDBPriorBackupCheckAdvisor.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
case count: 14 → 16):

  - "CREATE SEQUENCE + UPDATE fires mixed DDL+DML via pingcap path
    (Codex-fix-1f)": `CREATE SEQUENCE seq1 ...; UPDATE tech_book ...;`
    → MUST fire.
  - "FLASHBACK TABLE + DELETE fires mixed DDL+DML via pingcap path
    (Codex-fix-1f)": `FLASHBACK TABLE tech_book TO tech_book_old;
    DELETE FROM orders ...;` → MUST fire.

Both pins exercise the new pingcap-DDLNode resolution path.

Cumulative #30 algorithm-corrections sub-fix lineage now spans
eight sub-fixes:
  1 — SET-clause-based target extraction
  1b — distinct-base gating
  1c — DDL classification (DropView + AlterDatabase)
  1d — schema-qualified UPDATE target lookup
  1e — schema-aware unqualified-SET resolution
  1f — DDL detection via pingcap DDLNode (drops omniIsDDLStmt)
  2 — case-insensitive grouping key
  2-revised — DBSchema.Name default-DB

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

…ex P1, cumulative #30 Codex-fix-1g)

Codex caught: my reshape removed the pre-omni >5 DML feasibility
gate, but the backup transformer at parser/tidb/backup.go:96-110
still has that constraint. Inputs like 6 UPDATEs across different
tables now pass the advisor but fail at runtime backup generation
with "prior backup cannot handle statements on different tables
more than 5".

The reshape framing ("pre-omni count-cap is stale legacy logic")
was wrong here. The count-cap matches the transformer's CURRENT
behavior:

  generateSQL:
    if len(statementInfoList) <= maxMixedDMLCount {  // 5
        return generateSQLForMixedDML(...)  // OK for ≤5
    }
    return generateSQLForSingleTable(...)  // requires single table

  generateSQLForSingleTable:
    for _, item := range statementInfoList {
        if !equalTable(table, item.table) {
            return nil, errors.Errorf("prior backup cannot handle
              statements on different tables more than %d", ...)
        }
    }

Fix: reinstate the count-cap gate after per-table mixing detection.
When `len(dmlRefs) > maxMixedDMLCount`, compute distinct base
tables across all DML references; if more than one, fire advice
matching the transformer's reject condition.

Single-table batches above the threshold are intentionally allowed
— the transformer's single-table path handles them successfully.

Constant `maxMixedDMLCount = 5` reinstated at advisor level (matches
parser/tidb/backup.go:23). Synced via comment for maintenance.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
case count: 16 → 18):

  - "6+ DML across multiple tables fires count-cap (Codex-fix-1g)":
    6 UPDATEs across tech_book + orders → MUST fire.
  - "6+ DML on single table — no count-cap advice (Codex-fix-1g)":
    6 UPDATEs on tech_book only → MUST NOT fire count-cap (single-
    table path handles).

Cumulative #30 sub-fix lineage updated to nine sub-fixes:
  1 — SET-clause-based target extraction
  1b — distinct-base gating
  1c — DDL classification (DropView + AlterDatabase)
  1d — schema-qualified UPDATE target lookup
  1e — schema-aware unqualified-SET resolution
  1f — DDL detection via pingcap DDLNode
  1g — count-cap with multi-table gate (matches transformer)
  2 — case-insensitive grouping key
  2-revised — DBSchema.Name default-DB

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

…fied SET (Codex P1, cumulative #30 Codex-fix-1h)

Codex caught: my `omniResolveUnqualifiedSETColumn` returned nil on
multi-match or zero-match cases. The TiDB backup transformer at
parser/tidb/backup.go:539-576's `resolveUnqualifiedColumns` does
the opposite — falls back to all joined tables on either ambiguity.
Net: my advisor approved inputs the transformer rejected at runtime
(same gap shape as Codex-fix-1g).

Empirical verification of transformer behavior:

  // No metadata → fallback to ALL singleTables
  if schema == nil { ... fall back ... }

  for each col:
      resolved := false
      for each singleTable:
          if tableMetadata.GetColumn(col) != nil {
              result[aliasOrName] = true
              resolved = true
          }
      }
      if !resolved {
          // Column not found anywhere → fall back to ALL
          ... fall back ...
      }

Three divergent paths now mirrored:

  - No metadata available → return ALL distinctBases
  - Column found in N≥1 distinct bases → return THOSE N
    (single-match preserved as Codex-fix-1e subset)
  - Column not found in ANY base → return ALL distinctBases

Resolution dispatch updated: any-match attributes via `add(t)`
loop instead of `len == 1` gate.

Regression-direction pins added (TestTiDBPriorBackupCheckAdvisor
case count: 18 → 20):

  - "multi-match unqualified SET attributes to all (Codex-fix-1h)":
    `UPDATE tech_book JOIN orders SET name = 'x'; DELETE FROM
    tech_book` — `name` exists on BOTH tables; new fallback
    attributes UPDATE to both → tech_book has UPDATE+DELETE →
    mixed-DML fires.
  - "zero-match unqualified SET falls back to all (Codex-fix-1h
    count-cap path)": three multi-table UPDATEs with non-existent
    column → each contributes 2 distinctBases entries; total >5
    DML refs across distinct tables → count-cap fires.

Codex-fix-1e single-match case remains correct (single-element
return from the helper).

Cumulative #30 algorithm-corrections lineage now spans 10
sub-fixes; the dual-path architecture is settled.

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