-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expression: add builtin function tidb_decode_key()
#12193
Conversation
@winkyao PTAL |
Codecov Report
@@ Coverage Diff @@
## master #12193 +/- ##
===========================================
Coverage 81.1245% 81.1245%
===========================================
Files 454 454
Lines 99351 99351
===========================================
Hits 80598 80598
Misses 12950 12950
Partials 5803 5803 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If tidb_decode_key()
failed to decode the input, IMHO it's better to return a warning to the client, for example, given the following query:
>select region_id, tidb_decode_key(start_key) as k from `TIKV_REGION_STATUS` where tidb_decode_key(start_key) like 'tableID=%' limit 3
If the decoding failed for some unexpected reason, the end-user sees nothing result. In such cases, a warning can remind the user that something is wrong because of the failed decodes, rather than the real result does not exist actually.
@crazycs520 Please supplement the |
tidb_decode_key()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe tidb_decode_region_key
is preciser, although tidb_decode_key()
is simpler,
Co-Authored-By: Zhang Jian <[email protected]>
Co-Authored-By: Zhang Jian <[email protected]>
Co-Authored-By: Zhang Jian <[email protected]>
@zz-jason , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Your auto merge job has been accepted, waiting for 12254 |
/run-all-tests |
Co-Authored-By: Zhang Jian <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
// Try to decode it as a record key. | ||
tableID, handle, err := tablecodec.DecodeRecordKey(key) | ||
if err == nil { | ||
return "tableID=" + strconv.FormatInt(tableID, 10) + ", _tidb_rowid=" + strconv.FormatInt(handle, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we don't return json object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change this tomorrow.
What problem does this PR solve?
tidb_decode_key()
function to decode key.Related Parser PR: pingcap/parser#560
What is changed and how it works?
Other discuss
The decoded key format currently maybe not very well, If you have any suggestions, Please comment, Thanks very much.
Maybe we can use the format in
show table region.
Check List
Tests
Code changes
Side effects
Related changes
Release note
tidb_decode_key
build-in function to decode the key of TiDB used.