…AST (Phase 1.5 §1.5.3 batch 3)

First Class IIa batch — advisors that consume both AST nodes AND the
statement's source text for advice content. Four rules migrated:

  - advisor_stmt_disallow_commit.go    (STATEMENT_DISALLOW_COMMIT)
  - advisor_stmt_no_select_all.go      (STATEMENT_SELECT_NO_SELECT_ALL)
  - advisor_stmt_disallow_order_by.go  (STATEMENT_DISALLOW_ORDER_BY)
  - advisor_stmt_disallow_limit.go     (STATEMENT_DISALLOW_LIMIT)

Three use Recipe A (top-level type switch). One — no_select_all —
uses Recipe B (omniast.Walk) because the wildcard can appear in
nested subqueries; the existing fixture exercises this with
'SELECT a FROM (SELECT * FROM t1 JOIN t2) t' and a top-level switch
silently skips the inner SELECT.

Migration progress: 14 of 51 advisors on omni AST.

New shared helper:
  - OmniStmt.TrimmedText() in utils.go — returns Text with surrounding
    whitespace removed, for embedding into advice content. Multi-
    statement input passes leading newlines through Statement.Text;
    pingcap's node.Text() returned the trimmed form.

Notable mappings:
  pingcap UpdateStmt/DeleteStmt   omni
    Order != nil          →       len(OrderBy) > 0
    Limit != nil          →       Limit != nil
  pingcap InsertStmt              omni
    Select interface (SelectStmt | SetOprStmt) → SelectStmt with SetOp field;
                                    omni unifies both forms, single branch covers them.
  pingcap SelectStmt              omni
    Fields.Fields[].WildCard →    *StarExpr in TargetList[]
  pingcap CommitStmt              omni
    *ast.CommitStmt        →      *ast.CommitStmt (same)

Pre-batch checklist:
  1. Coupling scan: clean — useLimit was local to disallow_limit.go;
     inlined as insertUsesLimit during migration.
  2. Shape audit: pingcap *ast.SetOprStmt collapses to omni's
     *SelectStmt with SetOp field (similar to migration_compatibility's
     ConstrUnique unification). Fixture exercises the inner-subquery
     wildcard case which forced Recipe B for no_select_all.
  3. Yaml fixture coverage: TestTiDBRules green for all 4 rules.
  4. Pingcap-import quarantine: clean for migrated files.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  go test -count=1 ./backend/plugin/parser/tidb/...    # green
  go build ...                                         # green
  golangci-lint run ...                                # 0 issues

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

…overage

Round-1 review feedback on PR #20199.

#1 Line-number parity for no_select_all. The pre-migration pingcap
advisor set checker.line once per top-level statement, so every
wildcard hit (including nested-subquery hits) reported the OUTER
statement's line. The omni Recipe B port emitted per-match — using
the inner SelectStmt's Loc — which was an arguably-better behavioral
change but unacknowledged.

Reverted to outer-statement line for migration discipline: capture
ostmt.AbsoluteLine(0) once per statement and reuse for all advices.
Improving advice line precision is a follow-up decision, not a
side-effect of parser migration.

#2 UNION fixture coverage. omni's Walk recurses into SelectStmt.Left
and .Right (verified in walk_generated.go:756-762), so Recipe B
correctly visits UNION arm subqueries. Added three fixture cases to
pin the contract:

  - SELECT * FROM t1 UNION SELECT a FROM t2  → flagged (left arm)
  - SELECT a FROM t1 UNION SELECT * FROM t2  → flagged (right arm)
  - SELECT a FROM t1 UNION SELECT b FROM t2  → no advice (control)

#2 (cont) insertUsesLimit comment. Added documentation that only
the outermost LIMIT is checked — a LIMIT inside a parenthesized
UNION arm lives on Left.Limit, not on the outer .Limit, and was
not flagged by the pre-migration advisor either. Pingcap parity
preserved, behavior intentionally not extended.

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

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

Codex P2: 4 findings of line-number regression for inputs with leading
trivia (whitespace + comments). Empirically verified:

  Input: "\n-- cmt\nSTMT;"
    pingcap (main):    line 2
    omni (this PR):    line 1   ← regression

Codex's claimed direction was reversed for 3 of the 4 cases (claimed
line 3, actual line 1) but the regression itself is real for all 4.

Root cause: pingcap's lexer strips leading whitespace but keeps
comments in node.Text(), so its OriginTextPosition points at the line
of the first comment OR keyword. Omni's Loc.Start is 0 (start of
segment) for top-level statements; AbsoluteLine(0) gives BaseLine+1.

Fix: new OmniStmt.FirstTokenLine() helper finds the line of the first
non-whitespace character in s.Text. Matches pingcap behavior for the
common cases:

  | Input                            | pingcap | FirstTokenLine |
  |----------------------------------|---------|----------------|
  | "COMMIT;"                        | 1       | 1              |
  | "\n-- cmt\nCOMMIT;"              | 2       | 2              |
  | "\n/* block */\nCOMMIT;"         | 2       | 2              |

One edge case diverges: "   \n  COMMIT;" (leading whitespace, no
comments) — pingcap reports 1, FirstTokenLine reports 2. pingcap's
lexer preserves whitespace-only leading text in node.Text() so its
leadingNewlinesStripped is 0 here. Real SQL rarely has this shape;
both reported lines reasonably point at the statement.

All 4 batch-3 advisors (commit, no_select_all, disallow_order_by,
disallow_limit) updated to use FirstTokenLine() instead of
AbsoluteLine(0) or AbsoluteLine(n.Loc.Start).

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

Empirical line-parity test across 6 input shapes: 5/6 match pingcap;
the 1 mismatch is the contrived leading-whitespace-only case
documented above.

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

Codex P2: targetListHasStar only matched *ast.StarExpr, missing the
qualified-wildcard form. Empirically verified omni's TargetList shape:

  SELECT *      → *ast.StarExpr
  SELECT t.*    → *ast.ColumnRef with Star: true (NOT a StarExpr)

Pre-migration pingcap path used field.WildCard != nil, which was set
for both bare and qualified wildcards. omni splits them into two node
types; targetListHasStar must check both.

Fix: extend the type switch to also match *ast.ColumnRef with
Star == true.

Sanity-checked by reverting: TestTiDBRules fails on the new
"SELECT t.* FROM t" fixture case; restored fix passes.

Fixture additions:
  - SELECT t.* FROM t          → flagged (qualified)
  - SELECT a, t.* FROM t       → flagged (mixed with column)
  - SELECT a, t.b FROM t       → no advice (control: qualified non-star)

Validation:
  go test -count=1 ./backend/plugin/advisor/tidb/...   # green
  go test -count=1 ./backend/plugin/advisor/mysql/...  # green
  golangci-lint run ...                                # 0 issues

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