table: don't compare different types of handles #29600
Conversation
Signed-off-by: ekexium <ekexium@gmail.com>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
|
/run-check_dev |
| @@ -136,7 +136,8 @@ func checkHandleConsistency(rowInsertion mutation, indexMutations []mutation, in | |||
| if err != nil { | |||
| return errors.Trace(err) | |||
| } | |||
| if indexHandle.Compare(insertionHandle) != 0 { | |||
| // NOTE: handle type can be different, see issue 29520 | |||
| if indexHandle.IsInt() == insertionHandle.IsInt() && indexHandle.Compare(insertionHandle) != 0 { | |||
There was a problem hiding this comment.
How about the common handle type?
There was a problem hiding this comment.
indexHandle.IsInt() == insertionHandle.IsInt()
will evaluate to
false == false
There was a problem hiding this comment.
Do we need to compare and check if indexHandle.IsCommon() == insertionHandle.IsCommon() ?
There was a problem hiding this comment.
There's no such a method. I think we are only using IsInt() to distinguish handle types.
There was a problem hiding this comment.
I just notice there is an Equal method in the Handle interface which checks the type:
func (ih IntHandle) Equal(h Handle) bool {
return h.IsInt() && int64(ih) == h.IntValue()
}@ekexium Can we use this instead?
In this way the third kind of handle i.e. PartitionHandle (which is not used currently) can also be handled.
There was a problem hiding this comment.
Then the problem is what we should do if Equal returns false. We can't tell if it's type mismatch or not.
There was a problem hiding this comment.
Since we have only two kind of handles that are usable now, I think indexHandle.IsInt() == insertionHandle.IsInt() is enough.
Maybe a better way is to add a method to get the kind, and check something like indexHandle.Kind() == insertionHandle.Kind().
There was a problem hiding this comment.
Seems we check the handle content if it's int type, if it's clustered index handle type or common type do we need to check if the content does match?
There was a problem hiding this comment.
@cfzjywxk The expression indexHandle.IsInt() == insertionHandle.IsInt() checks equality of two bools, rather than AND. Two falses evaluates to true.
There was a problem hiding this comment.
This adds some implicit dependencies that there are two types of handle, as this constraint will not change very much seems it's fine.
ti-chi-bot
added
status/LGT2
guo-shaoge
added
the
needs-cherry-pick-release-5.3
|
@ekexium could you please continue work on this pr to cherry pick to v5.3. Thanks! |
|
cherry pick to release-5.3 failed |
@guo-shaoge The feature is not contained in v5.3, not even in v5.4. Note this PR fixes in a separate feature branch :). |
Got it. Thx! |
ekexium
removed
the
needs-cherry-pick-release-5.3
Signed-off-by: ekexium ekexium@gmail.com
What problem does this PR solve?
Issue Number: close #29520
Problem Summary:
What is changed and how it works?
A workaround: just don't compare different types of handles.
Check List
Tests
Documentation
Release note