github-actions[bot] commented on code in PR #61518:
URL: https://github.com/apache/doris/pull/61518#discussion_r2968388599
##########
be/src/io/cache/fs_file_cache_storage.cpp:
##########
@@ -197,7 +197,12 @@ Status FSFileCacheStorage::finalize(const FileCacheKey&
key, const size_t size)
}
BlockMetaKey mkey(key.meta.tablet_id, UInt128Wrapper(key.hash),
key.offset);
- BlockMeta meta(key.meta.type, size, key.meta.expiration_time);
+ uint64_t context_id = 0;
+ if (_meta_store) {
+ context_id =
+ _meta_store->get_or_create_context_id(key.meta.table_name,
key.meta.partition_name);
+ }
+ BlockMeta meta(key.meta.type, size, key.meta.expiration_time, context_id);
_meta_store->put(mkey, meta);
Review Comment:
**Bug (inconsistent null guard):** Line 201 guards
`get_or_create_context_id` with `if (_meta_store)`, but this line calls
`_meta_store->put()` unconditionally. If `_meta_store` can be null (which the
guard implies), this is a null pointer dereference. Either remove the guard on
line 201 (since `_meta_store` is always initialized in `init()`) or also guard
this call:
```cpp
if (_meta_store) {
_meta_store->put(mkey, meta);
}
```
##########
be/src/vec/exec/scan/olap_scanner.cpp:
##########
@@ -70,35 +70,15 @@ using ReadSource = TabletReadSource;
OlapScanner::OlapScanner(pipeline::ScanLocalStateBase* parent,
OlapScanner::Params&& params)
: Scanner(params.state, parent, params.limit, params.profile),
_key_ranges(std::move(params.key_ranges)),
- _tablet_reader_params({.tablet = std::move(params.tablet),
- .tablet_schema {},
- .aggregation = params.aggregation,
Review Comment:
**Style:** This uses a C-style cast
`(pipeline::OlapScanLocalState*)_local_state`. The same file uses `static_cast`
in other places (e.g., line 111). Please use
`static_cast<pipeline::OlapScanLocalState*>(_local_state)` for type safety.
##########
be/src/io/cache/cache_block_meta_store.cpp:
##########
@@ -42,6 +43,83 @@
namespace doris::io {
const std::string FILE_CACHE_META_COLUMN_FAMILY = "file_cache_meta";
+const std::string FILE_CACHE_CONTEXT_DICT_COLUMN_FAMILY =
"file_cache_context_dict";
+
+namespace {
+
+constexpr char kContextLookupPrefix = 'K';
+constexpr char kContextIdPrefix = 'I';
+
+void append_u32(std::string* out, uint32_t value) {
+ out->push_back(static_cast<char>((value >> 24) & 0xff));
+ out->push_back(static_cast<char>((value >> 16) & 0xff));
+ out->push_back(static_cast<char>((value >> 8) & 0xff));
+ out->push_back(static_cast<char>(value & 0xff));
+}
+
+void append_u64(std::string* out, uint64_t value) {
+ for (int shift = 56; shift >= 0; shift -= 8) {
+ out->push_back(static_cast<char>((value >> shift) & 0xff));
+ }
+}
+
+std::optional<uint32_t> decode_u32(std::string_view* in) {
+ if (in->size() < sizeof(uint32_t)) {
+ return std::nullopt;
+ }
+ const auto* data = reinterpret_cast<const uint8_t*>(in->data());
+ uint32_t value = (static_cast<uint32_t>(data[0]) << 24) |
+ (static_cast<uint32_t>(data[1]) << 16) |
+ (static_cast<uint32_t>(data[2]) << 8) |
static_cast<uint32_t>(data[3]);
+ in->remove_prefix(sizeof(uint32_t));
+ return value;
+}
+
+std::optional<uint64_t> decode_u64(std::string_view* in) {
+ if (in->size() < sizeof(uint64_t)) {
+ return std::nullopt;
+ }
+ const auto* data = reinterpret_cast<const uint8_t*>(in->data());
+ uint64_t value = 0;
+ for (size_t i = 0; i < sizeof(uint64_t); ++i) {
+ value = (value << 8) | static_cast<uint64_t>(data[i]);
+ }
Review Comment:
**Fragile rollback:** On write failure, this rolls back `_next_context_id`
with `store()`. This is only correct because the entire allocate+write path is
serialized under `_context_mutex`. However, `_next_context_id` is a
`std::atomic` which implies it might be accessed concurrently. If a second
thread called `fetch_add` between line 470 and this rollback, the `store` would
overwrite the second thread's allocation.
Consider either:
1. Documenting that `_next_context_id` is only modified under
`_context_mutex`, or
2. Using `compare_exchange_strong` for a safer rollback:
`_next_context_id.compare_exchange_strong(expected, context_id)` where
`expected = context_id + 1`.
--
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]
