-
Notifications
You must be signed in to change notification settings - Fork 412
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
[FLASH-348] Add TMT Property To DataPart #121
Conversation
{ | ||
storage.data.delayInsertIfNeeded(); | ||
|
||
Row partition(1, Field(UInt64(partition_id))); | ||
Block block_copy = block; |
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.
block_copy = block
is not bad, CH used it. block assignment is a small operating
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 also keep the TxnMergeTreeBlockOutputStream::write(const Block & block)
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 think we don't need to do move(block)
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.
just provide another method.
inline TiKVKey encodeAsTiKVKey(const String & ori_str) | ||
{ | ||
std::stringstream ss; | ||
encodeAsTiKVKey(ori_str, ss); | ||
EncodeBytes(ori_str, ss); |
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.
Start with lowercase
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.
In file storage/ch/dbms/src/Storages/Transaction/Codec.h
, most function starts with uppercase. Maybe need to change all of them.
return p == 0; | ||
} | ||
|
||
inline std::tuple<std::string, size_t> decodeTiKVKeyFull(const TiKVKey & 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.
Need another reviewer to double check this function's modification
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.
There is a tiny bug in EncodeBytes function when I wrote ddl branch. See https://github.com/pingcap/tics/pull/110/files#diff-56cb4a63b35a7383087298827d9269d5R296 |
/run-integration-tests |
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.
LGTM
Signed-off-by: guo-shaoge <shaoge1994@163.com>
add min max pk.
based on #119