rishi-rana commented on PR #39882: URL: https://github.com/apache/shardingsphere/pull/39882#issuecomment-6700921063
Thanks, addressed in 1f170620f87. **1. Support contract still needs acceptance** The sub-finding (openGauss-protocol over PostgreSQL storage) is fixed, see point 2 below. The contract itself still needs your sign-off; no further code action possible on my side. **2. Renaming a table to `"dual"` omits the destination metadata refresh** Confirmed and fixed. `deferMetaDataRefresh` now reads the statement's own `TableSQLStatementAttribute` directly instead of `TablesContext#getSimpleTables()`, which drops every table named `DUAL` case-insensitively to exclude the SQL pseudo-table convention — including a real DDL target legitimately named `dual`. Added `assertExecuteDefersMetaDataRefreshForRenameDestinationNamedDual`, verified it fails against the pre-fix code. While fixing this I also closed the gap it pointed at in point 1: `isDeferrableTableDDL` now also checks the session's own protocol type's `isSupportTransactionalDDL()`, not just each routed storage unit's, so an openGauss-protocol session routed to PostgreSQL storage correctly falls through to rejection instead of being admitted. Added `assertExecuteDoesNotDeferMetaDataRefreshWhenProtocolDoesNotSupportTransactionalDDL`, verified it fails against the pre-fix code. **3. Deferred CREATE loses information needed to recover long logical index names** Confirmed and fixed. Extracted `CreateTablePushDownMetaDataRefresher`'s revision-candidate-schema logic into reusable static methods. `deferMetaDataRefresh` now captures the created table's index-name candidates at admission time, carries them through `DeferredMetaDataRefreshContext`, and `ContextManager#reconcileTable` builds the same revision-candidate schema the synchronous CREATE path already builds, instead of always passing `database.getAllSchemas()`. Added a regression test at each layer (`StandardDatabaseProxyConnectorTest` and `ContextManagerTest`), verified both fail against the pre-fix code. **4. Four modified expressions violate the line-wrapping rule** Fixed — all four `DialectTransactionOption` constructions merged onto one line; reconstructed lengths (190/185/197/186 characters) match what you reported, confirming the fix targets the right lines. **5. New exception-propagation tests should use reference-identity assertions** Fixed — both tests now use `sameInstance(...)` instead of `is(...)`. **Verification** 213 targeted unit tests pass across `mode/core`, `proxy/backend/core`, and the four `DialectTransactionOption` dialect modules; `spotless:check checkstyle:check -Pcheck` clean; no line exceeds 200 characters in the diff. -- 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]
