terrymanu commented on PR #39588:
URL: https://github.com/apache/shardingsphere/pull/39588#issuecomment-5387135702
### Result
**Review Result: Mergeable**
The last commit (`900b910a672a`, "Support parsing Doris ADMIN SET TABLE
STATUS syntax") correctly adds Doris `ADMIN SET TABLE STATUS` parsing end to
end with no behavioral or contract risk found. The grammar matches the official
Doris syntax, the visitor and statement model follow the established
`DorisAdmin*Statement` pattern, and the new parser IT cases protect the owned
behavior. This result is code-scope only and covers only the last commit as
requested.
### Evidence
- Official syntax: the Doris official statement reference (SET TABLE STATUS,
`sql-manual/sql-statements/table-and-view/data-and-status-management/SET-TABLE-STATUS`)
defines `ADMIN SET TABLE <table_name> STATUS PROPERTIES ("key" = "value")`.
The new rule `adminSetTableStatus : ADMIN SET TABLE tableName STATUS
propertiesClause` in
`parser/sql/engine/dialect/doris/src/main/antlr4/imports/doris/DALStatement.g4:512`
matches this form exactly, and IT case 1 reuses the documented example
verbatim.
- Grammar safety: the rule shares the `ADMIN SET TABLE tableName` prefix
with the existing `adminSetPartitionVersion` but disambiguates on the following
token (`STATUS` vs `PARTITION`), so no existing alternative (`adminSetRepair`,
`adminCleanTrash`, `adminSetReplica*`) changes path. The `STATUS` token already
exists in `DorisKeyword.g4` and is reused by `showTableStatus`.
- Dialect family: Doris is a branch dialect of the MySQL trunk; the
statement is Doris-specific, so no trunk (MySQL) or sibling (MariaDB) grammar
change is required. The Doris-only additions are marked with `// DORIS ADDED
BEGIN/END` in `DALStatement.g4` and `DorisDALStatementVisitor.java:1427`.
- Model and dispatch: `DorisAdminSetTableStatusStatement` mirrors
`DorisAdminSetPartitionVersionStatement` (final table + properties),
`SQLVisitorRule.ADMIN_SET_TABLE_STATUS` follows the sibling enum convention,
and `propertiesClause` is mandatory in the grammar, so the visitor never sees a
null properties clause.
- Test validity:
`test/it/parser/src/main/resources/sql/supported/dal/admin-set-table-status.xml`
plus the assertion XML cover a simple table (documented example) and a
qualified table (`example_db.tbl2`) with two properties, asserting table name,
owner, and property key/value with exact indices. No unsupported-case resource
contains a conflicting `ADMIN SET TABLE` statement.
- Verification at public head `900b910a672a` (local HEAD equals PR head,
clean tree, 11 files match the GitHub commit file list): module tests for
`parser/sql/engine/core`, `parser/sql/engine/dialect/doris`,
`parser/sql/statement/dialect/doris` — exit 0; `InternalDorisParserIT` — Tests
run: 1379, Failures: 0, exit 0; `spotless:apply -Pcheck` and `checkstyle:check
-Pcheck` — exit 0.
### Coverage
- Reviewed head: `900b910a672a` (PR #39588 head). Per explicit request, only
the last commit was reviewed; the first PR commit `0a0592ba638` (ADMIN CANCEL
REBALANCE DISK, YEARWEEK, RTRIM) was out of scope for this review.
- All 11 changed files are accounted for and map to three behavior clusters:
grammar acceptance (2 g4 files), parse-result model and dispatch (visitor,
`SQLVisitorRule`, new statement class), and IT/assert/JAXB wiring (2 new XML
resources, new assert class, new JAXB test case, 3 registrations).
- Discovery lenses completed: root cause and behavior, blast radius and
contracts, tests and runtime. Two non-blocking candidates (a duplicated private
`assertProperty` helper shared with sibling assert classes, and keyword-named
edge tables such as a table literally named `status`) were classified as repo
convention and pre-existing pattern respectively and are not published as
required changes.
- No unresolved evidence gap remains for this commit scope. CI was not
reviewed; this verdict is code-scope only.
--
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]
