Repository navigation
fix(oracle): accept non-reserved keywords as column names, gate on Oracle's own PL/SQL (BYT-10301) - #446
Conversation
…acle's own PL/SQL (BYT-10301) Two engine-driven gates, from the BYT-10301 investigation: TestOraclePLSQLDictionary parses every unwrapped, VALID procedure, function, package, package body, type body, and trigger of the Oracle-maintained schemas in the pinned image (875 units) through Split and ParseRange, as Bytebase runs a script. The 102 units that fail today are recorded with their category in plsql_dictionary_known_failures.tsv; a new failure is a regression and a fixed unit must leave the list, so it only shrinks (-update regenerates it). Oracle's source is read at test time, never committed. TestOracleNonReservedKeywordsAsColumns takes every word omni lexes as a keyword that V$RESERVED_WORDS does not reserve (232) and checks it as a column against the engine: defined in CREATE TABLE, then referenced from a select list, WHERE, ORDER BY, UPDATE, an INSERT column list, and an index (1,624 checks). The keyword manifest only covered the definition, which is how OFFSET regressed in #118/#154 unnoticed. The audit found 36 disagreements; 35 were omni rejecting valid SQL and are fixed: - FETCH, JOIN, MODEL, USING (like OFFSET) start an operand as a column; only reserved clause keywords end one. - CAST, DECODE, INTERVAL name a column unless followed by '(' or a string literal; CASE names one when the next token cannot continue a CASE expression (Oracle reports `SELECT case` as ORA-00904, not a syntax error, so strictness row expr_014 now uses SELECT CASE 1). - PRIMARY and FOREIGN start a table constraint only before KEY. - PRIVILEGES is not reserved (V$RESERVED_WORDS, and absent from the 26ai reserved-word list); CONTENT and JOIN work as schema-qualified table names. The keyword manifest rows now say so. - A statement may end with a column named CONTENT, JOIN, or USING, so they leave the incomplete-statement heuristic; truncated clauses still fail in their own parsers. The one remaining gap, XMLELEMENT as an index column (Oracle rejects, omni accepts), stays in oracle_keyword_column_gaps.tsv. The Oracle image is pinned to gvenzl/oracle-free:23.26.3-slim-faststart, the same digest as the 23-slim-faststart tag used so far, so the dictionary the manifest describes cannot drift; AGENTS.md lists it. Co-Authored-By: Claude Opus 5.5 <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: 2114d1bb5d
ℹ️ 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".
…umns (BYT-10301) Codex review on #446: - `CASE NOT TRUE WHEN TRUE THEN 1 ELSE 0 END` regressed: CASE followed by NOT was taken for a column. Oracle 23ai accepts a Boolean selector there, and also accepts a column named CASE in NOT IN, NOT BETWEEN, and NOT LIKE, so caseNamesColumn now looks past NOT (peekAhead lexes a copy of the lexer). - `WHERE cast(+) = u.a` failed because CAST before '(' always opened CAST(...). A non-reserved keyword followed by (+) is now a column for every word Oracle accepts that way. Asked word by word, Oracle rejects word(+) for JSON, JSON_ARRAY, JSON_EXISTS, JSON_MERGEPATCH, JSON_OBJECT, JSON_QUERY, JSON_TABLE, JSON_VALUE, TREAT, XMLELEMENT, XMLFOREST, and XMLROOT; those now fail at the '+' (JSON and JSON_TABLE were accepted before). The keyword column audit gains the outer-join marker and NOT IN contexts (232 words, 2,088 checks). NOT IN exposed one pre-existing gap, recorded in oracle_keyword_column_gaps.tsv: Oracle never lets CONNECT_BY_ROOT name a referenced column, while omni accepts `CONNECT_BY_ROOT NOT IN (1, 2)` because a reserved word in operand position (IN) parses as a function name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c61863b7e
ℹ️ 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".
…t (BYT-10301) Codex review on #446: SYSTIMESTAMP(+) became an outer-joined column because SYSTIMESTAMP is a non-reserved keyword. Oracle 23ai reads it as SYSTIMESTAMP(precision) and rejects it with ORA-30088, even when the table has a column of that name; only a qualified t.systimestamp(+) is a column. The shortcut now skips pseudo-column keywords (SYSTIMESTAMP is the only non-reserved one), as main did. Pinned by TestParseKeywordColumnOuterJoin and reference row ref_096. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c04206251a
ℹ️ 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".
…ailure signatures (BYT-10301) Codex review on #446: - `CASE nulls WHEN 1 THEN 2 END` regressed: NULLS after CASE was taken for a column named CASE ordered NULLS FIRST/LAST. Oracle 23ai accepts both readings, and LIKEC, LIKE2, and LIKE4 (also not reserved) are ambiguous the same way. caseNamesColumn now reads one more token for each: NULLS orders the column only before FIRST or LAST; LIKEC/LIKE2/ LIKE4 open a selector before WHEN or an operator and compare the column otherwise. The keyword column audit gains a `case selector` context (every non-reserved word as CASE word WHEN ...; 232 words, 2,320 checks), and reference rows ref_097..ref_101 pin the column side. - TestOraclePLSQLDictionary compared only which units fail. It now also compares each known unit's recorded line:column and message, so a regression that makes it fail earlier is reported (and a later failure asks for the manifest to be re-recorded). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9906b13ffb
ℹ️ 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".
…T-10301) Codex review on #446: `CASE likec = 'x' WHEN TRUE THEN 1 ELSE 0 END` regressed. A simple CASE selector may be any expression, Boolean comparisons included (Oracle 23ai), so judging from the one or two tokens after CASE kept missing forms (NOT, NULLS, LIKEC, then comparisons). caseNamesColumn now applies the rule as Oracle reads it: CASE opens an expression when WHEN follows it, or when an expression follows and is itself followed by WHEN; otherwise it is a column. The selector is parsed on a probe whose parser state (saveState / restoreState) is discarded, so a real CASE expression is parsed again and its errors keep their positions (TestParseCaseExpressionErrorPosition). Verified on Oracle 23ai: Boolean comparison and IN selectors on LIKEC, `CASE case IS NULL WHEN TRUE ...`, and a column named CASE with an alias are all valid. Reference row ref_102 pins the comparison selector; the keyword audit (232 words, 2,320 checks) still agrees with Oracle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7e3b052cb
ℹ️ 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".
…(BYT-10301) Codex review on #446: probing the selector and then reparsing it made nested simple CASE selectors exponential, doubling per level (20 levels, 478 bytes, took 1 s; each further level doubled it), which a short statement could use to exhaust the parser in review or completion. parseCaseOrColumn replaces the probe-then-parse pair. When the selector is followed by WHEN, the CASE expression continues from the selector already parsed (parseCaseWhens). When it is not, the parser state is restored and the CASE offset recorded in caseColumns, so reparsing what follows does not probe the same CASE again; without that record a chain of columns named CASE (case LIKEC (case LIKEC (...))) is exponential too (1.5 s at 20 levels). Measured after the change: 400 nested selectors (9 KB) parse in under 1 ms, 400 nested CASE columns in about 60 ms. TestParseNestedCaseStaysFast pins both shapes at 60 levels. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab841ae3a4
ℹ️ 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".
Codex review on #446: probing the expression after each CASE kept the work quadratic for long sums of a column named CASE (case + case + ...; 1,600 terms took 0.68 s, doubling the input quadrupled it), because each failed probe parsed the whole remaining expression before the CASE was recorded as a column. The probe is gone. The first time parsePrimary meets CASE, decideCaseTokens lexes the rest of the range once and settles every CASE right to left with a jump table: CASE opens an expression when the first token after it that can end an operand at its depth is WHEN, skipping parenthesized groups and the CASE expressions after it. The token right after CASE must be able to start a selector (case IS NULL is a column), and END there is a column named END (Oracle accepts CASE end WHEN ..., which the keyword audit caught). Each token is visited a constant number of times. Measured: 20,000 CASE column terms (140 KB) in 24 ms, 4,000 nested CASE selectors in 11 ms, 4,000 nested CASE columns in 11 ms. TestParseCaseDecisionStaysLinear pins all three; the keyword audit (2,320 checks) and the CASE tests still agree with Oracle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e272144cfd
ℹ️ 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".
…E pre-pass (BYT-10301) Codex review on #446: the pre-pass took every END for the end of a CASE and every CASE for an opener, so `CASE t.end WHEN 1 THEN 2 END`, `CASE 1 + end WHEN 2 THEN 3 END`, and `CASE t.case WHEN 1 THEN 2 END` were read with a column named CASE and failed. Oracle 23ai accepts all three, and `THEN end ELSE 0 END` too. A word after '.' is now a name component the scan ignores, and END right where an operand must start (after an operator, '(', ',', CASE, WHEN, THEN, ELSE, AND, OR, NOT, ...) is a column named END; only END after an operand closes a CASE. This also covers END as the first selector token, which had its own branch. The keyword audit gains qualified and operand CASE selector contexts (232 words, 2,784 checks), so every non-reserved word is checked in those shapes against Oracle; it still agrees apart from the two recorded gaps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cafe4927a
ℹ️ 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".
…mn in the CASE pre-pass (BYT-10301) Codex review on #446: `SELECT CASE CONNECT_BY_ROOT end WHEN 1 THEN 2 END FROM t CONNECT BY ...` failed because expectsOperandAfter knew the unary PRIOR but not CONNECT_BY_ROOT, so the pre-pass took the column END for the end of the CASE. Auditing the other expression-level keywords that take an operand found the same gap after ESCAPE (`CASE a LIKE 'x' ESCAPE end WHEN TRUE ...`), OF (MEMBER OF, SUBMULTISET OF), and ZONE (AT TIME ZONE); Oracle 23ai accepts the CONNECT_BY_ROOT and ESCAPE forms. Tokens inside parentheses do not matter: the scan skips the group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f44b6635bf
ℹ️ 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".
…s (BYT-10301) Codex review on #446: `CASE sales['Mouse Pad', 1998] WHEN 1 THEN 2 END` regressed because the pre-pass matched only parentheses, so the comma inside the MODEL cell reference ended the scan and the CASE was taken for a column. Brackets are now matched and skipped like parentheses, and ']' ends an operand like ')'. Verified on Oracle 23ai with a two-dimensional MODEL rule; reference row ref_103 pins it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff32eea9ce
ℹ️ 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".
…BYT-10301) Codex review on #446: with a MODEL measure named CASE, `RULES (case[2] = CASE case[1] WHEN 10 THEN 20 END)` failed because the pre-pass let '[' start the selector of the inner CASE, which then consumed the outer WHEN ... END. '[' cannot begin a selector, so CASE followed by '[' is now the cell-reference identifier. Oracle 23ai runs the full query (main rejected it too, at the select-list column CASE); reference row ref_104 pins it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Follow-up to #443 (BYT-10301). That investigation found that omni's Oracle parser had no gate against the two failure classes the customer hit: PL/SQL coverage was never measured, and keyword checks covered column definitions only. That gap is how
OFFSETregressed unnoticed in #118/#154. This PR adds both gates, checked against the engine, and fixes what the second one finds.New gates
TestOraclePLSQLDictionarycovers the PL/SQL Oracle itself ships.Split+ParseRange, as Bytebase runs a script, and must come out as one segment that parses.plsql_dictionary_known_failures.tsv.-updateregenerates it.CHARACTER SET ANY_CS/%CHARSETPIPELINED/AGGREGATE USINGACCESSIBLE BY$IF/$END, which also mis-splits)REFtypes, triggerCALLwith argumentsTestOracleNonReservedKeywordsAsColumnscovers keywords that Oracle does not reserve.V$RESERVED_WORDSdoes not reserve (232 words).CREATE TABLE, then referenced from a select list,WHERE(IS NULLandNOT IN),ORDER BY,UPDATE, anINSERTcolumn list, an index, with the legacy outer-join marker(+), and in a simpleCASEselector (alone, qualified, and as an operand). That is 2,784 checks, and omni must agree with Oracle on each.Both tests take about 25 s together on the shared container.
Fixes
FETCH,JOIN,MODEL,USINGOFFSETalready was. Only reserved clause keywords end an operandCAST,DECODE,INTERVAL(or a string literalCASEWHENfollowsCASE, or follows a selector expression after it (Boolean selectors included, 23ai); a column otherwise (ORDER BY case NULLS FIRST,case LIKEC 'x%',case NOT IN (...)). Every CASE of a statement is decided in one linear pre-pass over its tokens (no expression is parsed twice), and real CASE errors keep their positions(+)cast(+),decode(+),case(+)failedJSON,JSON_*,TREAT,XMLELEMENT,XMLFOREST,XMLROOT); those are rejected at the+, andJSON(+)/JSON_TABLE(+)were wrongly accepted before. Pseudo-columns stay pseudo-columns:SYSTIMESTAMP(+)is ORA-30088PRIMARY,FOREIGNKEYPRIVILEGESV$RESERVED_WORDSsays so, it is absent from the 26ai reserved-word list, and Oracle accepts it as a table, column, alias, and qualified nameCONTENT,JOIN,USINGat statement endFROM t JOIN,MERGE INTO t USING) still fail in their own parsersFixtures corrected from engine evidence:
oracle_keywords.tsv:PRIMARY/FOREIGNcolumn definitions,PRIVILEGES, andCONTENT/JOINas qualified table names were local-policy rejects that Oracle accepts.strictness_v2.tsvrowexpr_014: Oracle answersSELECT casewith ORA-00904 (invalid identifier), not a syntax error. The row now usesSELECT CASE 1, which Oracle rejects with ORA-00923. The deeper truncations are already rowsexpr_015–expr_017.Two gaps remain in
oracle_keyword_column_gaps.tsv. In both, Oracle rejects and omni accepts:XMLELEMENTas an index column.CONNECT_BY_ROOT NOT IN (1, 2). Oracle never letsCONNECT_BY_ROOTname a referenced column. omni accepts this form because, on main as well, a reserved word in operand position parses as a function name (SELECT IN (1, 2) FROM tis accepted). That leniency is a separate change.Pinned image
The Oracle image is now
gvenzl/oracle-free:23.26.3-slim-faststart. This is the same digest (sha256:f5ff1903…) as the floating23-slim-faststarttag used so far, so no existing test changes behavior. Pinning keeps the dictionary the manifest describes from drifting. AGENTS.md lists it with the other pinned images.Verification
make test-oraclepasses.TestOracleCompatibilityReferenceReportoutcome counts are identical to main.FETCH/JOIN/MODEL/USINGcolumns now accepted, matching Oracle.go vet ./oracle/...has no new findings (13, same as main).gofmtis clean, andgo build ./...passes.The next steps in the plan each shrink the dictionary manifest:
;strictness, together with the statements it currently masksCHARACTER SET ANY_CS,PIPELINED/AGGREGATE USING,ACCESSIBLE BY, package initialization section,REF)🤖 Generated with Claude Code