menghaoranss commented on PR #38854: URL: https://github.com/apache/shardingsphere/pull/38854#issuecomment-4714739671
### Summary - **Merge Decision: Mergeable** - **Reason:** The PR fixes the single-table default-schema lookup path for mixed protocol/storage database types, keeps the change scoped to the owning Single DistSQL executor, and has targeted regression coverage plus passing scoped verification. ### Evidence - Root-cause fix: `kernel/single/distsql/handler/src/main/java/org/apache/shardingsphere/single/distsql/handler/update/LoadSingleTableExecutor.java:111` now resolves storage unit database types once and passes the storage type into `SingleTableDataNodeLoader.loadSchemaTableNames`, so actual metadata is loaded using the storage database semantics instead of the frontend protocol type. - Default schema compatibility: `LoadSingleTableExecutor.java:67` now calls `database.getDefaultSchemaName()`, which is the existing framework wrapper over `DatabaseTypeRegistry(protocolType).getDefaultSchemaName(databaseName)`, so it preserves the previous protocol default-schema semantics while avoiding duplicate construction logic. - Mixed-type fallback behavior: `LoadSingleTableExecutor.java:120` keeps same-protocol/storage behavior unchanged, and only formats the default schema with the storage database identifier pattern when the protocol type and storage type differ. - Regression coverage: `kernel/single/distsql/handler/src/test/java/org/apache/shardingsphere/single/distsql/handler/update/LoadSingleTableExecutorTest.java:142` adds a mixed protocol/storage database type case where the default schema is formatted to match actual storage metadata. - Existing adjacent paths remain covered: the test class still covers schema-required validation, schema-unsupported validation, duplicate logical table rejection, invalid storage unit rejection, empty actual table nodes, missing actual table, wildcard loading, and rule configuration building. - Scope and ownership: GitHub `/pulls/38854/files` contains only `LoadSingleTableExecutor.java` and `LoadSingleTableExecutorTest.java`; local triple-dot diff for base `f40fd90435ac28ce6eca996ebe7c29cbb3973673` to head `1cd1adcde2e91520c4bae47847a80d0069feb86d` matches those two files. No unrelated tracked changes were found in the PR scope. - Regression risk scan: the changed path is DistSQL `LOAD SINGLE TABLE` validation/loading only; it does not touch SQL parser grammar, shared SPI contracts, dependency metadata, packaging, Proxy protocol handling, or high-frequency Proxy/JDBC DML/DQL execution paths. ### Review Details - **Reviewed Scope:** PR #38854 latest head `1cd1adcde2e91520c4bae47847a80d0069feb86d`; base `f40fd90435ac28ce6eca996ebe7c29cbb3973673`; merge-base `f40fd90435ac28ce6eca996ebe7c29cbb3973673`. Reviewed `kernel/single/distsql/handler/src/main/java/org/apache/shardingsphere/single/distsql/handler/update/LoadSingleTableExecutor.java` and `kernel/single/distsql/handler/src/test/java/org/apache/shardingsphere/single/distsql/handler/update/LoadSingleTableExecutorTest.java`. Local file list matched GitHub `/pulls/38854/files`. - **Not Reviewed Scope:** GitHub Actions/check-run status was intentionally not reviewed. No linked issue was identified in the PR body, so linked-issue completeness was not applicable. - **Verification:** `./mvnw -pl kernel/single/distsql/handler -am -DskipITs -Dspotless.skip=true -Dtest=LoadSingleTableExecutorTest -DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false test` passed with 15 tests. `./mvnw -pl kernel/single/distsql/handler -Pcheck spotless:check -DskipTests -T1C` passed. `./mvnw -pl kernel/single/distsql/handler -Pcheck checkstyle:check -DskipTests -T1C` passed. `./mvnw -pl kernel/single/distsql/handler -am -DskipITs -Dspotless.skip=true -DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false test` passed with 55 tests. - **Release Note / User Docs:** Not required. This is a narrow internal bug fix for Single DistSQL validation/loading behavior that restores expected default-schema matching and does not add configuration, syntax, API/SPI, migration, or user workflow changes. -- 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]
