terrymanu commented on PR #39512: URL: https://github.com/apache/shardingsphere/pull/39512#issuecomment-5337167348
### Result **Review Result: Mergeable** The PR adds RECOVER and SHOW TRANSACTION statement parsing for the Doris dialect. The grammar matches the official documentation, it reuses the established statement/visitor/assert patterns, the dispatch registration is purely additive, and the 13 new IT cases cover every grammar branch and pass on the PR head. No blocking issues were found. (This verdict covers code scope only; CI was not reviewed.) ### Evidence - **Grammar vs. official docs**: `parser/sql/engine/dialect/doris/src/main/antlr4/imports/doris/DALStatement.g4:454` (three recover alternatives: DATABASE/TABLE/PARTITION, with optional id and AS rename) and `:522` (`SHOW TRANSACTION fromDatabase? showWhereClause?`) align with the Doris 2.1/3.x official RECOVER grammar block and the SHOW TRANSACTION grammar block; the 3.x page was re-verified and the structure is identical. The official docs self-contradict on the PARTITION alias position (grammar block puts AS after FROM, example 9 puts it before FROM); the PR accepts both orders, a reasonable tolerant superset. - **Entry and dispatch**: `DorisStatement.g4:178` mounts `| recover`; `SQLVisitorRule.java:610-616` registers 4 DAL entries. All dialect grammars were searched: apart from Hive's `showTransactions` (plural), no same-named rules exist, so no other dialect can produce these context names — the additive registration has no cross-dialect impact. - **Dialect family**: Doris is a branch dialect of the MySQL trunk. Neither the MySQL nor the MariaDB grammar contains RECOVER/SHOW TRANSACTION statement rules (searched), the PR does not touch trunk files, and the same root cause is not applicable to other dialects. - **Behavior modeling**: the 5 visit methods at `DorisDALStatementVisitor.java:1763-1810` follow the `DorisShowLoadStatement`/`DorisBackupStatement` precedents; `DorisRecoverStatement` and `DorisShowTransactionStatement` are dialect-private DAL models with no public API or SPI change. - **Local verification (against PR head e8abb58e74dc, clean worktree)**: `InternalDorisParserIT` exited 0 with Tests run: 1284, Failures: 0, Errors: 0 (including the 13 new cases, whose expected indices all match the actual parse output); `spotless:apply` and `checkstyle:check -Pcheck` also exited 0 on identical content during implementation. ### Coverage - **Reviewed candidate**: PR head `e8abb58e74dc` (local HEAD equals it; merge-base = base = `8c2649db8d5`; local triple-dot file list matches the GitHub list of 16 files/+647 exactly, with no differences). - **Behavior clusters**: 3 — RECOVER grammar and model (2 g4 files, visitor, statement, 2 IT resource files), SHOW TRANSACTION grammar and model (isomorphic), and shared dispatch registration (SQLVisitorRule) plus the IT assertion chain (3 asserts, 3 JAXB classes, RootSQLParserTestCases). All 16 authoritative files were accounted for; no churn-only files. - **Discovery lenses**: root cause and behavior, blast radius and contracts, and tests/runtime all completed; the convergence pass found 0 new independent candidates. Candidate classification: 2 non-blocking (the tolerant dual-position AS superset and the optional WHERE per repo convention — neither requires a change, so they are not listed in the result) and 1 cross-dialect clash candidate withdrawn after counter-evidence. - **Documentation evidence**: Doris official SQL manual pages for RECOVER (2.1 and 3.x) and SHOW TRANSACTION (3.x); repo precedent (peer parser PRs such as #38248 do not update release notes). - **Unresolved gaps**: none. CI scope: not reviewed (Code Correctness Review does not query 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]
