rishi-rana commented on PR #39660: URL: https://github.com/apache/shardingsphere/pull/39660#issuecomment-5528842930
Thanks for the thorough review — both points fixed: 1. **Stale table entry on case-only ALTER**: added `findStaleCaseChangedTables()` to `AlterMaskRuleExecutor.buildToBeDroppedRuleConfiguration()`, which now returns the original-cased table entry for deletion when its persisted name differs in case from the altered name (using a `CaseInsensitiveMap` lookup), so it's removed via `removeRuleConfigurationItem()` after the new-cased entry is persisted. An identical-case alter (the common path) is left untouched — confirmed the existing `assertBuildToBeDroppedRuleConfiguration` test (same case throughout) still passes, so the persist-then-delete-newly-altered-entry regression I was worried about doesn't happen. 2. **Cleanup test didn't distinguish the fix**: reworked `assertBuildToBeDroppedRuleConfigurationWithCaseChangedTableName` (renamed) to use distinct old/new algorithm names, and it now asserts both the stale table (`t_order`) and its orphaned algorithm (`old_order_mask`) are selected for deletion. I verified this test fails against the pre-fix implementation (reverted the production file locally, confirmed `Expected: is <1> but: was <0>`, then restored the fix) before pushing. Local `checkstyle:check -Pcheck` and targeted unit tests both pass. Let me know if there's anything else. -- 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]
