-
Notifications
You must be signed in to change notification settings - Fork 895
fix(advisor): correct line position and refactor statement text architecture #18529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+96
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
…tement text Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…nt text 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…t text Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…nt text Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
… text 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…atement text 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…t text 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ment text 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…atement text 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ment text 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]>
…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]>
…statement-text-architecture
d-bytebase
approved these changes
Dec 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
generateFinalAdvice/validateFinalStateacross multiple advisorsLine Position Bug Fix
When
generateFinalAdvice/validateFinalStateis called after walking all statements,r.baseLineis set to the last statement's baseline. However,AddAdviceaddsr.baseLineto the position, which is wrong for tables defined in earlier statements.Fixed by:
line + baseLine) at time of encounteradviceListin post-walk functions, bypassingAddAdvice's automatic baseLine offsetAffected advisors:
mssql/rule_table_require_pk.gomssql/rule_table_no_foreign_key.gopg/advisor_table_require_pk.gopg/advisor_index_total_number_limit.gosnowflake/rule_table_require_pk.goTest plan
golangci-lintwith no issues🤖 Generated with Claude Code