checker(dm): improve same table precheck error#12729
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Code Review
This pull request introduces a pre-check (sameTargetTableNameDetectionForRouteRules) to detect case-insensitive target table name conflicts in route rules before the table router is initialized. This prevents confusing "matches more than one rule" errors from being thrown by regexprrouter during precheck. Feedback on the changes suggests changing the condition else if nameO != name to a simple else to ensure that duplicate or colliding rules mapping to the exact same target table name (including case) are also caught and reported.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if nameO, ok := tableNameSets[nameL]; !ok { | ||
| tableNameSets[nameL] = name | ||
| } else if nameO != name { | ||
| messages = append(messages, fmt.Sprintf("same target table %v vs %s", nameO, name)) | ||
| } |
There was a problem hiding this comment.
If two route rules collide under case-insensitive matching but map to the exact same target table name (including case), nameO == name will be true. Because of the else if nameO != name condition, this collision will not be detected by this preflight check, and the user will still encounter the confusing matches more than one rule error from regexprrouter.
Changing this to a simple else ensures that any duplicate or colliding rules for the same source table pattern are caught and reported with a clear error message.
| if nameO, ok := tableNameSets[nameL]; !ok { | |
| tableNameSets[nameL] = name | |
| } else if nameO != name { | |
| messages = append(messages, fmt.Sprintf("same target table %v vs %s", nameO, name)) | |
| } | |
| if nameO, ok := tableNameSets[nameL]; !ok { | |
| tableNameSets[nameL] = name | |
| } else { | |
| messages = append(messages, fmt.Sprintf("same target table %v vs %s", nameO, name)) | |
| } |
There was a problem hiding this comment.
Thanks, this is reasonable. Updated the preflight to catch any case-insensitive source-rule collision, including collisions that map to the exact same target table name, and added regression coverage for that case.
563f2d1 to
fd049f8
Compare
What problem does this PR solve?
Issue Number: close #12737
DM precheck can report a confusing raw table-router error when route rules only differ by table-name case under case-insensitive matching, for example:
This hides the existing clearer same-target-table diagnostic.
What is changed and how it works?
Add a checker-side preflight for case-insensitive route rules before building the regular-expression table router.
The preflight detects source table patterns that collide after case folding and map to target tables that differ only by case, then returns the existing
ErrTaskCheckSameTableNameerror with actionable details such as:This preserves the clearer DM precheck error path and avoids exposing the router's ambiguous duplicate-rule message.
Check List
Tests
Commands run:
Questions
Will it cause performance regression or break compatibility?
No. The change only adds a lightweight route-rule validation step during checker initialization and reuses the existing same-table-name error type.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note