Parse UPDATE/DELETE ORDER BY and LIMIT behind an option - #5
Merged
Conversation
SQLITE_ENABLE_UPDATE_DELETE_LIMIT selects two extra grammar rules, giving UPDATE and DELETE the orderby_opt limit_opt tail that SELECT has. The pinned build defines neither it nor SQLITE_UDL_CAPABLE_PARSER, so meyer rejected those clauses outright -- correct for that build, and wrong for anyone pointing sqlc at a database compiled with the option, whose valid SQL meyer could not read. parser.Options carries the fork. Its zero value is the pinned build, so the corpus still defines the default behaviour (wherelimit.test expects the rejection, byte offset included), and the same entry points hang off Options as methods for a caller that needs the other side. Only top-level statements grow the clauses: trigger_cmd has no orderby_opt or limit_opt to gate, in either build. The expectations in parser/options_test.go come from a SQLite built with the option, which takes a Lemon run over src/parse.y rather than a -D on the amalgamation -- the released amalgamation ships a parse.c generated without the rules, so defining the macro against it changes nothing. The recipe is at the top of that file. cmd/difftest, pointed at that build with the option on, agreed with meyer on 228,652 mutations of the whole corpus. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019kZMPnsTWywB5icVh1wJrA
The doc comment named a specific vendor's build as shipping with SQLITE_ENABLE_UPDATE_DELETE_LIMIT. I could not verify it, so it goes; the part that carries the argument -- the option rewrites the parser tables, so it lives in a build meyer cannot detect from the SQL -- stands on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019kZMPnsTWywB5icVh1wJrA
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
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.
SQLITE_ENABLE_UPDATE_DELETE_LIMITselects two extra grammar rules, givingUPDATEandDELETEtheorderby_opt limit_opttail thatSELECThas. The pinned build defines neither it norSQLITE_UDL_CAPABLE_PARSER, so meyer rejected those clauses outright — correct for that build, and wrong for anyone pointing sqlc at a database compiled with the option, whose valid SQL meyer could not read.The shape
parser.Optionscarries the fork. Its zero value is the pinned build, so the corpus still defines the default behaviour (wherelimit.testexpects the rejection, byte offset included), and the same entry points hang offOptionsas methods for a caller that needs the other side:parser/parser.go—Options, withParse,ParseStringandParseStatementas methods; the package-level functions delegate toOptions{}.ParseExprhas noOptionsform, since no option reaches an expression.parser/parse_dml.go—parseUpdateDeleteLimitimplements the tail. With the option off it consumes nothing, soORDER/LIMITis a token no rule can shift and the caller's end-of-statement check reports it, which is how the default message and offset stay byte-identical.ast/dml.go,ast/render.go—OrderBy/LimitonUpdateStmtandDeleteStmt, inChildren()and the renderer.internal/roundtrip—CheckWith, so a tree parsed with an option is re-parsed with it.cmd/debug-parse—-update-delete-limit.Only top-level statements grow the clauses:
trigger_cmdhas noorderby_optorlimit_optto gate, in either build.Verification
The expectations in
parser/options_test.gocome from a SQLite built with the option, which takes a Lemon run oversrc/parse.yrather than a-Don the amalgamation — the released amalgamation ships aparse.cgenerated without the rules, so defining the macro against it changes nothing (confirmed: no call toupdateDeleteLimitErrorsurvives in it). The recipe is at the top of that file: buildtool/lemon.c, regenerateparse.cwith the define, splice it intosqlite3.cover the region markedBegin file parse.c, compile with the define. Lemon assigns the same token numbers either way — the generatedparse.his byte-identical — so the splice is sound.Against that build:
no such table: t1), i.e. it parsed;OFFSETwithout itsLIMIT,RETURNINGafter aLIMIT(where_opt_retprecedesorderby_opt), and both trigger-body forms;cmd/difftest, pointed at that build with the option on, agreed with meyer on 228,652 mutations across the whole corpus, 0 disagreements (that run needed a local patch to difftest, which is not part of this branch).go test ./...,gofmtandgo vetare clean; the corpus is untouched.Note for follow-up
SQLITE_ENABLE_ORDERED_SET_AGGREGATESis the only other gate inparse.ythat adds grammar (f(x) WITHIN GROUP (ORDER BY y)), so it is the only other way meyer can reject SQL a real build accepts.WITHINjoins the%fallback IDset under the same gate, so it would need no keyword-table change, andpercentile.testalready carries the "off" side. SQLite's docs call it off by default and omitted from most builds, so it is not urgent. TheSQLITE_OMIT_*family only ever subtracts, and is not worth an option each.Generated by Claude Code