terrymanu commented on PR #39591: URL: https://github.com/apache/shardingsphere/pull/39591#issuecomment-5390079629
### Result **Review Result: Mergeable** No blocking issues found in Code Correctness Review. The PR completes the last syntax gap of issue #27005 by making `REUSE SETTINGS` optional in the Oracle-private `compileTypeClause` (`parser/sql/engine/dialect/oracle/src/main/antlr4/imports/oracle/DDLStatement.g4:4303`) and adds parser IT cases for `COMPILE SPECIFICATION`, `COMPILE BODY`, and bare `COMPILE` without `REUSE SETTINGS`. Making a token group optional cannot break previously parseable inputs, and `visitAlterType` already returns a parameterless `AlterTypeStatement`, so no downstream contract consumes the optionalized tokens. One non-blocking observation: the PR title and bullets 1/2/4 describe the CREATE TYPE BODY work already merged into the base via #39589, while the actual delta contains only the REUSE SETTINGS fix; a title update before merge (for example "Support parsing Oracle ALTER TYPE COMPILE without REUSE SETTINGS") would align the PR scope with the diff. The `Fixes #27005` linkage is correct because this PR closes the remaining gap. ### Evidence - Scope: the GitHub changed-file list at `/pulls/39591/files` reports 3 files (+7/-1), matching the local triple-dot diff from base `e343ca1c6d1` to head `2adc0c53b73df8c20f0dac7a9ed180c0da571cd2` exactly (merge-base equals the base SHA; inventory script confirmed a 3/3 match). - Official syntax: the Oracle PL/SQL Language Reference (19c, `ALTER TYPE statement` / COMPILE clause) defines `REUSE SETTINGS` as optional and shows exactly the forms covered here (`ALTER TYPE link2 COMPILE SPECIFICATION;`, `ALTER TYPE link2 COMPILE BODY;`, `ALTER TYPE link2 COMPILE;`). - Ambiguity check: `alterType` alternatives start with distinct keywords (`RESET`, `REPLACE`, `ADD`/`MODIFY`/`DROP`, `NOT? (INSTANTIABLE|FINAL)`, `COMPILE`), so making everything after `COMPILE` optional introduces no cross-branch ambiguity; `DEBUG?` and `(REUSE SETTINGS)?` are independent optionals, so `COMPILE DEBUG` without `REUSE SETTINGS` exercises the same production path. - Local verification on the identical tree (sanitized, wrapped per spotless.skip): `./mvnw -pl parser/sql/engine/dialect/oracle ... install` exit 0 with grammar regeneration; `./mvnw -pl test/it/parser -Dtest=...InternalOracleParserIT ... test` exit 0 with **1947/1947 passing** (1944 pre-existing Oracle cases plus 3 new, zero regressions); `spotless:apply` + `checkstyle:check` on the changed modules exit 0 with no changes beyond this diff. - Test validity: the new sql-cases and expected `<alter-type>` entries use aligned unique `sql-case-id` values and follow the existing minimal expected form of this case family, consistent with the parameterless `AlterTypeStatement`. ### Coverage - Reviewed head: `2adc0c53b73df8c20f0dac7a9ed180c0da571cd2` (base `master` `e343ca1c6d1` containing #39589); both the GitHub and local file lists account for all 3 files; the PR has 0 comments and 0 reviews, so no challenged findings exist. - Behavior clusters: grammar optionality in `compileTypeClause`; IT case coverage — all 3 changed files are mapped, with no churn-only files; local dirty-tree entries (`AGENTS.md`, `.codex/harness/agents/cases.toml`, pycache) are outside the PR scope and were left untouched. - Dialects: target dialect Oracle; the dialect-family map was checked — Oracle has no trunk or branch dialects and `compileTypeClause` is private to `imports/oracle/`, so no sibling dialect shares or copies this rule; the new cases are tagged `db-types="Oracle"`, leaving other dialect IT runs unaffected. - Documentation used: Oracle 19c PL/SQL Language Reference ALTER TYPE / COMPILE clause pages; the issue #27005 statement list was checked — all six forms now parse on the base plus this PR. - Discovery lenses completed: root cause and behavior; blast radius and contracts; tests, runtime, and operations; plus a full convergence pass on the latest delta of the single commit; finding candidates were classified (1 non-blocking PR-metadata observation, 2 withdrawn on counter-evidence); the coverage ledger validated and the temporary ledger was removed. - Unresolved gaps: none. CI was not reviewed per Code Correctness Review scope; this result is code-scope only — merge readiness under this focus says nothing about pending or passing GitHub Actions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
