docs(wiki): rewrite protocol-parsing.md's SQL section against parse_sql as ADR-0008 leaves it - #282
Conversation
…ql as ADR-0008 leaves it The section still described the leading-token heuristic ADR-0008 replaced, and its `DROP MATERIALIZED VIEW mv` claim cited a test that asserts the opposite: `drop_if_exists_extracts_real_table` proves the table is empty, because a view is not a table. Write the section from the code instead: `parse_sql` classifies by what a statement executes (`EXPLAIN ANALYZE`, data-modifying CTEs), reports the most dangerous verb per `VERB_PRECEDENCE`, and fills `sql.table` with one relation or nothing — a multi-target or cascading statement deliberately names none, so only a table-blind rule can decide it. Every row of the new table cites the test that proves it. The `clean_identifier` sentence is kept verbatim, now placed in the fallback paragraph it describes. Repoint the three Test coverage rows the #261 thread found a thousand lines stale, add the schema-qualified row that thread asked for, and drop the unit-test count the table's intro had stopped matching. Drop the #261 entry from the anchor ledger, since the page no longer produces that finding, and regenerate llms-full.txt. Refs #261
…it names protocols.rs:41-93 now opens inside parse_postgres_query; parse_sql is at 72-99, where the rewritten SQL section already cites it. Refs #261
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation in wiki/deep-dive/protocol-parsing.md and wiki/llms-full.txt to describe the transition of parse_sql from a basic heuristic to using PostgreSQL's grammar via sqlparser's PostgreSqlDialect. It also updates the associated test coverage details and removes a tracked anchor in scripts/check-wiki-source-anchors.ts. The review feedback correctly identifies a documentation citation issue where the cited line range for parse_sql_heuristic does not start at its doc comment, which violates the repository's documentation conventions.
 |
![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: 16e294148b
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
- state that the parser is sqlparser's PostgreSQL dialect, not the server's grammar, as ADR-0008's consequences already say - narrow the sql.table rule: writes of different rank are not a tie, the higher-ranked verb keeps its table and the lower write is visible to no rule (#104) - start the parse_sql_heuristic citation on its doc comment Refs #261
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
/gemini review |
…ames more_dangerous blanks the table on every tied write without comparing targets; the page had narrowed that to different relations. Refs #261
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 682c6f4f4a
ℹ️ 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".
There was a problem hiding this comment.
Code Review
This pull request updates the documentation in wiki/deep-dive/protocol-parsing.md and wiki/llms-full.txt, along with the anchor-checking script in scripts/check-wiki-source-anchors.ts, to align with the new SQL parsing behavior introduced by ADR-0008. The documentation now details how parse_sql uses sqlparser's PostgreSQL dialect to classify statements based on execution rather than leading tokens, outlines verb precedence and table extraction rules, and updates the corresponding test coverage references. There are no review comments to evaluate, and I have no additional feedback to provide.
…with the first The framing paragraph cited the test at 219-237 while the coverage table, repointed in this PR, cites it at 1061-1079. Refs #261
|
1 participant
What
wiki/deep-dive/protocol-parsing.md's SQL section still described the leading-token heuristic that ADR-0008 replaced withsqlparser, and one claim was refuted by the test it cited: the page saidDROP MATERIALIZED VIEW mvmust yieldmv, "proven bydrop_if_exists_extracts_real_table", while that test asserts the table is empty because a view is not a table.Changes
parse_sqldoes: parse withsqlparser's PostgreSQL dialect (with ADR-0008's caveat that the dialect is not the server's grammar), classify by what the statement executes (EXPLAIN ANALYZE, data-modifying CTEs), report the most dangerous verb perVERB_PRECEDENCE, and fillsql.tablewith one relation or nothing. The empty-table cases are stated as the rule (sole_relation,more_dangerous): several targets, a cascade, any same-rank tie, a read over several relations. The two limits the precedence choice leaves are named rather than papered over: writes of different rank are not a tie, and an upsert reported asUPDATEpasses a deny-INSERT/allow-UPDATEpolicy (postgres facts: carry every verb and relation a statement executes (sql.verbs / sql.tables) #104). Every row of the new table cites the test that proves it, with the statement that test asserts.clean_identifiersentence is unchanged, moved verbatim into the fallback paragraph it describes; the parsed path'srelation_namegets its own sentence.parses_postgres_truncate_and_selectrow that thread asked for is added, and the "9 unit tests" count in the intro is dropped (the module holds 46 now).parse_sqlrow (41-93, now insideparse_postgres_query, to72-99) and the framing paragraph'srejects_malformed_query_frames(219-237to1061-1079, matching the coverage table). The page's remaining stale anchors are in the PostgreSQL framing and Kubernetes sections and are filed as docs(wiki): protocol-parsing.md's PostgreSQL framing and Kubernetes anchors predate the sqlparser growth of protocols.rs #284, not widened into this diff.#261TRACKEDentry inscripts/check-wiki-source-anchors.tsis removed, since the page no longer produces that finding, and the comment above the ledger says so.wiki/llms-full.txtregenerated.Verification
Both wiki checks go red on the old state and green with the fix:
check-wiki-source-anchors.tswith the old ledger entry against the fixed page:TRACKED names protocols.rs#L248-L254 (blank, #261) … which produces no such finding any more.check-wiki-bundle-current.tswith the old bundle against the new page:its wiki/deep-dive/protocol-parsing.md section is not that page's current content.At the final head: 1150 citations resolve, bundle byte-identical to 14 pages,
bun run lint && bun run typecheck && bun testgreen (533 pass),grep -n '^#[0-9]'on the page prints nothing.Not changed: anything under
crates/. The code is right and the page was wrong.Closes #261