Skip to content

Conversation

@h3n4l
Copy link
Member

@h3n4l h3n4l commented Dec 18, 2025

Summary

  • Fixed line position bugs in generateFinalAdvice/validateFinalState across multiple advisors

Line Position Bug Fix

When generateFinalAdvice/validateFinalState is called after walking all statements, r.baseLine is set to the last statement's baseline. However, AddAdvice adds r.baseLine to the position, which is wrong for tables defined in earlier statements.

Fixed by:

  1. Storing absolute line numbers (line + baseLine) at time of encounter
  2. Directly appending to adviceList in post-walk functions, bypassing AddAdvice's automatic baseLine offset

Affected advisors:

  • mssql/rule_table_require_pk.go
  • mssql/rule_table_no_foreign_key.go
  • pg/advisor_table_require_pk.go
  • pg/advisor_index_total_number_limit.go
  • snowflake/rule_table_require_pk.go

Test plan

  • All existing advisor tests pass
  • Added new test cases for multi-statement line position verification
  • Ran golangci-lint with no issues

🤖 Generated with Claude Code

h3n4l and others added 30 commits December 17, 2025 15:06
Document the bug where extractStatementText uses relative line numbers
from ANTLR but indexes into the full SQL text, and the solution to pass
ParsedStatements (with per-statement Text) instead of just ASTs.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Detailed task-by-task plan for migrating advisors to use per-statement
ParsedStatements.Text instead of extractStatementText() with line numbers.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Adds ParsedStatements []base.ParsedStatement to advisor.Context to provide
per-statement text alongside AST, fixing the statement text extraction bug.

Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Pass the full ParsedStatement slice to advisors, making per-statement
text available without line-number-based extraction.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Adds ParsedStatementInfo struct and getParsedStatements() function that
provides per-statement text directly, eliminating need for line-based
text extraction.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
…text

Use getParsedStatements() and per-statement Text field instead of
extractStatementText() with line numbers.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
No longer needed now that all advisors use per-statement Text directly.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Mark AST and Statements fields as deprecated in favor of ParsedStatements.
These fields are still populated for backward compatibility with non-PG
advisors, but new code should use ParsedStatements for accessing
per-statement text.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Empty statements (e.g., trailing semicolons) have nil AST. Skip them
instead of returning an error, matching the behavior of ExtractASTs.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
…uleContext

Add helper function that uses GetTextFromRuleContext to extract text
including hidden channel tokens (whitespace, comments). This provides
clean text extraction from token stream.

For statement text display, use strings.TrimSpace on statementText to
handle leading/trailing newlines while preserving full statement context.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update advisor_statement_where_required_select to store tokens and use
getTextFromTokens(tokens, ctx) to extract clean text from the token
stream. This provides more precise error messages showing the specific
context that violated the rule.

Update test expectation to reflect new behavior: subquery violations
now show the subquery text instead of the full statement.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
h3n4l and others added 13 commits December 17, 2025 17:44
…in error messages

Updated PostgreSQL advisors to use tokens + getTextFromTokens() pattern
for generating error messages with clean statement text.

Changes:
- Add tokens *antlr.CommonTokenStream field to rule structs
- Use getTextFromTokens(r.tokens, ctx) for error message content
- Remove unused statementText fields where applicable
- Update test expectations (no trailing semicolons in extracted text)

Files updated:
- advisor_table_no_fk.go
- advisor_table_disallow_partition.go
- advisor_migration_compatibility.go
- advisor_insert_must_specify_column.go
- advisor_insert_disallow_order_by_rand.go
- advisor_builtin_prior_backup_check.go
- advisor_naming_primary_key_convention.go (removed unused field)
- advisor_table_require_pk.go (fixed handleCreatestmt)

Files that keep statementText (for functional reasons):
- advisor_naming_fully_qualified.go (re-parses statement)
- advisor_insert_row_limit.go (EXPLAIN queries)
- advisor_statement_dml_dry_run.go (EXPLAIN queries)
- advisor_statement_affected_row_limit.go (EXPLAIN queries)
- advisor_statement_disallow_on_del_cascade.go (position conversion)
- advisor_statement_non_transactional.go (IsNonTransactionStatement)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
…acle, MSSQL, Snowflake

Add ParsedStatementInfo struct and getParsedStatements() helper function
to all database engines (MySQL, TiDB, Oracle, MSSQL, Snowflake) following
the PostgreSQL pattern.

This provides a unified way to access statement information including:
- Tree/Node: the parsed AST
- Tokens: for ANTLR-based engines (MySQL, Oracle, MSSQL, Snowflake)
- BaseLine: the line offset for error reporting
- Text: the original statement text

