Copilot commented on code in PR #67013:
URL: https://github.com/apache/doris/pull/67013#discussion_r3826849785
##########
cloud/src/meta-store/blob_message.cpp:
##########
@@ -40,6 +41,15 @@ static std::vector<std::string_view> split_string(const
std::string_view& str, i
return substrings;
}
+std::string encode_blob_key(std::string_view origin_key, uint8_t version,
size_t sequence) {
+ std::string split_key(origin_key);
+ int64_t suffix = version;
+ suffix <<= 56;
+ suffix += sequence;
+ encode_int64(suffix, &split_key);
+ return split_key;
Review Comment:
`encode_blob_key()` builds an `int64_t` suffix from an unconstrained
`uint8_t version`. When `version == 0x80` and `sequence == 0`, `suffix` becomes
`INT64_MIN`. `encode_int64()` currently does `v = val < 0 ? -val : val;`
(cloud/src/meta-store/codec.cpp:94-97), so passing `INT64_MIN` triggers signed
overflow/UB and can produce an invalid key.
##########
cloud/src/meta-store/blob_message.h:
##########
@@ -62,6 +66,30 @@ struct ValueBuf {
int8_t ver {-1};
};
+inline constexpr size_t DEFAULT_BLOB_SPLIT_SIZE = 90 * 1000;
+
+/**
+ * Encode a split KV key.
+ * @param origin_key logical key
+ * @param version value encoding version
+ * @param sequence zero-based split KV index
+ * @return encoded split KV key
+ */
+std::string encode_blob_key(std::string_view origin_key, uint8_t version,
size_t sequence);
+
+/**
+ * Decode a split KV key.
+ * @param raw_key encoded split KV key
+ * @param origin_key optional logical key output
+ * @param version value encoding version output, must not be nullptr
+ * @param sequence zero-based split KV index output, must not be nullptr
+ * @param fields optional decoded origin-key fields; non-null enables
validation
+ * @return true on success
+ */
+bool decode_blob_key(
+ std::string_view raw_key, std::string* origin_key, uint8_t* version,
uint16_t* sequence,
+ std::vector<std::tuple<std::variant<int64_t, std::string>, int, int>>*
fields = nullptr);
Review Comment:
The blob-key API isn’t round-trippable for `sequence` values > `UINT16_MAX`:
`encode_blob_key()` accepts `size_t sequence`, but `decode_blob_key()` returns
`uint16_t` (and the implementation truncates with `suffix & 0xffff`). This also
conflicts with the documented suffix layout where bytes 1–5 are “dummy”.
Consider making `sequence` consistently `uint16_t` everywhere (and rejecting
oversized blobs), or widening the decoded type and validating/using the full
field.
##########
cloud/src/meta-store/blob_message.cpp:
##########
@@ -240,31 +246,47 @@ void BlobIterator::load_current_blob() {
}
}
-bool BlobIterator::extract_origin_key(std::string_view raw_key, std::string*
output,
- uint8_t* version, uint16_t* sequence) {
- // The suffix is 8 bytes: |version(1)|dummy(5)|sequence(2)|
+bool decode_blob_key(
+ std::string_view raw_key, std::string* origin_key, uint8_t* version,
uint16_t* sequence,
+ std::vector<std::tuple<std::variant<int64_t, std::string>, int, int>>*
fields) {
if (raw_key.size() < 9) {
- LOG_WARNING("failed to extract origin key").tag("key", hex(raw_key));
- error_code_ = TxnErrorCode::TXN_INVALID_DATA;
return false;
}
Review Comment:
`decode_blob_key()` documents `version`/`sequence` as non-null, but
immediately dereferences them without an assertion. Adding
`DCHECK`/`DORIS_CHECK` makes contract violations fail fast with a clear signal
instead of an obscure crash.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
