docs: add AGENTS.md and bring the parsers up to its parser contract - #435
Merged
Merged
Conversation
…ates Root AGENTS.md is the instruction source of truth; CLAUDE.md imports it. It records the repository shape and fork map, the engine contract, how the parser is called from Bytebase and from omni's own packages, the deviations that do not meet that contract yet, and the test and verification gates CI actually runs. pg/parser/CLAUDE.md becomes pg/parser/AGENTS.md with its stale Docker-skip section corrected; redshift/parser keeps a pointer instead of a copy. docs/AGENTS.md marks plans, specs, and scenarios as history. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… parse errors Port the doris/starrocks #402 discipline. Parse now drops a statement whose prefix parsed but left tokens behind, instead of returning the truncated node next to its error; ParseBestEffort keeps the prefix and reports no error for the tail. Strict trino Parse also drains a rejected segment so later lex errors surface, and promotes lex errors from segments Split dropped as empty. analysis fails closed to match. GetQuerySpan and ClassifySQL return the first parse error instead of analyzing a partial tree: `SELECT a FROM t )))` used to analyze as the prefix, so masking and lineage saw less than the engine would execute. trino re-parses subquery placeholder bodies and now records a failure there too, with positions shifted into the outer statement through the new SubqueryExpr.TextStart and accumulated across nesting; empty, comment-only, multi-statement, and non-query bodies fail the span (all four rejected by Trino 482, checked against the oracle). An unparseable view definition stays an opaque AccessTable, since that text is one Trino already accepted. googlesql diagnostics reads Parse so trailing junk is still diagnosed; trino completion retries its empty-select-list fill when the caret-patched statement does not parse, since analysis no longer hands it a partial span to inspect. Verification: strict-trailing tests and corpus canaries for both parsers (trino accept corpora, googlesql official corpus), fail-closed analysis tests, full trino and googlesql trees against the Trino container and Spanner emulator, and bytebase's trino, googlesql, spanner, and bigquery adapter packages against this checkout. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…spaces oracle.Parse kept positions absolute by prefixing every segment with ByteStart spaces before parsing it, so a script of n statements copied and lexed O(n^2) bytes: a 2000-statement script took 775 ms and 462 MB of allocations to parse. The lexer now takes a range: NewLexerRange(input, start, end) lexes input[start:end] in place, and parser.ParseRange(source, start, end) parses one segment with every Loc and error Position already an offset into source. Parse(sql) is ParseRange(sql, 0, len(sql)). The wrapped procedure body scan stops at the range end instead of the end of the string, so a later segment never leaks into WrappedSource. The same script now parses in 12 ms and 4 MB. Tests pin absolute Locs and error positions for a middle segment, parity with the padded parse across SQL, PL/SQL, and WRAPPED segments, and clamped bounds. AGENTS.md names ParseRange as the way to parse a segment in place and drops the deviation. Verified with the full oracle tree and bytebase's plsql, advisor/oracle, and schema/oracle packages against this checkout. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n columns in code points The top-level Statement.Start and Statement.End of each engine counted columns in bytes, each through its own copy of buildLineIndex and offsetToPosition, while review.Position counts them in code points, the units of Bytebase's Position. A statement after a non-ASCII literal on the same line therefore reported a column Bytebase would place too far right. Every top-level package now converts through review.Index and a four-line positionAt; the seven private copies are gone. elasticsearch's Statement positions already counted code points through its splitter and only the doc claimed bytes; its SyntaxError positions keep the 0-based legacy convention Diagnose pins. The mssql precision matrix case that pinned byte columns now pins code points, and pg and mssql gain a multi-byte column test. Bytebase reads only ByteStart and ByteEnd from these statements; its pg, tsql, plsql, redshift, cassandra, cosmosdb, mongodb, and elasticsearch adapters pass against this checkout. AGENTS.md drops the deviation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…sdb): unify ParseError to Message and Position Every engine's ParseError now carries Message and Position, the byte offset into the parsed text, so a consumer converts it to a line and column the same way for every engine. snowflake, trino, googlesql, doris, starrocks, and partiql replace Loc and Msg with Position, End, and Message; cosmosdb renames Pos; cassandra splits Loc into Position and End beside its Line, Column, and Near. trino, googlesql, doris, and starrocks Parse return error instead of []ParseError. A failure is a ParseErrors list that unwraps to each *ParseError, so errors.As reaches the first one like every other engine while Diagnose still reports a script's every failure through AllErrors; FirstError serves analysis, which reports one position. The File still holds the statements that parsed. The parser test corpora of those four engines keep their error-list assertions through a parseForTest shim that calls Parse. Literals were rewritten mechanically and readers compile-driven, with a contract test per engine pinning nil on success, errors.As, the first Position, AllErrors, and the kept File. Verified with the full trees of all eight engines. AGENTS.md drops the deviation and names ParseErrors, AllErrors, and FirstError; bytebase's adapters migrate when its pin bumps. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3b75eb009
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…oes not parse analysis now fails closed on any parse error, and a statement being edited often has an unfinished fragment away from the caret: with the caret in the select list of `SELECT | FROM customer WHERE custkey =`, the placeholder fixes the select list but the WHERE still fails, the empty-select fill changes nothing, and completion offered no columns. Before the strict parse the tolerant span kept the FROM relation. When both analyses fail, completion now reads the FROM/JOIN relations (with aliases and comma-continued FROM lists) and the WITH names from the tokens, the lexer-based fallback the parser contract asks of every fragment consumer. It over-approximates on purpose: an extra table costs a spurious candidate, a missing one hides every column. Reported by Codex review on #435. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d33f81237
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… qualified tables in the completion fallback The placeholder scan captures an expression-embedded subquery's body as raw text between balanced parentheses without parsing it, so strict Parse accepted `SELECT (SELECT 1 FROM t a b)` with a nil error while Trino rejects it. Strict mode now parses every placeholder body it created: a body must parse and be exactly one query, and an empty, comment-only, multi-statement, or non-query body fails the statement like any other syntax error, with positions in outer-statement coordinates. The parser records placeholders as it creates them and restore truncates the list, so an abandoned speculative parse validates nothing. ParseBestEffort stays tolerant. The completion fallback dropped any relation whose last name part matched a CTE, so `WITH customer AS (...) SELECT | FROM prod.customer` lost its catalog columns. Only an unqualified name can reference a CTE; qualified relations are kept. Both reported by Codex review on #435. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 758a6df182
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…; accept keyword aliases in the completion fallback googlesql parseSingle returned the outer node when fillSubqueries had recorded an error for an embedded query (`SELECT (SELECT 1 FROM t a b)`), so strict Parse listed the error and still kept the rejected SELECT in File.Stmts. Strict mode now drops the node whenever the segment accumulated an error; ParseBestEffort keeps the partial tree. trino's token fallback read only identifier aliases, so `FROM customer comment` (comment is a non-reserved keyword Trino accepts as an alias) recorded no alias and `comment.` completion resolved nothing. A non-reserved keyword is now an alias when nothing operand-like follows it, so LIMIT 1 and FETCH FIRST stay clauses; reserved words never are. Both reported by Codex review on #435. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faef33ca82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ery body and drop nodes on lex errors in strict Parse Three more ways a strict Parse returned a node the engine would reject, all reported by Codex review on #435: - trino's SHOW STATS FOR (query) captured its body as raw text outside the placeholder list, so `SHOW STATS FOR (SELECT 1 FROM t a b)` parsed clean. The placeholder list is now a list of raw query bodies that both capture sites register, and strict mode validates all of it. - doris and starrocks captured expression subqueries, IN/EXISTS bodies, FROM subqueries, and the CTAS query as raw text with no strict check, so `SELECT (SELECT 1 */ 2 FROM secret) FROM public` parsed clean. Both parsers now record every raw body (save/restore truncate the list), and strict mode parses each one and requires exactly one query, mirroring trino. - A lex error after a complete statement (`SELECT 1 /* unterminated`) was promoted after the node was built, so the node stayed. trino, doris, and starrocks drop the node when the segment carries any error; googlesql collects lex errors once for the whole input and drops the node of the segment they fall in. ParseBestEffort keeps the prefix in every case. googlesql and snowflake diagnostics converted columns with the byte LineTable; they now use review.Index, so a diagnostic after a multi-byte character reports the code-point column Bytebase's Position expects. Offset stays the byte offset. AGENTS.md's Split row claimed every splitter returns empty segments; only pg, redshift, partiql, and cassandra do. The row says so, the target stays keeping them, and the nine splitters that drop them are a listed deviation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 867947db35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…report lex-only input, place subquery errors exactly, order diagnostics Four findings from Codex review on #435: - doris and starrocks panicked on `CREATE TABLE t AS ` (P1): the EOF token starts after the trailing whitespace while prev ends at AS, so parseRawQuery sliced backwards. It now returns a positioned parse error when nothing follows AS, in both modes. - snowflake strict Parse returned nil for `/* unterminated`: Split dropped the tokenless segment and its lex error with it. Strict mode lexes the whole input once more and promotes what the segments did not report, so diagnostics see it. - googlesql re-parsed embedded subqueries with the node's start as the lexer base, so an error after `EXISTS( ` was reported nine bytes early. SubqueryExpr, ExistsExpr, and ArraySubqueryExpr carry TextStart, the exact offset of the trimmed body, and the re-parse uses it. - googlesql appended the whole-input lex errors after every segment's parse errors, so an invalid byte at 0 came after a syntax error at 10. The list is now stably sorted by position. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a76beae5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… reject empty subquery bodies, drop nodes after hint errors Three findings from Codex review on #435: - A lexical error inside a raw query body was reported by the nested strict parse and again when the outer lexer, which scanned the same bytes, promoted its own: `SELECT (SELECT \x00)` listed the error twice, one more per nesting level. parseSingle now drops an error that repeats an earlier one's position and message in trino, doris, and starrocks. - googlesql skipped the re-parse when a subquery body was empty, so `SELECT EXISTS()` and `SELECT ARRAY(/* c */)` parsed clean. An empty body is now a syntax error at the node. - googlesql captured the error count after the statement-hint scan, so `@[5@] SELECT 1` kept its node next to the hint error. Strict mode now drops the node when the segment recorded any error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Adds the repository's agent instructions and then fixes the four places where the parsers did not meet the contract those instructions state. One commit per step; each is reviewable on its own.
docs: add AGENTS.md— rootAGENTS.mdis the instruction source of truth (CLAUDE.mdimports it). It records the repository shape and fork map, the engine contract, how the parser is called from Bytebase and from omni's own packages, the test and verification gates CI actually runs, document locations, and the commit format.pg/parser/CLAUDE.mdbecomespg/parser/AGENTS.mdwith its stale Docker-skip section corrected;redshift/parserkeeps a pointer instead of a 217-line copy;docs/AGENTS.mdmarks plans, specs, and scenarios as history.fix(trino,googlesql): make Parse strict and fail query span closed— ports doris/starrocks: make Parse strict, reserve tolerance for ParseBestEffort (BYT-10085) #402.Parsedrops a statement whose prefix parsed but left tokens behind;ParseBestEffortkeeps the prefix.GetQuerySpanandClassifySQLreturn the parse error instead of analyzing a partial tree (masking and lineage saw less than the engine would execute). trino subquery placeholders gainTextStartso nested errors report outer-statement positions; empty, comment-only, multi-statement, and non-query bodies fail the span (all four rejected by Trino 482, checked against the oracle). googlesql diagnostics readsParse; trino completion retries its empty-select-list fill when the caret-patched statement does not parse.perf(oracle): parse each segment in place—oracle.Parsepadded every segment withByteStartspaces, so an n-statement script lexed O(n²) bytes: a 2000-statement script took 775 ms / 462 MB.parser.ParseRange(source, start, end)andNewLexerRangeparse a segment in place with absolute positions; the same script now takes 12 ms / 4 MB. The wrapped-procedure scan stops at the range end.fix(...): count Position columns in code points— the top-levelStatementpositions of seven engines counted columns in bytes through seven private copies of the line index; they now go throughreview.Index(code points, Bytebase'sPositionunit). elasticsearch's doc was wrong, not its code.fix(...): unify ParseError to Message and Position— every engine'sParseErrorcarriesMessageand a bytePosition. trino, googlesql, doris, and starrocksParsereturnerror(aParseErrorslist that unwraps to each*ParseError;AllErrors/FirstErrorread it) instead of[]ParseError. Their parser test corpora keep the list assertions through aparseForTestshim.Verification
go build ./...,go veton every changed package (no new findings;oracle/parsercarries older unreachable-code findings).backend/plugin/parser/{pg,tsql,plsql,redshift,cassandra,cosmosdb,mongodb,elasticsearch,trino,googlesql,spanner,bigquery},advisor/oracle,schema/oracle) pass against this branch via a replace directive for commits 2–4. Commit 5 changes field names andParse's return type for eight engines' adapters; that migration is a Bytebase PR once the pin bumps.Follow-ups
ParseErrorand tooracle/parser.ParseRange(drops the reflectionLocshifter in plsql).🤖 Generated with Claude Code