Each engine also gets getTextFromTokens() (ANTLR-based) or uses node.Text()
(TiDB) to extract clean text from specific AST contexts.

All new functions are marked with nolint:unused as they will be used
in Phase 2 when advisors are updated to use these helpers.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove AST field from advisor.Context struct
- Update getANTLRTree() in all engines (MySQL, PostgreSQL, Oracle,
  MSSQL, Snowflake, OceanBase, Redshift) to use ParsedStatements
- Update getTiDBNodes() to use ParsedStatements
- Remove checkContext.AST assignment in sql_review.go
- Remove fallback code and getParsedStatementsFromAST() functions
- Fix Oracle SplitSQL to properly set BaseLine field

This completes the migration from the deprecated AST field to
ParsedStatements, which provides per-statement text directly
without needing line-number-based extraction.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ntext

These fields were set but never read by any advisor. Advisors that
deal with charset/collation get this information from AST nodes or
column metadata instead.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace FullStatement with:
- StatementsTotalSize for size limit checks
- Direct use of ParsedStatements for prepareTransformation functions
- Simplified line-only position conversion

Changes:
- Add StatementsTotalSize field to Context for size checks
- Refactor prepareTransformation in PG, MSSQL, Oracle to accept
  []*base.ParseResult instead of re-parsing the statement string
- Simplify MySQL rule_online_migration to use ConvertANTLRLineToPosition
- Remove FullStatement field and its assignment

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove unused getParsedStatements, getTextFromTokens, and
ParsedStatementInfo from MSSQL, MySQL, Oracle, Snowflake, and TiDB.
These were scaffolding for a future migration that was superseded
by the current approach. PostgreSQL retains these as they are
actively used.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update TestOracleSplitMultiSQL to include BaseLine expectations
after the previous commit that added BaseLine calculation to
Oracle SplitSQL.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Update test data to expect newlines preserved in statement text instead
of spaces. This reflects the correct behavior where original SQL
formatting is maintained in error messages.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add a unified GetANTLRParseResults function in the advisor package
that extracts ANTLR parse results from the advisor context. All
ANTLR-based engines (MySQL, MSSQL, Oracle, Snowflake, PostgreSQL,
OceanBase) now use this shared implementation instead of duplicating
the same logic in each engine's utils.go.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
All ANTLR-based engines now directly use advisor.GetANTLRParseResults()
instead of individual wrapper functions. This eliminates code duplication.

- Remove utils.go files from MSSQL, Oracle, Snowflake, OceanBase (only had wrapper)
- Remove getANTLRTree wrapper from MySQL and PostgreSQL utils.go
- Update all advisor files to use advisor.GetANTLRParseResults() directly
- Redshift keeps its own getANTLRTree (different return type - single tree)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
…RParseResults

- Replace getParsedStatements() with advisor.GetANTLRParseResults()
- Remove ParsedStatementInfo struct (use base.ParseResult directly)
- Extract statement text on-demand using getTextFromTokens()
- Clean up utils.go imports

This makes PostgreSQL advisors consistent with other engines by:
1. Using the unified GetANTLRParseResults() from advisor package
2. Not storing statement text upfront, extracting it when needed

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
When generateFinalAdvice/validateFinalState is called after walking all
statements, r.baseLine is set to the last statement's baseline. However,
AddAdvice adds r.baseLine to the position, which is wrong for tables
defined in earlier statements.

Fix by:
1. Store absolute line numbers (line + baseLine) at time of encounter
2. Directly append to adviceList in generateFinalAdvice, bypassing
   AddAdvice's automatic baseLine offset

Affected files:
- mssql/rule_table_require_pk.go
- mssql/rule_table_no_foreign_key.go
- pg/advisor_table_require_pk.go

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
Fix the same line position bug in two more advisors:

1. snowflake/rule_table_require_pk.go
2. pg/advisor_index_total_number_limit.go

The bug: When generateFinalAdvice/GetAdviceList is called after walking
all statements, r.baseLine is set to the last statement's baseline.
However, the stored line numbers were relative, causing incorrect
positions for tables defined in earlier statements.

Fix by:
1. Store absolute line numbers (line + baseLine) at time of encounter
2. Directly append to adviceList in post-walk functions, bypassing
   AddAdvice's automatic baseLine offset

Also added test cases to verify correct multi-statement line positions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@h3n4l h3n4l requested a review from a team as a code owner December 18, 2025 08:16
@cla-bot cla-bot bot added the cla-signed label Dec 18, 2025
@h3n4l h3n4l enabled auto-merge (squash) December 18, 2025 08:23
@h3n4l h3n4l merged commit 59959b1 into main Dec 18, 2025
11 checks passed
@h3n4l h3n4l deleted the fix/advisor-statement-text-architecture branch December 18, 2025 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants