oracle: support constraint_state clause on constraints (BYT-10010) - #399
oracle: support constraint_state clause on constraints (BYT-10010)#399rebelice wants to merge 6 commits into
Conversation
The Oracle parser only implemented DEFERRABLE / INITIALLY out of Oracle's constraint_state grammar. Everything else — USING INDEX (all forms), ENABLE/DISABLE, VALIDATE/NOVALIDATE, RELY/NORELY, EXCEPTIONS INTO — failed with a syntax error, rejecting valid DDL such as PRIMARY KEY (...) USING INDEX LOCAL and the NOT NULL ENABLE emitted by DBMS_METADATA. Add a shared parseConstraintState that accepts the subclauses in any order, and a parseUsingIndexClause whose whitelist-driven property parser terminates safely inside a CREATE TABLE column list. Wire them into table-level constraints, column-level constraints, the implicit NOT NULL/NULL paths, and collapse the three ad-hoc ALTER TABLE implementations (skipConstraintState, the ENABLE/DISABLE USING INDEX block, and the ADD CONSTRAINT path) onto the same code. Also fix adjacent gaps found while auditing the same family: - USING INDEX ... COMPUTE STATISTICS (DBMS_METADATA output) - table-level MAXTRANS and IOT PCTTHRESHOLD options - out-of-line view constraints (CONSTRAINT ... RELY DISABLE NOVALIDATE) - CREATE/ALTER MATERIALIZED VIEW USING INDEX, USING NO INDEX, USING TRUSTED/ENFORCED CONSTRAINTS TableConstraint gains UsingIndexLocal and now populates the previously-dead Tablespace field; ColumnConstraint gains both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up to the constraint_state change: CREATE VIEW's out-of-line constraints (CONSTRAINT ... PRIMARY KEY/UNIQUE/FOREIGN KEY ... RELY DISABLE NOVALIDATE) were parsed and discarded. Store them on the new CreateViewStmt.Constraints list as *TableConstraint nodes so schema extraction can consume them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…23ai Cross-validated the constraint_state implementation against a real Oracle 23ai instance (gvenzl/oracle-free testcontainer). Oracle is not order-free: out-of-order subclauses raise ORA-03075 (e.g. ENABLE USING INDEX, DISABLE RELY, NOVALIDATE ENABLE). Rewrite parseConstraintState from a free-order loop to the documented slot sequence — [NOT] DEFERRABLE / INITIALLY (interchangeable within the group), then RELY|NORELY, using_index_clause, ENABLE|DISABLE, VALIDATE|NOVALIDATE — so review verdicts match rollout behavior in both directions. EXCEPTIONS INTO moves out of parseConstraintState into its own parseExceptionsIntoClause: Oracle rejects it inside CREATE TABLE constraints (ORA-00922) and accepts it only in ALTER TABLE enable / modify-constraint contexts, which now share the helper. Engine comparison result: 33/34 statements agree. The one deliberate leniency is ALTER MATERIALIZED VIEW USING INDEX PCTFREE, which 23ai rejects (ORA-02243) while INITRANS/STORAGE are accepted; noted in a comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 026cde98d5
ℹ️ 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".
All three Codex review findings verified against Oracle 23ai before fixing: - Accept the remaining legal index_attributes in using_index_clause: PARALLEL [n], NOPARALLEL, INDEXING FULL|PARTIAL, and COMPRESS ADVANCED [LOW|HIGH]. All parse on 23ai (their failures — ORA-03001, ORA-12812, ORA-14227, ORA-25193 — are semantic, not syntax). - Validate the parenthesized using_index form: require the CREATE [UNIQUE|BITMAP] INDEX prefix instead of skipping any balanced tokens. Oracle raises ORA-02000 "missing CREATE keyword" for anything else. - Restrict view constraints to their actual grammar: [RELY|NORELY] DISABLE [NOVALIDATE], with DISABLE mandatory (ORA-02000) and VALIDATE rejected (ORA-03082). parseTableConstraint is split into parseTableConstraintBody + state so CREATE VIEW and ALTER VIEW ADD CONSTRAINT share the body but apply the view-specific state. Engine comparison after the fixes: 39/40 agree; the sole remaining deliberate leniency is ALTER MATERIALIZED VIEW USING INDEX PCTFREE. Co-Authored-By: Claude Fable 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: 8da4176a0f
ℹ️ 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".
All three findings verified against Oracle 23ai before fixing: - GLOBAL PARTITION BY HASH now accepts hash_partitions_by_quantity: PARTITIONS n [STORE IN (ts, ...)] in addition to explicit partition specs. - The parenthesized using_index form validates the nested statement's required structure — CREATE [UNIQUE|BITMAP] INDEX name ON table (...) — matching Oracle's ORA-00953 (missing index name) and ORA-00969 (missing ON). Trailing index attributes remain balanced-skipped. - MODIFY PRIMARY KEY / MODIFY UNIQUE attach a populated TableConstraint (type, Deferrable, Initially, Tablespace, UsingIndexLocal) instead of discarding the parsed state, matching the MODIFY CONSTRAINT branch. Co-Authored-By: Claude Fable 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: 9d742c94bd
ℹ️ 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".
Each finding was tested against Oracle 23ai before acting; three were real, two were refuted by the engine, one was wrong but exposed a different gap: - PARTITIONS quantity is now HASH-only: GLOBAL PARTITION BY RANGE ... PARTITIONS n is rejected (Oracle: ORA-00906). - LOCAL STORE IN (...) and explicit partition specs are now mutually exclusive, matching ORA-14153/ORA-03075 — the review's suggested "partition list then STORE IN" form is itself rejected by Oracle. - USING TRUSTED|ENFORCED CONSTRAINTS now requires a preceding REFRESH clause (Oracle: ORA-00906 for the standalone form). - PCTTHRESHOLD now requires ORGANIZATION INDEX (Oracle: ORA-00922 on heap tables). Not changed, with engine evidence recorded in tests: - IF NOT EXISTS inside the nested USING INDEX (CREATE INDEX ...) form is rejected by Oracle itself (ORA-00969), although valid standalone; the current rejection matches the engine. - Column aliases after an out-of-line view constraint are accepted by Oracle despite the documented BNF ordering; the current acceptance matches the engine. Co-Authored-By: Claude Fable 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: 98ba1ba62f
ℹ️ 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".
| isHash := p.cur.Type == kwHASH | ||
| if p.cur.Type == kwRANGE || p.cur.Type == kwHASH { | ||
| p.advance() |
There was a problem hiding this comment.
When GLOBAL PARTITION BY is followed directly by (, this conditional simply skips the method instead of rejecting it, so invalid SQL such as CREATE TABLE t (a NUMBER, CONSTRAINT pk PRIMARY KEY (a) USING INDEX GLOBAL PARTITION BY (a) (PARTITION p1 VALUES LESS THAN (MAXVALUE))) parses successfully. global_partitioned_index in oracle/parser/bnf/CREATE-INDEX.bnf requires exactly RANGE or HASH here, so return a syntax error when neither token is present.
Useful? React with 👍 / 👎.
| if p.cur.Type != '(' { | ||
| return p.syntaxErrorAtCur() | ||
| } | ||
| depth := 1 // the outer '(' consumed above is still open |
There was a problem hiding this comment.
Fresh evidence beyond the resolved missing-name/ON cases is that this only checks for an opening parenthesis and then skips its contents, so USING INDEX (CREATE INDEX idx ON t ()) and malformed lists such as (a,) are accepted even though table_index_clause requires one or more valid index_expr entries. Parse the column list rather than treating any balanced group as sufficient.
Useful? React with 👍 / 👎.
| if p.cur.Type == tokQIDENT || | ||
| (p.cur.Type == tokIDENT && !identLikeUsingIndexProperties[p.cur.Str]) { |
There was a problem hiding this comment.
Fresh evidence beyond the four resolved index attributes is the ANNOTATIONS alternative in oracle/parser/bnf/CREATE-INDEX.bnf, which the standalone CREATE INDEX parser already handles. Here ANNOTATIONS is absent from the property-starter set, so USING INDEX ANNOTATIONS (classification 'pii') treats ANNOTATIONS as an index name and leaves ( unconsumed, rejecting valid constraint DDL; classify and consume this attribute with the other index properties.
Useful? React with 👍 / 👎.
| // parseExceptionsIntoClause parses [ EXCEPTIONS INTO [schema.]table ] if | ||
| // present. Oracle allows this only in ALTER TABLE enable/modify-constraint | ||
| // contexts, not inside CREATE TABLE constraints. |
There was a problem hiding this comment.
This restriction omits ALTER TABLE ... ADD CONSTRAINT, although constraint_clauses and constraint_state in oracle/parser/bnf/ALTER-TABLE.bnf allow the trailing exceptions_clause on an added out-of-line constraint. Consequently, valid statements such as ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY (a) ENABLE VALIDATE EXCEPTIONS INTO bad_rows leave EXCEPTIONS unconsumed and fail; invoke this helper from the ADD-constraint path as well.
Useful? React with 👍 / 👎.
| case "MAXTRANS": | ||
| p.advance() // consume MAXTRANS | ||
| if p.cur.Type == tokICONST { | ||
| p.advance() | ||
| } |
There was a problem hiding this comment.
The newly added MAXTRANS branch makes its numeric operand optional, so CREATE TABLE t (a NUMBER) MAXTRANS reaches the statement terminator and parses successfully even though MAXTRANS requires an integer. The USING INDEX implementation correctly rejects the same missing operand; apply that validation here rather than silently accepting an incomplete physical attribute.
Useful? React with 👍 / 👎.
| // UNIQUE | FOREIGN KEY } (cols) [RELY|NORELY] DISABLE [NOVALIDATE]. | ||
| // View constraints are declarative only; the table-only state | ||
| // clauses are rejected — see parseViewConstraintState. | ||
| if p.isTableConstraintStart() { |
There was a problem hiding this comment.
Using the table-specific start predicate here rejects valid out-of-line view constraints whose unquoted name is a nonreserved datatype token. For example, CREATE VIEW v (a, CONSTRAINT blob PRIMARY KEY (a) DISABLE) AS SELECT 1 FROM dual treats CONSTRAINT as an alias because BLOB satisfies isOracleTypeToken, then fails at the column list; unlike a CREATE TABLE column list, a view alias list has no datatype declaration ambiguity, so CONSTRAINT should always start a constraint here.
Useful? React with 👍 / 👎.
1 participant
Fixes the customer-blocking parser gap from BYT-10010: SQL Review rejected valid Oracle DDL such as
PRIMARY KEY (...) USING INDEX LOCAL, blocking MBBank's partitioned-table changes since 3.19.0 (when the ANTLR fallback was removed).What was broken
The Oracle parser implemented only
DEFERRABLE/INITIALLYout of Oracle'sconstraint_stategrammar. Everything else failed with a syntax error:USING INDEXform (LOCAL, bare,TABLESPACE, index name,(CREATE INDEX ...))ENABLE/DISABLE,VALIDATE/NOVALIDATE,RELY/NORELYNOT NULL ENABLE— the default shape ofDBMS_METADATA.GET_DDLoutputALTER TABLE ADD/MODIFY CONSTRAINTWhat this PR does
Commit 1 — shared constraint_state implementation. One
parseConstraintStateplus aparseUsingIndexClausewhose whitelist-driven property parser terminates safely inside a CREATE TABLE column list (deliberately not shared withparseCreateIndexAttributes, whose collector runs to the statement terminator). Wired into table-level, column-level, andNOT NULL/NULLpaths; collapses the three ad-hoc ALTER TABLE implementations (skipConstraintStateis deleted). Also fixes adjacent gaps found while auditing the same family:COMPUTE STATISTICSinUSING INDEX(DBMS_METADATA output), table-levelMAXTRANS, IOTPCTTHRESHOLD, out-of-line view constraints, andCREATE/ALTER MATERIALIZED VIEW USING INDEX/USING NO INDEX/USING TRUSTED|ENFORCED CONSTRAINTS.Commit 2 — view constraints in the AST.
CreateViewStmt.Constraints(list of*TableConstraint), walker regenerated.Commit 3 — slot order, verified against a real engine. Cross-validated against Oracle 23ai (gvenzl/oracle-free testcontainer, real DDL execution). Oracle is not order-free: out-of-order subclauses raise ORA-03075, so
parseConstraintStateenforces the documented slot sequence (DEFERRABLE/INITIALLYinterchangeable within their group, thenRELY|NORELY,using_index_clause,ENABLE|DISABLE,VALIDATE|NOVALIDATE).EXCEPTIONS INTOmoved to a separate helper used only by ALTER contexts — Oracle rejects it inside CREATE TABLE constraints (ORA-00922).AST changes
TableConstraint: newUsingIndexLocal; the previously-deadTablespacefield is now populatedColumnConstraint: newTablespace,UsingIndexLocalCreateViewStmt: newConstraintsVerification
ALTER MATERIALIZED VIEW USING INDEX PCTFREE, which 23ai rejects (ORA-02243) while acceptingINITRANS/STORAGE; noted in a comment./oracle/...suite passes, including the parser contract gatesLanding
After merge:
go get -u github.com/bytebase/omniin bytebase, runparser/plsql/advisor/oracle/schema/oracle, cherry-pick to 3.21.x for MBBank. Wiring local-index semantics intoIndexMetadatais deferred to a separate issue.🤖 Generated with Claude Code