terrymanu commented on PR #39511: URL: https://github.com/apache/shardingsphere/pull/39511#issuecomment-5336791195
### Result **Review Result: Mergeable** All 20 changed files of PR #39511 (head `347db415dd00`, base `master@dace9b249`) have been reviewed across three behavior clusters: Doris `ADMIN REBALANCE DISK` parsing, Doris CTAS key clause with corrected clause ordering, and the same-actual-table-name test split. All clusters match the official Doris documentation and pass local verification; no code-correctness blocker was found. Two non-blocking observations: the PR description covers only the test-split commit, and the new parse support has no RELEASE-NOTES entry per repository convention. ### Evidence **Cluster 1: ADMIN REBALANCE DISK (12 files)** - The grammar at `parser/sql/engine/dialect/doris/src/main/antlr4/imports/doris/DALStatement.g4:478` defines `ADMIN REBALANCE DISK (ON LP_ string_ (COMMA_ string_)* RP_)?`, which matches the Doris official SQL manual syntax `ADMIN REBALANCE DISK [ ON ( "<host>:<port>" [, ... ] ) ]` item by item; double-quoted backend strings are covered by `string_: DOUBLE_QUOTED_TEXT | SINGLE_QUOTED_TEXT`. - The visitor strips quotes via `SQLUtils.getExactlyValue` (`EXCLUDED_CHARACTERS = "[]'\""`); the bare expected value `192.168.0.1:9050` is verified by the actual IT run. - `SQLVisitorRule.java` only adds the enum constant `ADMIN_REBALANCE_DISK`, consistent with the existing `ADMIN_CLEAN_TRASH` pattern and with no effect on other dialects; `DorisAdminRebalanceDiskStatement` is a new public type, purely additive. **Cluster 2: CTAS key clause (3 files)** - `DDLStatement.g4:39` reorders the optional clauses to ENGINE → key → COMMENT → PARTITION → DISTRIBUTED → PROPERTIES → AS SELECT, matching the Doris official CREATE TABLE manual syntax block; the old grammar placed `duplicateAsQueryExpression` before `duplicatekeyClause`, which contradicts real Doris syntax — this is the root-cause fix. - `duplicatekeyClause` is generalized to `(DUPLICATE | UNIQUE | AGGREGATE) KEY LP_ columnNames RP_`; the official manual confirms all three key types with multi-column lists (example `UNIQUE KEY(k1, k2)`). The `AGGREGATE`/`UNIQUE`/`DUPLICATE` tokens and the `columnNames` rule already exist in the grammar. - The old single-column `DUPLICATE KEY (col)` rule had no existing test coverage, so there is no regression surface; all existing Doris cases pass under the reordered grammar (see below). **Cluster 3: Test split (5 files)** - `ShardingSameActualTableNameMetaDataVisibilityTest`, which conflicted with the mandatory unit-test naming contract, is removed; protection moves back to each owner: `MetaDataLoaderTest` (storage-unit-name stamping), `TableMetadataReviseEngineTest` (storageUnitName argument sourcing), `ShardingRuleTest` (same-actual-name disambiguation across storage units), and `ShardingTableTest` (three `containsDataNode` branches). Together with the existing `ShardingTableNameReviserTest` and `SchemaTableMetaDataAggregatorTest`, the fix chain remains fully covered. **Local runtime verification (all commands exit 0)** - `./mvnw -pl parser/sql/engine/core,parser/sql/engine/dialect/doris,parser/sql/statement/dialect/doris -DskipTests install` (regenerates ANTLR and installs the changed parser modules) - `./mvnw -pl test/it/parser -Dtest=InternalDorisParserIT test`: **Tests run: 1271, Failures: 0, Errors: 0**, including the 3 new admin-rebalance-disk cases and 2 new CTAS cases, and confirming the reordered grammar causes no regression across the 1266 existing Doris cases. - Test-split cluster: focused tests (4 test classes) plus `spotless:apply` and `checkstyle:check` all pass (content byte-identical to the head commit). **Official documentation sources**: [Doris REBALANCE DISK SQL manual](https://doris.apache.org/docs/2.1/sql-manual/sql-statements/table-and-view/data-and-status-management/REBALANCE-DISK/), [Doris CREATE TABLE SQL manual](https://doris.apache.org/docs/3.x/sql-manual/sql-statements/table-and-view/table/CREATE-TABLE/) **Non-blocking observations (not a Change Request)** 1. The PR title and description cover only the test split, while the change set also contains the Doris parser feature commit `1ea7106` (15 files). Update the PR description (or split the PR per community practice) so reviewers cover the full scope. 2. RELEASE-NOTES.md convention adds an entry for new parse support (for example "SQL Parser: Support to parse CREATE MATERIALIZED VIEW for Doris - #31499"); this PR adds none — consider adding one line. ### Coverage - Reviewed head: `347db415dd00` (commits `1ea7106f61b` + `347db415dd0`); the authoritative scope is the GitHub `/pulls/39511/files` list of 20 files, reconciled one-to-one with the local triple-dot list (merge-base `38527d1b84f`). - Behavior clusters: admin-rebalance-disk (12 files), ctas-key-clause (3 files), test-split (5 files); no churn-only files. - All three discovery lenses (root cause and behavior, blast radius and contracts, tests and runtime) completed; every candidate passed through the Finding Proof Gate with no blocker; the convergence pass found zero new candidates and the coverage ledger validated. - Dialect-family check: MySQL trunk and other branch-dialect grammars are untouched; blast radius is limited to Doris; both official documentation pages verified. - No unresolved evidence gaps. CI was not reviewed (Code Correctness Review; the verdict covers code scope only); there are no existing review threads or comments on GitHub. -- 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]
