terrymanu commented on PR #39524: URL: https://github.com/apache/shardingsphere/pull/39524#issuecomment-5345450140
### Result **Review Result: Mergeable** PR #39524 (head `1f7e6bae6a3`, 5 files, +133/−4) correctly implements parsing of the five Oracle CREATE TABLE statements required by issue #27096, with no blocking issues (this conclusion is code-scope only; CI was not reviewed). ### Evidence - Issue coverage: the PR adds five IT cases (`create_table_with_clob`, `create_table_with_fields_rtrim`, `create_table_with_fields_ltrim`, `create_table_with_records_delimited_by`, `create_table_with_records_fixed`) mapped one-to-one to the five SQL statements listed in the issue; all four external-table statements were verified to fail parsing before the change and pass after it, providing real regression protection. - Grammar correctness: the parenthesized `ACCESS PARAMETERS` body (`parenthesizedOpaqueFormatSpec`/`recordFormatInfo`/`fieldList`/`fieldDefinition`/`positionSpecification`) matches the official Oracle [ORACLE_LOADER Access Driver](https://docs.oracle.com/en/database/oracle/oracle-database/23/sutil/oracle_loader-access-driver.html) documentation example-by-example (`RECORDS FIXED 20 FIELDS (...)`, `RECORDS DELIMITED BY '|' FIELDS TERMINATED BY ',' (...)`, `FIELDS LTRIM`, `FIELDS RTRIM (...)`, `(start:end)` position specs with `*±n`); the parenthesized access parameters body and quoted LOCATION are also the forms required by the official docs. - Compatibility: the four new tokens `RECORDS`/`DELIMITED`/`FIXED`/`FIELDS` are all added to `unreservedWord` (`parser/sql/engine/dialect/oracle/src/main/antlr4/imports/oracle/BaseRule.g4:85`), so these words remain usable as plain identifiers; no token prefix conflicts (`RECORDS` vs `RECORDS_PER_BLOCK` disambiguated by longest match). The legacy unparenthesized `ACCESS PARAMETERS` form is retained, so the existing case `create_table_with_enclosed` is unaffected; relaxing the `LOCATION` list from `(COMMA_...)+` to `*` only widens acceptance and cannot reject previously valid input; `locationSpecifier` accepting `STRING_` is likewise additive for the DML path (`DMLStatement.g4:564`). - Local verification (sanitized summary; local HEAD is the same commit as the PR head `1f7e6bae6a3`): oracle dialect module tests exit 0; `InternalOracleParserIT` Tests run: 1841, Failures: 0, Errors: 0, exit 0; direct parse of the five SQLs 5/5 OK; spotless:apply and checkstyle:check both exit 0. ### Coverage - Reviewed head: `1f7e6bae6a3` (GitHub changed-file list matches the local triple-dot scope exactly; merge-base `743996c9e75`). - File accounting: all three grammar files (lexer tokens / rule extension / LOCATION relaxation) and both test resource files were reviewed by behavior cluster (lexer keywords, ACCESS PARAMETERS rules, LOCATION, test cases); no churn-only files. - Dialect scope: target dialect Oracle (a trunk dialect with no branch dialect copying its grammar; MariaDB/Doris branch MySQL only); other dialects' lexers and cases are unaffected. - Official documentation: Oracle Database Utilities ORACLE_LOADER Access Driver chapter (syntax forms) and the external tables concepts chapter (parenthesized `ACCESS PARAMETERS` body, quoted `LOCATION` form). - All three discovery lenses (root cause and behavior / blast radius and contracts / tests, runtime, and operations) are complete; the convergence pass found zero new independent candidates; no unresolved evidence gaps remain. - Non-blocking observation: the PR contains no `RELEASE-NOTES.md` entry, while similar merged PRs (#39292, #39302) added one; consider adding `1. SQL Parser: Support parsing Oracle CREATE TABLE with ORGANIZATION EXTERNAL clause - [#39524](https://github.com/apache/shardingsphere/pull/39524)` before merge (does not affect the code-correctness conclusion). - This is a Code Correctness Review; CI was not queried, awaited, or reported. -- 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]
