terrymanu commented on PR #39517:
URL: https://github.com/apache/shardingsphere/pull/39517#issuecomment-5342844071
### Result
**Review Result: Mergeable**
PR #39517 (head `c99fb61`) correctly implements end-to-end parsing support
for the two remaining Doris statement groups from issue #31483. The grammar
matches the official Doris documentation, all changes are purely additive, and
no blocking issues were found. This conclusion is code-scope only (Code
Correctness Review); CI was not reviewed.
### Evidence
- **Grammar matches official documentation**:
`parser/sql/engine/dialect/doris/src/main/antlr4/imports/doris/DALStatement.g4:484`
defines `adminCheckTablet: ADMIN CHECK TABLET LP_ NUMBER_ (COMMA_ NUMBER_)*
RP_ propertiesClause`, which corresponds item by item to `ADMIN CHECK TABLET (
<tablet_id> [,...] ) PROPERTIES("type" = "<type_value>")` from
apache/doris-website master
`docs/sql-manual/sql-statements/table-and-view/data-and-status-management/ADMIN-CHECK-TABLET.md`
(at least one tablet id, PROPERTIES required). `DALStatement.g4:488` defines
`adminSetPartitionVersion: ADMIN SET TABLE tableName PARTITION VERSION
propertiesClause`, matching the syntax in `SET-TABLE-PARTITION-VERSION.md` from
the same directory; per official semantics the partition id and version live in
PROPERTIES and the SQL carries only the table name, which the model reflects
correctly.
- **Reuse and pattern consistency**: the two visitor methods reuse the
existing `extractPropertiesSegment` helper and the `(SimpleTableSegment)
visit(ctx.tableName())` pattern; the statement classes, asserts, and JAXB
structures align fully with the same-family precedent PR #38349 (ADMIN SET
REPLICA / ADMIN COPY TABLET).
- **No dialect-family blast radius**: Doris is a branch dialect of the MySQL
trunk; every change in this PR sits in Doris-only files and touches no MySQL
trunk grammar. `SQLVisitorRule` is a shared enum, but the new
`ADMIN_CHECK_TABLET` and `ADMIN_SET_PARTITION_VERSION` entries reference rule
names that exist only in the DorisStatement grammar, so other dialects are
unaffected. All required keywords
(CHECK/TABLET/SET/TABLE/PARTITION/VERSION/PROPERTIES) already exist, so no new
lexer tokens and no reserved-word shadowing risk exist.
- **Local verification passed** (against a working tree identical to the PR
head): `InternalDorisParserIT` Tests run: 1287, Failures: 0 (exit 0); module
tests for `parser/sql/statement/dialect/doris` and
`parser/sql/engine/dialect/dialect` engine module exited 0; `spotless:apply
-Pcheck` and `checkstyle:check -Pcheck` both exited 0.
- **Test validity**: the `admin_set_partition_version_with_qualified_table`
case covers qualified table name parsing, and its `SimpleTableSegment`
start-index counting from the owner (16–42) follows the established
`show-data.xml` precedent semantics; property assertions include keys, values,
and zero-based inclusive indices, protecting the parsing behavior itself.
### Coverage
- Reviewed scope: PR head `c99fb616524`, base `9fdc12bd541` (master). All 18
files from the authoritative GitHub changed-file list were individually
reviewed and mapped to behavior clusters: the ADMIN CHECK TABLET parsing chain,
the ADMIN SET TABLE PARTITION VERSION parsing chain, registration wiring
(SQLVisitorRule / DorisDALStatementAssert dispatch / RootSQLParserTestCases),
and an unrelated configuration change.
- All three discovery lenses (root cause and behavior, blast radius and
contracts, tests and runtime) completed; the convergence pass produced no new
independent candidates.
- Non-blocking scope note:
`test/e2e/env/src/test/resources/container/mysql/cnf/5/my.cnf` and
`cnf/8/my.cnf` (`max_connections` 600→2000) are unrelated to the PR's declared
scope (Fixes #31483, Doris parsing) and constitute a piggybacked E2E
environment tweak. It does not affect correctness and is not blocking, but the
path filter will trigger E2E CI jobs that would otherwise not run for a
parser-only change; the author may keep or split it at discretion.
- The local repository holds no git ref for the PR head (not fetched), so
the ledger/inventory scripts that depend on local refs were not run; per-file
manual accounting against the GitHub file list was completed instead (18/18).
- Cross-checked against issue #31483: the other four SQL groups (DROP USER,
FLOAT, split_by_string, ALTER VIEW) already parse under the existing grammar,
so once this PR merges all items in the issue are complete.
- CI was 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]
