From 3dc33f37beed819e260847d683c966fc9a1cb3e2 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Thu, 17 Apr 2025 16:46:59 -0300 Subject: [PATCH 01/46] draft immpl --- programs/server/Server.cpp | 5 + src/Common/TTLCachePolicy.h | 4 +- src/Core/ServerSettings.cpp | 6 +- src/Core/Settings.cpp | 3 + .../Cache/ObjectStorageListObjectsCache.cpp | 228 ++++++++++++++++++ .../Cache/ObjectStorageListObjectsCache.h | 69 ++++++ .../StorageObjectStorageSource.cpp | 138 ++++++++++- .../StorageObjectStorageSource.h | 5 +- 8 files changed, 447 insertions(+), 11 deletions(-) create mode 100644 src/Storages/Cache/ObjectStorageListObjectsCache.cpp create mode 100644 src/Storages/Cache/ObjectStorageListObjectsCache.h diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 59a8598b2137..b07b51c46522 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -83,6 +83,7 @@ #include #include #include +#include #include #include #include @@ -2327,6 +2328,10 @@ try if (dns_cache_updater) dns_cache_updater->start(); + ObjectStorageListObjectsCache::instance().setMaxSizeInBytes(server_settings[ServerSetting::object_storage_list_objects_cache_size]); + ObjectStorageListObjectsCache::instance().setMaxCount(server_settings[ServerSetting::object_storage_list_objects_cache_max_entries]); + ObjectStorageListObjectsCache::instance().setTTL(server_settings[ServerSetting::object_storage_list_objects_cache_ttl]); + auto replicas_reconnector = ReplicasReconnector::init(global_context); ParquetFileMetaDataCache::instance()->setMaxSizeInBytes(server_settings[ServerSetting::input_format_parquet_metadata_cache_max_size]); diff --git a/src/Common/TTLCachePolicy.h b/src/Common/TTLCachePolicy.h index 6c548e5042bd..b38bcc522e56 100644 --- a/src/Common/TTLCachePolicy.h +++ b/src/Common/TTLCachePolicy.h @@ -243,10 +243,10 @@ class TTLCachePolicy : public ICachePolicy; Cache cache; - +private: /// TODO To speed up removal of stale entries, we could also add another container sorted on expiry times which maps keys to iterators /// into the cache. To insert an entry, add it to the cache + add the iterator to the sorted container. To remove stale entries, do a /// binary search on the sorted container and erase all left of the found key. diff --git a/src/Core/ServerSettings.cpp b/src/Core/ServerSettings.cpp index 32de4c70d8e9..b9dd72418e5f 100644 --- a/src/Core/ServerSettings.cpp +++ b/src/Core/ServerSettings.cpp @@ -1004,8 +1004,10 @@ namespace DB ``` )", 0) \ DECLARE(Bool, storage_shared_set_join_use_inner_uuid, false, "If enabled, an inner UUID is generated during the creation of SharedSet and SharedJoin. ClickHouse Cloud only", 0) \ - DECLARE(UInt64, input_format_parquet_metadata_cache_max_size, 500000000, "Maximum size of parquet file metadata cache", 0) \ - + DECLARE(UInt64, input_format_parquet_metadata_cache_max_size, 500000000, "Maximum size of parquet file metadata cache", 0) \ + DECLARE(UInt64, object_storage_list_objects_cache_size, 5000000, "Maximum size of ObjectStorage list objects cache in bytes. Zero means disabled.", 0) \ + DECLARE(UInt64, object_storage_list_objects_cache_max_entries, 1000, "Maximum size of ObjectStorage list objects cache in entries. Zero means disabled.", 0) \ + DECLARE(UInt64, object_storage_list_objects_cache_ttl, 3600, "Time to live of records in ObjectStorage list objects cache in seconds. Zero means unlimited", 0) \ // clang-format on /// If you add a setting which can be updated at runtime, please update 'changeable_settings' map in dumpToSystemServerSettingsColumns below diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 404442216317..558ff43852ab 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -6108,6 +6108,9 @@ Limit for hosts used for request in object storage cluster table functions - azu Possible values: - Positive integer. - 0 — All hosts in cluster. +)", EXPERIMENTAL) \ + DECLARE(Bool, use_object_storage_list_objects_cache, true, R"( +Cache the list of objects returned by list objects calls in object storage )", EXPERIMENTAL) \ \ /* ####################################################### */ \ diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp new file mode 100644 index 000000000000..8e78e1b94a5c --- /dev/null +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -0,0 +1,228 @@ +#include + +namespace DB +{ + +template +class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy +{ +public: + using BasePolicy = TTLCachePolicy; + using typename BasePolicy::MappedPtr; + using typename BasePolicy::KeyMapped; + using BasePolicy::cache; + + ObjectStorageListObjectsCachePolicy() + : BasePolicy(std::make_unique()) + { + } + + MappedPtr get(const Key & key) override + { + if (const auto it = cache.find(key); it != cache.end()) + { + if (IsStaleFunction()(it->first)) + { + BasePolicy::remove(it->first); + return {}; + } + return it->second; + } + + if (const auto it = findBestMatchingPrefix(key); it != cache.end()) + { + if (IsStaleFunction()(it->first)) + { + BasePolicy::remove(it->first); + return {}; + } + return it->second; + } + + return {}; + } + + /// + /// select * from s3(http://aws.s3.us-east-1/table_root/year=*/*.parquet) + /// select * from s3(http://aws.s3.us-east-1/table_root/year={01..3}/*.parquet) -> lista de arquivos cacheada + // year=4 + /// select * from s3(http://aws.s3.us-east-1/table_root/year=201*/*.parquet) -> lista de arquivos cacheada + /// select * from s3(http://aws.s3.us-east-1/table_root/year=2011/*.parquet) -> + // + + std::optional getWithKey(const Key & key) override + { + if (const auto it = cache.find(key); it != cache.end()) + { + if (IsStaleFunction()(it->first)) + { + BasePolicy::remove(it->first); + return std::nullopt; + } + return std::make_optional({it->first, it->second}); + } + + if (const auto it = findBestMatchingPrefix(key); it != cache.end()) + { + if (IsStaleFunction()(it->first)) + { + BasePolicy::remove(it->first); + return std::nullopt; + } + return std::make_optional({it->first, it->second}); + } + + return std::nullopt; + } + + bool contains(const Key & key) const override + { + if (cache.contains(key)) + { + return true; + } + + return findAnyMatchingPrefix(key) != cache.end(); + } + +private: + auto findBestMatchingPrefix(const Key & key) + { + const auto & prefix = key.prefix; + + auto best_match = cache.end(); + size_t best_length = 0; + + for (auto it = cache.begin(); it != cache.end(); ++it) + { + const auto & candidate_prefix = it->first.prefix; + + if (prefix.compare(0, candidate_prefix.size(), candidate_prefix) == 0) + { + if (candidate_prefix.size() > best_length) + { + best_match = it; + best_length = candidate_prefix.size(); + } + } + } + + return best_match; + } + + auto findAnyMatchingPrefix(const Key & key) const + { + const auto & prefix = key.prefix; + return std::find_if(cache.begin(), cache.end(), [&](const auto & it) + { + const auto & candidate_prefix = it.first.prefix; + if (prefix.compare(0, candidate_prefix.size(), candidate_prefix) == 0) + { + return true; + } + + return false; + }); + } +}; + +ObjectStorageListObjectsCache::Key::Key( + const String & bucket_, + const String & prefix_, + const std::chrono::system_clock::time_point & expires_at_, + std::optional user_id_) + : bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} + +bool ObjectStorageListObjectsCache::Key::operator==(const Key & other) const +{ + return bucket == other.bucket && prefix == other.prefix; +} + +size_t ObjectStorageListObjectsCache::KeyHasher::operator()(const Key & key) const +{ + return std::hash()(key.prefix) + std::hash()(key.bucket); +} + +bool ObjectStorageListObjectsCache::IsStale::operator()(const Key & key) const +{ + return key.expires_at < std::chrono::system_clock::now(); +} + +size_t ObjectStorageListObjectsCache::WeightFunction::operator()(const Value & value) const +{ + std::size_t weight = 0; + + for (const auto & object : value) + { + weight += object->relative_path.size() + sizeof(ObjectMetadata); + } + + return weight; +} + +ObjectStorageListObjectsCache::ObjectStorageListObjectsCache() + : cache(std::make_unique>()) +{ +} + +void ObjectStorageListObjectsCache::set( + const std::string & bucket, + const std::string & prefix, + const std::shared_ptr & value) +{ + const auto key = Key{bucket, prefix, std::chrono::system_clock::now() + std::chrono::seconds(ttl)}; + + cache.set(key, value); +} + +ObjectStorageListObjectsCache::Cache::MappedPtr ObjectStorageListObjectsCache::get(const String & bucket, const String & prefix, bool filter_by_prefix) +{ + const auto input_key = Key{bucket, prefix}; + auto pair = cache.getWithKey(input_key); + + if (!pair) + { + return {}; + } + + if (pair->key == input_key || filter_by_prefix) + { + return pair->mapped; + } + + auto filtered_objects = std::make_shared>(); + filtered_objects->reserve(pair->mapped->size()); + + for (const auto & object : *pair->mapped) + { + if (object->relative_path.starts_with(input_key.prefix)) + { + filtered_objects->push_back(object); + } + } + + return filtered_objects; +} + +void ObjectStorageListObjectsCache::setMaxSizeInBytes(std::size_t size_in_bytes_) +{ + cache.setMaxSizeInBytes(size_in_bytes_); +} + +void ObjectStorageListObjectsCache::setMaxCount(std::size_t count) +{ + cache.setMaxCount(count); +} + +void ObjectStorageListObjectsCache::setTTL(std::size_t ttl_) +{ + ttl = ttl_; +} + +ObjectStorageListObjectsCache & ObjectStorageListObjectsCache::instance() +{ + static ObjectStorageListObjectsCache instance; + return instance; +} + +} diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h new file mode 100644 index 000000000000..d16a083dc0c4 --- /dev/null +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -0,0 +1,69 @@ +#pragma once + +#include +#include +#include +#include +#include + +namespace DB +{ + +class ObjectStorageListObjectsCache +{ +public: + static ObjectStorageListObjectsCache & instance(); + + struct Key + { + Key( + const String & bucket_, + const String & prefix_, + const std::chrono::system_clock::time_point & expires_at_ = std::chrono::system_clock::now(), + std::optional user_id_ = std::nullopt); + + std::string bucket; + std::string prefix; + std::chrono::system_clock::time_point expires_at; + std::optional user_id; + + bool operator==(const Key & other) const; + }; + + using Value = StorageObjectStorage::ObjectInfos; + struct KeyHasher + { + size_t operator()(const Key & key) const; + }; + + struct IsStale + { + bool operator()(const Key & key) const; + }; + + struct WeightFunction + { + size_t operator()(const Value & value) const; + }; + + using Cache = CacheBase; + + void set( + const std::string & bucket, + const std::string & prefix, + const std::shared_ptr & value); + + Cache::MappedPtr get(const String & bucket, const String & prefix, bool filter_by_prefix = true); + + void setMaxSizeInBytes(std::size_t size_in_bytes_); + void setMaxCount(std::size_t count); + void setTTL(std::size_t ttl_); + +private: + ObjectStorageListObjectsCache(); + + Cache cache; + size_t ttl; +}; + +} diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 5622a2605535..208df61e990e 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -52,6 +52,7 @@ namespace Setting extern const SettingsString filesystem_cache_name; extern const SettingsUInt64 filesystem_cache_boundary_alignment; extern const SettingsBool use_iceberg_partition_pruning; + extern const SettingsBool use_object_storage_list_objects_cache; } namespace ErrorCodes @@ -62,6 +63,98 @@ namespace ErrorCodes extern const int FILE_DOESNT_EXIST; } +struct CachedGlobObjectIterator : IObjectIterator, WithContext +{ + using Configuration = StorageObjectStorage::Configuration; + using ConfigurationPtr = StorageObjectStorage::ConfigurationPtr; + using ObjectInfos = StorageObjectStorage::ObjectInfos; + + CachedGlobObjectIterator( + const std::shared_ptr & object_infos_, + ConfigurationPtr configuration_, + const ActionsDAG::Node * predicate, + const NamesAndTypesList & virtual_columns, + ContextPtr context_, + std::size_t , + ObjectInfos * read_keys = nullptr) + : WithContext(context_), + object_infos(object_infos_), + configuration(configuration_) + { + if (configuration->isPathWithGlobs()) + { + const auto key_with_globs = configuration->getPath(); + + // + auto matcher = std::make_unique(makeRegexpPatternFromGlobs(key_with_globs)); + if (!matcher->ok()) + { + throw Exception(ErrorCodes::CANNOT_COMPILE_REGEXP, "Cannot compile regex from glob ({}): {}", key_with_globs, matcher->error()); + } + + ExpressionActionsPtr filter_expr; + recursive = key_with_globs == "/**"; + if (auto filter_dag = VirtualColumnUtils::createPathAndFileFilterDAG(predicate, virtual_columns)) + { + VirtualColumnUtils::buildSetsForDAG(*filter_dag, getContext()); + filter_expr = std::make_shared(std::move(*filter_dag)); + } + + for (auto it = object_infos->begin(); it != object_infos->end();) + { + if (!recursive && !re2::RE2::FullMatch((*it)->getPath(), *matcher)) + it = object_infos->erase(it); + else + ++it; + } + + if (filter_expr) + { + std::vector paths; + paths.reserve(object_infos->size()); + for (const auto & object_info : *object_infos) + paths.push_back(StorageObjectStorageSource::getUniqueStoragePathIdentifier(*configuration, *object_info, false)); + + VirtualColumnUtils::filterByPathOrFile(*object_infos, paths, filter_expr, virtual_columns, getContext()); + + LOG_TEST(getLogger("Arthur"), "Filtered files: {} -> {}", paths.size(), object_infos->size()); + } + + if (read_keys) + read_keys->insert(read_keys->end(), object_infos->begin(), object_infos->end()); + } + else + { + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Using glob iterator with path without globs is not allowed (used path: {})", + configuration->getPath()); + } + } + + ObjectInfoPtr next(size_t) override { + std::lock_guard lock(mutex); + + if (index >= object_infos->size()) { + return {}; + } + + return (*object_infos)[index++]; + } + + size_t estimatedKeysCount() override { + return object_infos->size(); + } + +private: + + std::shared_ptr object_infos; + std::mutex mutex; + size_t index = 0; + ConfigurationPtr configuration; + bool recursive {false}; +}; + StorageObjectStorageSource::StorageObjectStorageSource( String name_, ObjectStoragePtr object_storage_, @@ -156,11 +249,32 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( query_settings.ignore_non_existent_file, file_progress_callback); } else - /// Iterate through disclosed globs and make a source for each file - iterator = std::make_unique( - object_storage, configuration, predicate, virtual_columns, - local_context, is_archive ? nullptr : read_keys, query_settings.list_object_keys_size, - query_settings.throw_on_zero_files_match, file_progress_callback); + { + if (local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache]) + { + auto & cache = ObjectStorageListObjectsCache::instance(); + + if (auto objects_info = cache.get(configuration->getNamespace(), configuration->getPathWithoutGlobs(), /*filter_by_prefix=*/ false)) + { + iterator = std::make_unique(objects_info, configuration, predicate, virtual_columns, local_context, 0, read_keys); + } + else + { + /// Iterate through disclosed globs and make a source for each file + iterator = std::make_unique( + object_storage, configuration, predicate, virtual_columns, + local_context, is_archive ? nullptr : read_keys, query_settings.list_object_keys_size, + query_settings.throw_on_zero_files_match, file_progress_callback, &cache); + } + } + else { + /// Iterate through disclosed globs and make a source for each file + iterator = std::make_unique( + object_storage, configuration, predicate, virtual_columns, + local_context, is_archive ? nullptr : read_keys, query_settings.list_object_keys_size, + query_settings.throw_on_zero_files_match, file_progress_callback, nullptr); + } + } } else if (configuration->supportsFileIterator()) { @@ -665,7 +779,8 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( ObjectInfos * read_keys_, size_t list_object_keys_size, bool throw_on_zero_files_match_, - std::function file_progress_callback_) + std::function file_progress_callback_, + ObjectStorageListObjectsCache * list_cache_) : WithContext(context_) , object_storage(object_storage_) , configuration(configuration_) @@ -675,6 +790,7 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( , read_keys(read_keys_) , local_context(context_) , file_progress_callback(file_progress_callback_) + , list_cache(list_cache_) { if (configuration->isNamespaceWithGlobs()) { @@ -750,11 +866,21 @@ StorageObjectStorage::ObjectInfoPtr StorageObjectStorageSource::GlobIterator::ne auto result = object_storage_iterator->getCurrentBatchAndScheduleNext(); if (!result.has_value()) { + if (list_cache) + { + list_cache->set(configuration->getNamespace(), configuration->getPathWithoutGlobs(), std::make_shared(std::move(object_list))); + } is_finished = true; return {}; } new_batch = std::move(result.value()); + + if (list_cache) + { + object_list.insert(object_list.end(), new_batch.begin(), new_batch.end()); + } + for (auto it = new_batch.begin(); it != new_batch.end();) { if (!recursive && !re2::RE2::FullMatch((*it)->getPath(), *matcher)) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 1301c089e1e0..0071a20209d1 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -172,7 +172,8 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon ObjectInfos * read_keys_, size_t list_object_keys_size, bool throw_on_zero_files_match_, - std::function file_progress_callback_ = {}); + std::function file_progress_callback_ = {}, + ObjectStorageListObjectsCache * list_cache_ = nullptr); ~GlobIterator() override = default; @@ -209,6 +210,8 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon const ContextPtr local_context; std::function file_progress_callback; + ObjectStorageListObjectsCache * list_cache; + ObjectInfos object_list; }; class StorageObjectStorageSource::KeysIterator : public IObjectIterator From 7e182c7460d191c9c87b1333dafe940c85ce8cc5 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Thu, 17 Apr 2025 16:49:13 -0300 Subject: [PATCH 02/46] remove garbage --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 8e78e1b94a5c..ad6e5df60f45 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -42,14 +42,6 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy lista de arquivos cacheada - // year=4 - /// select * from s3(http://aws.s3.us-east-1/table_root/year=201*/*.parquet) -> lista de arquivos cacheada - /// select * from s3(http://aws.s3.us-east-1/table_root/year=2011/*.parquet) -> - // - std::optional getWithKey(const Key & key) override { if (const auto it = cache.find(key); it != cache.end()) From ef985c9364d9e442b26a34878dd3ca9b37994f4a Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Thu, 17 Apr 2025 17:01:11 -0300 Subject: [PATCH 03/46] use starts_with --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index ad6e5df60f45..0652a9e75b71 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -89,13 +89,10 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfirst.prefix; - if (prefix.compare(0, candidate_prefix.size(), candidate_prefix) == 0) + if (prefix.starts_with(candidate_prefix) && candidate_prefix.size() > best_length) { - if (candidate_prefix.size() > best_length) - { - best_match = it; - best_length = candidate_prefix.size(); - } + best_match = it; + best_length = candidate_prefix.size(); } } @@ -108,7 +105,7 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy Date: Thu, 17 Apr 2025 17:08:50 -0300 Subject: [PATCH 04/46] fix a few obvious bugs --- .../Cache/ObjectStorageListObjectsCache.cpp | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 0652a9e75b71..b3bd97967b3a 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -87,12 +87,22 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfirst.bucket; const auto & candidate_prefix = it->first.prefix; - if (prefix.starts_with(candidate_prefix) && candidate_prefix.size() > best_length) + if (candidate_bucket == key.bucket && prefix.starts_with(candidate_prefix)) { - best_match = it; - best_length = candidate_prefix.size(); + if (IsStaleFunction()(it->fist)) + { + BasePolicy::remove(it->first); + continue; + } + + if (candidate_prefix.size() > best_length) + { + best_match = it; + best_length = candidate_prefix.size(); + } } } @@ -101,12 +111,19 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfist)) + { + BasePolicy::remove(it->first); + continue; + } return true; } From 989cfe0caac2af59046ee0693d94679f8407c855 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Thu, 17 Apr 2025 17:12:35 -0300 Subject: [PATCH 05/46] some other fixes --- .../Cache/ObjectStorageListObjectsCache.cpp | 51 ++++++------------- .../StorageObjectStorageSource.h | 1 + 2 files changed, 16 insertions(+), 36 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index b3bd97967b3a..67b074a03528 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -1,4 +1,5 @@ #include +#include namespace DB { @@ -67,16 +68,6 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy to_remove; + for (auto it = cache.begin(); it != cache.end(); ++it) { const auto & candidate_bucket = it->first.bucket; @@ -92,9 +85,9 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfist)) + if (IsStaleFunction()(it->first)) { - BasePolicy::remove(it->first); + to_remove.push_back(it->first); continue; } @@ -106,29 +99,10 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfist)) - { - BasePolicy::remove(it->first); - continue; - } - return true; - } + for (const auto & k : to_remove) + BasePolicy::remove(k); - return false; - }); + return best_match; } }; @@ -146,7 +120,12 @@ bool ObjectStorageListObjectsCache::Key::operator==(const Key & other) const size_t ObjectStorageListObjectsCache::KeyHasher::operator()(const Key & key) const { - return std::hash()(key.prefix) + std::hash()(key.bucket); + std::size_t seed = 0; + + boost::hash_combine(seed, std::hash()(key.bucket)); + boost::hash_combine(seed, std::hash()(key.prefix)); + + return seed; } bool ObjectStorageListObjectsCache::IsStale::operator()(const Key & key) const @@ -160,7 +139,7 @@ size_t ObjectStorageListObjectsCache::WeightFunction::operator()(const Value & v for (const auto & object : value) { - weight += object->relative_path.size() + sizeof(ObjectMetadata); + weight += object->relative_path.capacity() + sizeof(ObjectMetadata); } return weight; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 0071a20209d1..090379864367 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -7,6 +7,7 @@ #include #include #include +#include namespace DB From 157450de23406c38e00a15837a6487b03d8313e1 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Thu, 17 Apr 2025 20:16:04 -0300 Subject: [PATCH 06/46] another fix --- programs/server/Server.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index b07b51c46522..fc85a5e853f4 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -319,6 +319,9 @@ namespace ServerSetting extern const ServerSettingsUInt64 max_prefixes_deserialization_thread_pool_free_size; extern const ServerSettingsUInt64 prefixes_deserialization_thread_pool_thread_pool_queue_size; extern const ServerSettingsUInt64 input_format_parquet_metadata_cache_max_size; + extern const ServerSettingsUInt64 object_storage_list_objects_cache_size; + extern const ServerSettingsUInt64 object_storage_list_objects_cache_max_entries; + extern const ServerSettingsUInt64 object_storage_list_objects_cache_ttl; } } From 727b64dce81412260b140eac0f4b87c748439312 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 10:37:51 -0300 Subject: [PATCH 07/46] remove unused method --- .../Cache/ObjectStorageListObjectsCache.cpp | 29 ++----------------- 1 file changed, 2 insertions(+), 27 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 67b074a03528..6aa2a92b89aa 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -18,31 +18,6 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfirst)) - { - BasePolicy::remove(it->first); - return {}; - } - return it->second; - } - - if (const auto it = findBestMatchingPrefix(key); it != cache.end()) - { - if (IsStaleFunction()(it->first)) - { - BasePolicy::remove(it->first); - return {}; - } - return it->second; - } - - return {}; - } - std::optional getWithKey(const Key & key) override { if (const auto it = cache.find(key); it != cache.end()) @@ -55,7 +30,7 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy({it->first, it->second}); } - if (const auto it = findBestMatchingPrefix(key); it != cache.end()) + if (const auto it = findBestMatchingPrefixAndRemoveExpiredEntries(key); it != cache.end()) { if (IsStaleFunction()(it->first)) { @@ -69,7 +44,7 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy Date: Fri, 18 Apr 2025 10:43:44 -0300 Subject: [PATCH 08/46] some other fixes --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 6aa2a92b89aa..55bbb56394c8 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -32,11 +32,6 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfirst)) - { - BasePolicy::remove(it->first); - return std::nullopt; - } return std::make_optional({it->first, it->second}); } From 00f58b3a54c8ca9665b8c85b336f31205fa5550c Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 10:45:46 -0300 Subject: [PATCH 09/46] some other fixes --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 55bbb56394c8..b5602409651c 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -140,7 +140,7 @@ ObjectStorageListObjectsCache::Cache::MappedPtr ObjectStorageListObjectsCache::g return {}; } - if (pair->key == input_key || filter_by_prefix) + if (pair->key == input_key || !filter_by_prefix) { return pair->mapped; } From 67ccaf0eb6a267b7fc228963077eb0cce39c93fe Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 11:17:33 -0300 Subject: [PATCH 10/46] use steady_clock instead of system_clock --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 7 ++++--- src/Storages/Cache/ObjectStorageListObjectsCache.h | 5 ++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index b5602409651c..2ee9da9b6ac9 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -1,4 +1,5 @@ #include +#include #include namespace DB @@ -79,7 +80,7 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy user_id_) : bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} @@ -100,7 +101,7 @@ size_t ObjectStorageListObjectsCache::KeyHasher::operator()(const Key & key) con bool ObjectStorageListObjectsCache::IsStale::operator()(const Key & key) const { - return key.expires_at < std::chrono::system_clock::now(); + return key.expires_at < std::chrono::steady_clock::now(); } size_t ObjectStorageListObjectsCache::WeightFunction::operator()(const Value & value) const @@ -125,7 +126,7 @@ void ObjectStorageListObjectsCache::set( const std::string & prefix, const std::shared_ptr & value) { - const auto key = Key{bucket, prefix, std::chrono::system_clock::now() + std::chrono::seconds(ttl)}; + const auto key = Key{bucket, prefix, std::chrono::steady_clock::now() + std::chrono::seconds(ttl)}; cache.set(key, value); } diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index d16a083dc0c4..798d56f92dd5 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -3,7 +3,6 @@ #include #include #include -#include #include namespace DB @@ -19,12 +18,12 @@ class ObjectStorageListObjectsCache Key( const String & bucket_, const String & prefix_, - const std::chrono::system_clock::time_point & expires_at_ = std::chrono::system_clock::now(), + const std::chrono::steady_clock::time_point & expires_at_ = std::chrono::steady_clock::now(), std::optional user_id_ = std::nullopt); std::string bucket; std::string prefix; - std::chrono::system_clock::time_point expires_at; + std::chrono::steady_clock::time_point expires_at; std::optional user_id; bool operator==(const Key & other) const; From 2b37e0c7dafbf59896b902dd0311a3a2356ea421 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 11:23:35 -0300 Subject: [PATCH 11/46] small fixes --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 4 ++-- src/Storages/Cache/ObjectStorageListObjectsCache.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 2ee9da9b6ac9..d49ac048adef 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -93,8 +93,8 @@ size_t ObjectStorageListObjectsCache::KeyHasher::operator()(const Key & key) con { std::size_t seed = 0; - boost::hash_combine(seed, std::hash()(key.bucket)); - boost::hash_combine(seed, std::hash()(key.prefix)); + boost::hash_combine(seed, key.bucket); + boost::hash_combine(seed, key.prefix); return seed; } diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 798d56f92dd5..9dab4a24adec 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -62,7 +62,7 @@ class ObjectStorageListObjectsCache ObjectStorageListObjectsCache(); Cache cache; - size_t ttl; + size_t ttl {0}; }; } From 0d6e343eef2dda77387f87d914b0ba95e7cb71de Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 11:42:47 -0300 Subject: [PATCH 12/46] Dont return null in case of exact match with stale entry, perform linear prefix matchin search --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index d49ac048adef..ea123cc684c6 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -23,12 +23,12 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicyfirst)) + if (!IsStaleFunction()(it->first)) { - BasePolicy::remove(it->first); - return std::nullopt; + return std::make_optional({it->first, it->second}); } - return std::make_optional({it->first, it->second}); + // found a stale entry, remove it but don't return. We still want to perform the prefix matching search + BasePolicy::remove(it->first); } if (const auto it = findBestMatchingPrefixAndRemoveExpiredEntries(key); it != cache.end()) From 0f605c4edaec0aabd5d3f5e2c7dd2db2be1f2775 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 13:06:00 -0300 Subject: [PATCH 13/46] add some metrics --- src/Common/ProfileEvents.cpp | 6 ++++- .../Cache/ObjectStorageListObjectsCache.cpp | 22 ++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/Common/ProfileEvents.cpp b/src/Common/ProfileEvents.cpp index ec7fa7fef0a7..01b82581e44f 100644 --- a/src/Common/ProfileEvents.cpp +++ b/src/Common/ProfileEvents.cpp @@ -963,7 +963,11 @@ The server successfully detected this situation and will download merged part fr M(ParquetFetchWaitTimeMicroseconds, "Time of waiting fetching parquet data", ValueType::Microseconds) \ \ M(ParquetMetaDataCacheHits, "Number of times the read from filesystem cache hit the cache.", ValueType::Number) \ - M(ParquetMetaDataCacheMisses, "Number of times the read from filesystem cache miss the cache.", ValueType::Number) \ + M(ParquetMetaDataCacheMisses, "Number of times the read from filesystem cache miss the cache.", ValueType::Number) \ + M(ObjectStorageListObjectsCacheHits, "Number of times object storage list objects operation hit the cache.", ValueType::Number) \ + M(ObjectStorageListObjectsCacheMisses, "Number of times object storage list objects operation miss the cache.", ValueType::Number) \ + M(ObjectStorageListObjectsCacheExactMatchHits, "Number of times the read from filesystem cache hit the cache with an exact match.", ValueType::Number) \ + M(ObjectStorageListObjectsCachePrefixMatchHits, "Number of times the read from filesystem cache miss the cache using prefix matching.", ValueType::Number) \ #ifdef APPLY_FOR_EXTERNAL_EVENTS #define APPLY_FOR_EVENTS(M) APPLY_FOR_BUILTIN_EVENTS(M) APPLY_FOR_EXTERNAL_EVENTS(M) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index ea123cc684c6..c61907d8541f 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -1,7 +1,16 @@ #include #include +#include #include +namespace ProfileEvents +{ +extern const Event ObjectStorageListObjectsCacheHits; +extern const Event ObjectStorageListObjectsCacheMisses; +extern const Event ObjectStorageListObjectsCacheExactMatchHits; +extern const Event ObjectStorageListObjectsCachePrefixMatchHits; +} + namespace DB { @@ -138,10 +147,21 @@ ObjectStorageListObjectsCache::Cache::MappedPtr ObjectStorageListObjectsCache::g if (!pair) { + ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCacheMisses); return {}; } - if (pair->key == input_key || !filter_by_prefix) + ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCacheHits); + + if (pair->key == input_key) + { + ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCacheExactMatchHits); + return pair->mapped; + } + + ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCachePrefixMatchHits); + + if (!filter_by_prefix) { return pair->mapped; } From 3ed234911c258b98a379a41a6362ca6c76c04388 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 16:32:12 -0300 Subject: [PATCH 14/46] implement cache clear command --- src/Access/Common/AccessType.h | 3 ++- src/Interpreters/InterpreterSystemQuery.cpp | 12 ++++++++++++ src/Parsers/ASTSystemQuery.cpp | 1 + src/Parsers/ASTSystemQuery.h | 1 + src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 5 +++++ src/Storages/Cache/ObjectStorageListObjectsCache.h | 2 ++ ...03377_object_storage_list_objects_cache.reference | 0 .../03377_object_storage_list_objects_cache.sql | 0 8 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference create mode 100644 tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql diff --git a/src/Access/Common/AccessType.h b/src/Access/Common/AccessType.h index d170d56500d3..bd89c1c1cc94 100644 --- a/src/Access/Common/AccessType.h +++ b/src/Access/Common/AccessType.h @@ -182,7 +182,8 @@ enum class AccessType : uint8_t M(SYSTEM_DROP_SCHEMA_CACHE, "SYSTEM DROP SCHEMA CACHE, DROP SCHEMA CACHE", GLOBAL, SYSTEM_DROP_CACHE) \ M(SYSTEM_DROP_FORMAT_SCHEMA_CACHE, "SYSTEM DROP FORMAT SCHEMA CACHE, DROP FORMAT SCHEMA CACHE", GLOBAL, SYSTEM_DROP_CACHE) \ M(SYSTEM_DROP_S3_CLIENT_CACHE, "SYSTEM DROP S3 CLIENT, DROP S3 CLIENT CACHE", GLOBAL, SYSTEM_DROP_CACHE) \ - M(SYSTEM_DROP_PARQUET_METADATA_CACHE, "SYSTEM DROP PARQUET METADATA CACHE", GLOBAL, SYSTEM_DROP_CACHE) \ + M(SYSTEM_DROP_PARQUET_METADATA_CACHE, "SYSTEM DROP PARQUET METADATA CACHE", GLOBAL, SYSTEM_DROP_CACHE) \ + M(SYSTEM_DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE, "SYSTEM DROP OBJECT STORAGE LIST OBJECTS CACHE", GLOBAL, SYSTEM_DROP_CACHE) \ M(SYSTEM_DROP_CACHE, "DROP CACHE", GROUP, SYSTEM) \ M(SYSTEM_RELOAD_CONFIG, "RELOAD CONFIG", GLOBAL, SYSTEM_RELOAD) \ M(SYSTEM_RELOAD_USERS, "RELOAD USERS", GLOBAL, SYSTEM_RELOAD) \ diff --git a/src/Interpreters/InterpreterSystemQuery.cpp b/src/Interpreters/InterpreterSystemQuery.cpp index 9a3b1e7d6ff8..63f4b8961262 100644 --- a/src/Interpreters/InterpreterSystemQuery.cpp +++ b/src/Interpreters/InterpreterSystemQuery.cpp @@ -47,6 +47,7 @@ #include #include #include +#include #include #include #include @@ -435,6 +436,16 @@ BlockIO InterpreterSystemQuery::execute() break; #else throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "The server was compiled without the support for Parquet"); +#endif + } + case Type::DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE: + { +#if USE_PARQUET + getContext()->checkAccess(AccessType::SYSTEM_DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE); + ObjectStorageListObjectsCache::instance().clear(); + break; +#else + throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "The server was compiled without the support for Parquet"); #endif } case Type::DROP_COMPILED_EXPRESSION_CACHE: @@ -1469,6 +1480,7 @@ AccessRightsElements InterpreterSystemQuery::getRequiredAccessForDDLOnCluster() case Type::DROP_SCHEMA_CACHE: case Type::DROP_FORMAT_SCHEMA_CACHE: case Type::DROP_PARQUET_METADATA_CACHE: + case Type::DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE: case Type::DROP_S3_CLIENT_CACHE: { required_access.emplace_back(AccessType::SYSTEM_DROP_CACHE); diff --git a/src/Parsers/ASTSystemQuery.cpp b/src/Parsers/ASTSystemQuery.cpp index 811321975aa4..baa70d233ab9 100644 --- a/src/Parsers/ASTSystemQuery.cpp +++ b/src/Parsers/ASTSystemQuery.cpp @@ -432,6 +432,7 @@ void ASTSystemQuery::formatImpl(WriteBuffer & ostr, const FormatSettings & setti case Type::DROP_COMPILED_EXPRESSION_CACHE: case Type::DROP_S3_CLIENT_CACHE: case Type::DROP_PARQUET_METADATA_CACHE: + case Type::DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE: case Type::DROP_ICEBERG_METADATA_CACHE: case Type::RESET_COVERAGE: case Type::RESTART_REPLICAS: diff --git a/src/Parsers/ASTSystemQuery.h b/src/Parsers/ASTSystemQuery.h index 4a906ea457af..cc3740bdec61 100644 --- a/src/Parsers/ASTSystemQuery.h +++ b/src/Parsers/ASTSystemQuery.h @@ -42,6 +42,7 @@ class ASTSystemQuery : public IAST, public ASTQueryWithOnCluster DROP_FORMAT_SCHEMA_CACHE, DROP_S3_CLIENT_CACHE, DROP_PARQUET_METADATA_CACHE, + DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE, STOP_LISTEN, START_LISTEN, RESTART_REPLICAS, diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index c61907d8541f..863b8ba3c130 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -140,6 +140,11 @@ void ObjectStorageListObjectsCache::set( cache.set(key, value); } +void ObjectStorageListObjectsCache::clear() +{ + cache.clear(); +} + ObjectStorageListObjectsCache::Cache::MappedPtr ObjectStorageListObjectsCache::get(const String & bucket, const String & prefix, bool filter_by_prefix) { const auto input_key = Key{bucket, prefix}; diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 9dab4a24adec..bf0274ca2a46 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -54,6 +54,8 @@ class ObjectStorageListObjectsCache Cache::MappedPtr get(const String & bucket, const String & prefix, bool filter_by_prefix = true); + void clear(); + void setMaxSizeInBytes(std::size_t size_in_bytes_); void setMaxCount(std::size_t count); void setTTL(std::size_t ttl_); diff --git a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql new file mode 100644 index 000000000000..e69de29bb2d1 From f345b3321d6fdd52a1d55aa73c09d1ca1be2b5b1 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 18 Apr 2025 18:27:10 -0300 Subject: [PATCH 15/46] remove ifdef from parquet --- src/Interpreters/InterpreterSystemQuery.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Interpreters/InterpreterSystemQuery.cpp b/src/Interpreters/InterpreterSystemQuery.cpp index 63f4b8961262..726eea882ec8 100644 --- a/src/Interpreters/InterpreterSystemQuery.cpp +++ b/src/Interpreters/InterpreterSystemQuery.cpp @@ -440,13 +440,9 @@ BlockIO InterpreterSystemQuery::execute() } case Type::DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE: { -#if USE_PARQUET getContext()->checkAccess(AccessType::SYSTEM_DROP_OBJECT_STORAGE_LIST_OBJECTS_CACHE); ObjectStorageListObjectsCache::instance().clear(); break; -#else - throw Exception(ErrorCodes::SUPPORT_IS_DISABLED, "The server was compiled without the support for Parquet"); -#endif } case Type::DROP_COMPILED_EXPRESSION_CACHE: #if USE_EMBEDDED_COMPILER From 92428438d995b0136b1b56bf71b5e5d26053b907 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Sun, 20 Apr 2025 17:39:50 -0300 Subject: [PATCH 16/46] add stateless tests --- ...3377_object_storage_list_objects_cache.sql | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql index e69de29bb2d1..5436824bdf6a 100644 --- a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql +++ b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql @@ -0,0 +1,109 @@ +-- Tags: no-parallel, no-fasttest + +SYSTEM DROP OBJECT STORAGE LIST OBJECTS CACHE; + +INSERT INTO TABLE FUNCTION s3(s3_conn, filename='dir_a/dir_b/t_03377_sample.parquet', format='Parquet', structure='id UInt64') SETTINGS s3_truncate_on_insert=1 VALUES (1); + +SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='cold_list_cache'; +SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='warm_list_exact_cache'; +SELECT * FROM s3(s3_conn, filename='dir_a/dir_b**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='warm_list_prefix_match_cache'; +SELECT * FROM s3(s3_conn, filename='dirr_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='warm_list_cache_miss'; -- { serverError CANNOT_EXTRACT_TABLE_STRUCTURE } +SELECT * FROM s3(s3_conn, filename='d**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='even_shorter_prefix'; +SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='still_exact_match_after_shorter_prefix'; +SYSTEM DROP OBJECT STORAGE LIST OBJECTS CACHE; +SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='after_drop'; + +SYSTEM FLUSH LOGS; + +-- { echoOn } + +SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss +FROM system.query_log +where log_comment = 'cold_list_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'warm_list_exact_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheExactMatchHits'] > 0 as hit +FROM system.query_log +where log_comment = 'warm_list_exact_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCachePrefixMatchHits'] > 0 as prefix_match_hit +FROM system.query_log +where log_comment = 'warm_list_exact_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'warm_list_prefix_match_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheExactMatchHits'] > 0 as exact_match_hit +FROM system.query_log +where log_comment = 'warm_list_prefix_match_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCachePrefixMatchHits'] > 0 as prefix_match_hit +FROM system.query_log +where log_comment = 'warm_list_prefix_match_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'even_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss +FROM system.query_log +where log_comment = 'even_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'still_exact_match_after_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheExactMatchHits'] > 0 as exact_match_hit +FROM system.query_log +where log_comment = 'still_exact_match_after_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'after_drop' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; + +SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss +FROM system.query_log +where log_comment = 'after_drop' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; From 4e19b09482d5a78f86ba5ebc2ca57c79dca55365 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Sun, 20 Apr 2025 18:49:21 -0300 Subject: [PATCH 17/46] add unit tests --- ...test_object_storage_list_objects_cache.cpp | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp new file mode 100644 index 000000000000..5c480fba78d6 --- /dev/null +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -0,0 +1,149 @@ +#include +#include +#include +#include + +namespace DB +{ + +auto & initializeAndGetCacheInstance() +{ + static auto & cache = ObjectStorageListObjectsCache::instance(); + cache.setTTL(3); + cache.setMaxCount(100); + cache.setMaxSizeInBytes(1000000); + return cache; +} + +static auto & cache = initializeAndGetCacheInstance(); + +std::shared_ptr createTestValue(const std::vector& paths) +{ + auto value = std::make_shared(); + for (const auto & path : paths) + { + value->push_back(std::make_shared(path)); + } + return value; +} + + +TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) +{ + cache.clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; + auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); + + cache.set(bucket, prefix, value); + + auto result = cache.get(bucket, prefix); + ASSERT_EQ(result->size(), 2); + EXPECT_EQ((*result)[0]->getPath(), "test-prefix/file1.txt"); + EXPECT_EQ((*result)[1]->getPath(), "test-prefix/file2.txt"); +} + +TEST(ObjectStorageListObjectsCacheTest, CacheMiss) +{ + cache.clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; + + auto result = cache.get(bucket, prefix); + EXPECT_EQ(result, nullptr); +} + +TEST(ObjectStorageListObjectsCacheTest, ClearCache) +{ + cache.clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; + auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); + + cache.set(bucket, prefix, value); + cache.clear(); + + auto result = cache.get(bucket, prefix); + EXPECT_EQ(result, nullptr); +} + +TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) +{ + cache.clear(); + const std::string bucket = "test-bucket"; + const std::string short_prefix = "parent/"; + const std::string mid_prefix = "parent/child/"; + const std::string long_prefix = "parent/child/grandchild/"; + + auto value = createTestValue( + { + "parent/child/grandchild/file1.txt", + "parent/child/grandchild/file2.txt"}); + + cache.set(bucket, mid_prefix, value); + + auto result1 = cache.get(bucket, mid_prefix); + EXPECT_EQ(result1->size(), 2); + + auto result2 = cache.get(bucket, long_prefix); + EXPECT_EQ(result2->size(), 2); + + auto result3 = cache.get(bucket, short_prefix); + ASSERT_EQ(result3, nullptr); +} + +TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) +{ + cache.clear(); + const std::string bucket = "test-bucket"; + const std::string short_prefix = "parent/"; + + auto value = createTestValue({ + "parent/file1.txt", + "parent/child1/file2.txt", + "parent/child2/file3.txt" + }); + + cache.set(bucket, short_prefix, value); + + auto result = cache.get(bucket, "parent/child1/", true); + EXPECT_EQ(result->size(), 1); + EXPECT_EQ((*result)[0]->getPath(), "parent/child1/file2.txt"); +} + +TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) +{ + cache.clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; + auto value = createTestValue({"test-prefix/file1.txt"}); + + cache.set(bucket, prefix, value); + + // Verify we can get it immediately + auto result1 = cache.get(bucket, prefix); + EXPECT_EQ(result1->size(), 1); + + std::this_thread::sleep_for(std::chrono::seconds(4)); + + auto result2 = cache.get(bucket, prefix); + EXPECT_EQ(result2, nullptr); +} + +TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) +{ + cache.clear(); + const std::string bucket = "test-bucket"; + + auto short_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt", "a/b/file2.txt"}); + auto mid_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt"}); + + cache.set(bucket, "a/b/", short_prefix); + cache.set(bucket, "a/b/c/", mid_prefix); + + // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size + auto result = cache.get(bucket, "a/b/c/d/", false); + EXPECT_EQ(result->size(), 2u); +} + +} From 8c4ea488f9a44199c0030dabcebcbf774d547ffc Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Sun, 20 Apr 2025 18:51:00 -0300 Subject: [PATCH 18/46] rename ttl argument and member variable --- src/Storages/Cache/ObjectStorageListObjectsCache.cpp | 6 +++--- src/Storages/Cache/ObjectStorageListObjectsCache.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 863b8ba3c130..8cc8e5a2e26e 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -135,7 +135,7 @@ void ObjectStorageListObjectsCache::set( const std::string & prefix, const std::shared_ptr & value) { - const auto key = Key{bucket, prefix, std::chrono::steady_clock::now() + std::chrono::seconds(ttl)}; + const auto key = Key{bucket, prefix, std::chrono::steady_clock::now() + std::chrono::seconds(ttl_in_seconds)}; cache.set(key, value); } @@ -195,9 +195,9 @@ void ObjectStorageListObjectsCache::setMaxCount(std::size_t count) cache.setMaxCount(count); } -void ObjectStorageListObjectsCache::setTTL(std::size_t ttl_) +void ObjectStorageListObjectsCache::setTTL(std::size_t ttl_in_seconds_) { - ttl = ttl_; + ttl_in_seconds = ttl_in_seconds_; } ObjectStorageListObjectsCache & ObjectStorageListObjectsCache::instance() diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index bf0274ca2a46..89adbf874317 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -58,13 +58,13 @@ class ObjectStorageListObjectsCache void setMaxSizeInBytes(std::size_t size_in_bytes_); void setMaxCount(std::size_t count); - void setTTL(std::size_t ttl_); + void setTTL(std::size_t ttl_in_seconds_); private: ObjectStorageListObjectsCache(); Cache cache; - size_t ttl {0}; + size_t ttl_in_seconds {0}; }; } From 74b980c72930dce3b6151f40cc3a922604b652dd Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Sun, 20 Apr 2025 18:55:33 -0300 Subject: [PATCH 19/46] new settings history --- src/Core/SettingsChangesHistory.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index a8c525f37851..7520b73734e7 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -72,6 +72,7 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() {"use_iceberg_metadata_files_cache", true, true, "New setting"}, {"iceberg_timestamp_ms", 0, 0, "New setting."}, {"iceberg_snapshot_id", 0, 0, "New setting."}, + {"use_object_storage_list_objects_cache", false, true, "New setting."}, }); addSettingsChanges(settings_changes_history, "24.12.2.20000", { From 4f55a75c893731b0e5b9382f69e800347a5a65ff Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 21 Apr 2025 10:08:58 -0300 Subject: [PATCH 20/46] make the setting false by default so other tests are not affected --- src/Core/Settings.cpp | 2 +- src/Core/SettingsChangesHistory.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 558ff43852ab..0cf2cbfc5d11 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -6109,7 +6109,7 @@ Possible values: - Positive integer. - 0 — All hosts in cluster. )", EXPERIMENTAL) \ - DECLARE(Bool, use_object_storage_list_objects_cache, true, R"( + DECLARE(Bool, use_object_storage_list_objects_cache, false, R"( Cache the list of objects returned by list objects calls in object storage )", EXPERIMENTAL) \ \ diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index 7520b73734e7..c9d35accbaa7 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -72,7 +72,7 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() {"use_iceberg_metadata_files_cache", true, true, "New setting"}, {"iceberg_timestamp_ms", 0, 0, "New setting."}, {"iceberg_snapshot_id", 0, 0, "New setting."}, - {"use_object_storage_list_objects_cache", false, true, "New setting."}, + {"use_object_storage_list_objects_cache", true, false, "New setting."}, }); addSettingsChanges(settings_changes_history, "24.12.2.20000", { From 303ee274afdcf8ce24de7a62f383c3a91b134118 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 21 Apr 2025 16:10:07 -0300 Subject: [PATCH 21/46] add ref file --- ...bject_storage_list_objects_cache.reference | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference index e69de29bb2d1..396959f0e1ed 100644 --- a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference +++ b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference @@ -0,0 +1,93 @@ +-- { echoOn } + +SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss +FROM system.query_log +where log_comment = 'cold_list_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'warm_list_exact_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCacheExactMatchHits'] > 0 as hit +FROM system.query_log +where log_comment = 'warm_list_exact_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCachePrefixMatchHits'] > 0 as prefix_match_hit +FROM system.query_log +where log_comment = 'warm_list_exact_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +0 +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'warm_list_prefix_match_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCacheExactMatchHits'] > 0 as exact_match_hit +FROM system.query_log +where log_comment = 'warm_list_prefix_match_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +0 +SELECT ProfileEvents['ObjectStorageListObjectsCachePrefixMatchHits'] > 0 as prefix_match_hit +FROM system.query_log +where log_comment = 'warm_list_prefix_match_cache' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'even_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +0 +SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss +FROM system.query_log +where log_comment = 'even_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'still_exact_match_after_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCacheExactMatchHits'] > 0 as exact_match_hit +FROM system.query_log +where log_comment = 'still_exact_match_after_shorter_prefix' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 +SELECT ProfileEvents['ObjectStorageListObjectsCacheHits'] > 0 as hit +FROM system.query_log +where log_comment = 'after_drop' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +0 +SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss +FROM system.query_log +where log_comment = 'after_drop' +AND type = 'QueryFinish' +ORDER BY event_time desc +LIMIT 1; +1 From e6b379e9e9bb5effd72bcbc7628538953abbd7d3 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 21 Apr 2025 16:11:43 -0300 Subject: [PATCH 22/46] remove cachedglobiterator in favor of expensive copy. I think I'll refactor --- .../StorageObjectStorageSource.cpp | 115 ++---------------- .../StorageObjectStorageSource.h | 6 +- 2 files changed, 14 insertions(+), 107 deletions(-) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 79d107db4c0d..64f2b076b73e 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -63,98 +63,6 @@ namespace ErrorCodes extern const int FILE_DOESNT_EXIST; } -struct CachedGlobObjectIterator : IObjectIterator, WithContext -{ - using Configuration = StorageObjectStorage::Configuration; - using ConfigurationPtr = StorageObjectStorage::ConfigurationPtr; - using ObjectInfos = StorageObjectStorage::ObjectInfos; - - CachedGlobObjectIterator( - const std::shared_ptr & object_infos_, - ConfigurationPtr configuration_, - const ActionsDAG::Node * predicate, - const NamesAndTypesList & virtual_columns, - ContextPtr context_, - std::size_t , - ObjectInfos * read_keys = nullptr) - : WithContext(context_), - object_infos(object_infos_), - configuration(configuration_) - { - if (configuration->isPathWithGlobs()) - { - const auto key_with_globs = configuration->getPath(); - - // - auto matcher = std::make_unique(makeRegexpPatternFromGlobs(key_with_globs)); - if (!matcher->ok()) - { - throw Exception(ErrorCodes::CANNOT_COMPILE_REGEXP, "Cannot compile regex from glob ({}): {}", key_with_globs, matcher->error()); - } - - ExpressionActionsPtr filter_expr; - recursive = key_with_globs == "/**"; - if (auto filter_dag = VirtualColumnUtils::createPathAndFileFilterDAG(predicate, virtual_columns)) - { - VirtualColumnUtils::buildSetsForDAG(*filter_dag, getContext()); - filter_expr = std::make_shared(std::move(*filter_dag)); - } - - for (auto it = object_infos->begin(); it != object_infos->end();) - { - if (!recursive && !re2::RE2::FullMatch((*it)->getPath(), *matcher)) - it = object_infos->erase(it); - else - ++it; - } - - if (filter_expr) - { - std::vector paths; - paths.reserve(object_infos->size()); - for (const auto & object_info : *object_infos) - paths.push_back(StorageObjectStorageSource::getUniqueStoragePathIdentifier(*configuration, *object_info, false)); - - VirtualColumnUtils::filterByPathOrFile(*object_infos, paths, filter_expr, virtual_columns, getContext()); - - LOG_TEST(getLogger("Arthur"), "Filtered files: {} -> {}", paths.size(), object_infos->size()); - } - - if (read_keys) - read_keys->insert(read_keys->end(), object_infos->begin(), object_infos->end()); - } - else - { - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Using glob iterator with path without globs is not allowed (used path: {})", - configuration->getPath()); - } - } - - ObjectInfoPtr next(size_t) override { - std::lock_guard lock(mutex); - - if (index >= object_infos->size()) { - return {}; - } - - return (*object_infos)[index++]; - } - - size_t estimatedKeysCount() override { - return object_infos->size(); - } - -private: - - std::shared_ptr object_infos; - std::mutex mutex; - size_t index = 0; - ConfigurationPtr configuration; - bool recursive {false}; -}; - StorageObjectStorageSource::StorageObjectStorageSource( String name_, ObjectStoragePtr object_storage_, @@ -256,22 +164,27 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( if (auto objects_info = cache.get(configuration->getNamespace(), configuration->getPathWithoutGlobs(), /*filter_by_prefix=*/ false)) { - iterator = std::make_unique(objects_info, configuration, predicate, virtual_columns, local_context, 0, read_keys); + RelativePathsWithMetadata loose_copy(*objects_info); + auto some_iterator = std::make_shared(std::move(loose_copy)); + iterator = std::make_unique( + some_iterator, configuration, predicate, virtual_columns, + local_context, is_archive ? nullptr : read_keys, + query_settings.throw_on_zero_files_match, file_progress_callback); } else { /// Iterate through disclosed globs and make a source for each file iterator = std::make_unique( - object_storage, configuration, predicate, virtual_columns, - local_context, is_archive ? nullptr : read_keys, query_settings.list_object_keys_size, + object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size), configuration, predicate, virtual_columns, + local_context, is_archive ? nullptr : read_keys, query_settings.throw_on_zero_files_match, file_progress_callback, &cache); } } else { /// Iterate through disclosed globs and make a source for each file iterator = std::make_unique( - object_storage, configuration, predicate, virtual_columns, - local_context, is_archive ? nullptr : read_keys, query_settings.list_object_keys_size, + object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size), configuration, predicate, virtual_columns, + local_context, is_archive ? nullptr : read_keys, query_settings.throw_on_zero_files_match, file_progress_callback, nullptr); } } @@ -767,18 +680,17 @@ std::unique_ptr StorageObjectStorageSource::createReadBu } StorageObjectStorageSource::GlobIterator::GlobIterator( - ObjectStoragePtr object_storage_, + const ObjectStorageIteratorPtr & object_storage_iterator_, ConfigurationPtr configuration_, const ActionsDAG::Node * predicate, const NamesAndTypesList & virtual_columns_, ContextPtr context_, ObjectInfos * read_keys_, - size_t list_object_keys_size, bool throw_on_zero_files_match_, std::function file_progress_callback_, ObjectStorageListObjectsCache * list_cache_) : WithContext(context_) - , object_storage(object_storage_) + , object_storage_iterator(object_storage_iterator_) , configuration(configuration_) , virtual_columns(virtual_columns_) , throw_on_zero_files_match(throw_on_zero_files_match_) @@ -795,9 +707,6 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( if (configuration->isPathWithGlobs()) { const auto key_with_globs = configuration_->getPath(); - const auto key_prefix = configuration->getPathWithoutGlobs(); - - object_storage_iterator = object_storage->iterate(key_prefix, list_object_keys_size); matcher = std::make_unique(makeRegexpPatternFromGlobs(key_with_globs)); if (!matcher->ok()) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 090379864367..0974b2abd80f 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -165,13 +165,12 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon { public: GlobIterator( - ObjectStoragePtr object_storage_, + const ObjectStorageIteratorPtr & object_storage_iterator_, ConfigurationPtr configuration_, const ActionsDAG::Node * predicate, const NamesAndTypesList & virtual_columns_, ContextPtr context_, ObjectInfos * read_keys_, - size_t list_object_keys_size, bool throw_on_zero_files_match_, std::function file_progress_callback_ = {}, ObjectStorageListObjectsCache * list_cache_ = nullptr); @@ -187,7 +186,7 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon void createFilterAST(const String & any_key); void fillBufferForKey(const std::string & uri_key); - const ObjectStoragePtr object_storage; + ObjectStorageIteratorPtr object_storage_iterator; const ConfigurationPtr configuration; const NamesAndTypesList virtual_columns; const bool throw_on_zero_files_match; @@ -198,7 +197,6 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon ObjectInfos object_infos; ObjectInfos * read_keys; ExpressionActionsPtr filter_expr; - ObjectStorageIteratorPtr object_storage_iterator; bool recursive{false}; std::vector expanded_keys; std::vector::iterator expanded_keys_iter; From d7b50f44797f1580a283427e7f870474ed61d632 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 21 Apr 2025 16:19:51 -0300 Subject: [PATCH 23/46] simplify things a bit --- .../StorageObjectStorageSource.cpp | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 64f2b076b73e..6c69e0cddb58 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -158,6 +158,9 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( } else { + std::shared_ptr object_iterator = nullptr; + ObjectStorageListObjectsCache * cache_ptr = nullptr; + if (local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache]) { auto & cache = ObjectStorageListObjectsCache::instance(); @@ -165,28 +168,24 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( if (auto objects_info = cache.get(configuration->getNamespace(), configuration->getPathWithoutGlobs(), /*filter_by_prefix=*/ false)) { RelativePathsWithMetadata loose_copy(*objects_info); - auto some_iterator = std::make_shared(std::move(loose_copy)); - iterator = std::make_unique( - some_iterator, configuration, predicate, virtual_columns, - local_context, is_archive ? nullptr : read_keys, - query_settings.throw_on_zero_files_match, file_progress_callback); + object_iterator = std::make_shared(std::move(loose_copy)); } else { - /// Iterate through disclosed globs and make a source for each file - iterator = std::make_unique( - object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size), configuration, predicate, virtual_columns, - local_context, is_archive ? nullptr : read_keys, - query_settings.throw_on_zero_files_match, file_progress_callback, &cache); + cache_ptr = &cache; + object_iterator = object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size); } } - else { - /// Iterate through disclosed globs and make a source for each file - iterator = std::make_unique( - object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size), configuration, predicate, virtual_columns, - local_context, is_archive ? nullptr : read_keys, - query_settings.throw_on_zero_files_match, file_progress_callback, nullptr); + else + { + object_iterator = object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size); } + + /// Iterate through disclosed globs and make a source for each file + iterator = std::make_unique( + object_iterator, configuration, predicate, virtual_columns, + local_context, is_archive ? nullptr : read_keys, + query_settings.throw_on_zero_files_match, file_progress_callback, cache_ptr); } } else if (configuration->supportsFileIterator()) From 6cfa5106d5fde5dd57d7e2e02d2e10bed1534486 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 22 Apr 2025 09:46:39 -0300 Subject: [PATCH 24/46] add some more tests --- ...03377_object_storage_list_objects_cache.reference | 10 ++++++++++ .../03377_object_storage_list_objects_cache.sql | 12 +++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference index 396959f0e1ed..76535ad25106 100644 --- a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference +++ b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.reference @@ -1,5 +1,15 @@ -- { echoOn } +-- The cached key should be `dir_`, and that includes all three files: 1, 2 and 3. Cache should return all three, but ClickHouse should filter out the third. +SELECT _path, * FROM s3(s3_conn, filename='dir_a/dir_b/t_03377_sample_{1..2}.parquet') order by id SETTINGS use_object_storage_list_objects_cache=1; +test/dir_a/dir_b/t_03377_sample_1.parquet 1 +test/dir_a/dir_b/t_03377_sample_2.parquet 2 +-- Make sure the filtering did not interfere with the cached values +SELECT _path, * FROM s3(s3_conn, filename='dir_a/dir_b/t_03377_sample_*.parquet') order by id SETTINGS use_object_storage_list_objects_cache=1; +test/dir_a/dir_b/t_03377_sample_1.parquet 1 +test/dir_a/dir_b/t_03377_sample_2.parquet 2 +test/dir_a/dir_b/t_03377_sample_3.parquet 3 +SYSTEM FLUSH LOGS; SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss FROM system.query_log where log_comment = 'cold_list_cache' diff --git a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql index 5436824bdf6a..9638faa88d23 100644 --- a/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql +++ b/tests/queries/0_stateless/03377_object_storage_list_objects_cache.sql @@ -2,7 +2,7 @@ SYSTEM DROP OBJECT STORAGE LIST OBJECTS CACHE; -INSERT INTO TABLE FUNCTION s3(s3_conn, filename='dir_a/dir_b/t_03377_sample.parquet', format='Parquet', structure='id UInt64') SETTINGS s3_truncate_on_insert=1 VALUES (1); +INSERT INTO TABLE FUNCTION s3(s3_conn, filename='dir_a/dir_b/t_03377_sample_{_partition_id}.parquet', format='Parquet', structure='id UInt64') PARTITION BY id SETTINGS s3_truncate_on_insert=1 VALUES (1), (2), (3); SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='cold_list_cache'; SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='warm_list_exact_cache'; @@ -13,10 +13,16 @@ SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_ob SYSTEM DROP OBJECT STORAGE LIST OBJECTS CACHE; SELECT * FROM s3(s3_conn, filename='dir_**.parquet') Format Null SETTINGS use_object_storage_list_objects_cache=1, log_comment='after_drop'; -SYSTEM FLUSH LOGS; - -- { echoOn } +-- The cached key should be `dir_`, and that includes all three files: 1, 2 and 3. Cache should return all three, but ClickHouse should filter out the third. +SELECT _path, * FROM s3(s3_conn, filename='dir_a/dir_b/t_03377_sample_{1..2}.parquet') order by id SETTINGS use_object_storage_list_objects_cache=1; + +-- Make sure the filtering did not interfere with the cached values +SELECT _path, * FROM s3(s3_conn, filename='dir_a/dir_b/t_03377_sample_*.parquet') order by id SETTINGS use_object_storage_list_objects_cache=1; + +SYSTEM FLUSH LOGS; + SELECT ProfileEvents['ObjectStorageListObjectsCacheMisses'] > 0 as miss FROM system.query_log where log_comment = 'cold_list_cache' From be8c6a1684c44709e071325a29ec59d7656d8f36 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 22 Apr 2025 10:01:01 -0300 Subject: [PATCH 25/46] make cache return a copy instead of a pointer, we don't want modifications --- .../Cache/ObjectStorageListObjectsCache.cpp | 13 +++++++------ src/Storages/Cache/ObjectStorageListObjectsCache.h | 2 +- .../ObjectStorage/StorageObjectStorageSource.cpp | 3 +-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 8cc8e5a2e26e..0197edc8b503 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -145,7 +145,7 @@ void ObjectStorageListObjectsCache::clear() cache.clear(); } -ObjectStorageListObjectsCache::Cache::MappedPtr ObjectStorageListObjectsCache::get(const String & bucket, const String & prefix, bool filter_by_prefix) +std::optional ObjectStorageListObjectsCache::get(const String & bucket, const String & prefix, bool filter_by_prefix) { const auto input_key = Key{bucket, prefix}; auto pair = cache.getWithKey(input_key); @@ -161,24 +161,25 @@ ObjectStorageListObjectsCache::Cache::MappedPtr ObjectStorageListObjectsCache::g if (pair->key == input_key) { ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCacheExactMatchHits); - return pair->mapped; + return *pair->mapped; } ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCachePrefixMatchHits); if (!filter_by_prefix) { - return pair->mapped; + return *pair->mapped; } - auto filtered_objects = std::make_shared>(); - filtered_objects->reserve(pair->mapped->size()); + Value filtered_objects; + + filtered_objects.reserve(pair->mapped->size()); for (const auto & object : *pair->mapped) { if (object->relative_path.starts_with(input_key.prefix)) { - filtered_objects->push_back(object); + filtered_objects.push_back(object); } } diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 89adbf874317..be0291dc7c17 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -52,7 +52,7 @@ class ObjectStorageListObjectsCache const std::string & prefix, const std::shared_ptr & value); - Cache::MappedPtr get(const String & bucket, const String & prefix, bool filter_by_prefix = true); + std::optional get(const String & bucket, const String & prefix, bool filter_by_prefix = true); void clear(); diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 6c69e0cddb58..ff623c0cdce8 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -167,8 +167,7 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( if (auto objects_info = cache.get(configuration->getNamespace(), configuration->getPathWithoutGlobs(), /*filter_by_prefix=*/ false)) { - RelativePathsWithMetadata loose_copy(*objects_info); - object_iterator = std::make_shared(std::move(loose_copy)); + object_iterator = std::make_shared(std::move(*objects_info)); } else { From b60cb959e4cc21a06f87803eaf287f721e46c317 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 22 Apr 2025 11:10:25 -0300 Subject: [PATCH 26/46] update ut --- ...test_object_storage_list_objects_cache.cpp | 46 +++++++++---------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index 5c480fba78d6..e62540a56415 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -37,10 +37,10 @@ TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) cache.set(bucket, prefix, value); - auto result = cache.get(bucket, prefix); - ASSERT_EQ(result->size(), 2); - EXPECT_EQ((*result)[0]->getPath(), "test-prefix/file1.txt"); - EXPECT_EQ((*result)[1]->getPath(), "test-prefix/file2.txt"); + auto result = cache.get(bucket, prefix).value(); + ASSERT_EQ(result.size(), 2); + EXPECT_EQ(result[0]->getPath(), "test-prefix/file1.txt"); + EXPECT_EQ(result[1]->getPath(), "test-prefix/file2.txt"); } TEST(ObjectStorageListObjectsCacheTest, CacheMiss) @@ -48,9 +48,8 @@ TEST(ObjectStorageListObjectsCacheTest, CacheMiss) cache.clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; - - auto result = cache.get(bucket, prefix); - EXPECT_EQ(result, nullptr); + + EXPECT_FALSE(cache.get(bucket, prefix)); } TEST(ObjectStorageListObjectsCacheTest, ClearCache) @@ -62,9 +61,8 @@ TEST(ObjectStorageListObjectsCacheTest, ClearCache) cache.set(bucket, prefix, value); cache.clear(); - - auto result = cache.get(bucket, prefix); - EXPECT_EQ(result, nullptr); + + EXPECT_FALSE(cache.get(bucket, prefix)); } TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) @@ -82,14 +80,13 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) cache.set(bucket, mid_prefix, value); - auto result1 = cache.get(bucket, mid_prefix); - EXPECT_EQ(result1->size(), 2); + auto result1 = cache.get(bucket, mid_prefix).value(); + EXPECT_EQ(result1.size(), 2); - auto result2 = cache.get(bucket, long_prefix); - EXPECT_EQ(result2->size(), 2); + auto result2 = cache.get(bucket, long_prefix).value(); + EXPECT_EQ(result2.size(), 2); - auto result3 = cache.get(bucket, short_prefix); - ASSERT_EQ(result3, nullptr); + EXPECT_FALSE(cache.get(bucket, short_prefix)); } TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) @@ -106,9 +103,9 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) cache.set(bucket, short_prefix, value); - auto result = cache.get(bucket, "parent/child1/", true); - EXPECT_EQ(result->size(), 1); - EXPECT_EQ((*result)[0]->getPath(), "parent/child1/file2.txt"); + auto result = cache.get(bucket, "parent/child1/", true).value(); + EXPECT_EQ(result.size(), 1); + EXPECT_EQ(result[0]->getPath(), "parent/child1/file2.txt"); } TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) @@ -121,13 +118,12 @@ TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) cache.set(bucket, prefix, value); // Verify we can get it immediately - auto result1 = cache.get(bucket, prefix); - EXPECT_EQ(result1->size(), 1); + auto result1 = cache.get(bucket, prefix).value(); + EXPECT_EQ(result1.size(), 1); std::this_thread::sleep_for(std::chrono::seconds(4)); - auto result2 = cache.get(bucket, prefix); - EXPECT_EQ(result2, nullptr); + EXPECT_FALSE(cache.get(bucket, prefix)); } TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) @@ -142,8 +138,8 @@ TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) cache.set(bucket, "a/b/c/", mid_prefix); // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size - auto result = cache.get(bucket, "a/b/c/d/", false); - EXPECT_EQ(result->size(), 2u); + auto result = cache.get(bucket, "a/b/c/d/", false).value(); + EXPECT_EQ(result.size(), 2u); } } From d91bf002ecf6b210866e50bc7603b9dc0aa961c9 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 22 Apr 2025 13:26:34 -0300 Subject: [PATCH 27/46] improve prefix matching by implementing search with time complexity of O(key_size) rather than O(cache_length) --- .../Cache/ObjectStorageListObjectsCache.cpp | 32 ++++++------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 0197edc8b503..cc70a728b005 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -49,40 +49,26 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy to_remove; - - for (auto it = cache.begin(); it != cache.end(); ++it) + while (!key.prefix.empty()) { - const auto & candidate_bucket = it->first.bucket; - const auto & candidate_prefix = it->first.prefix; - - if (candidate_bucket == key.bucket && prefix.starts_with(candidate_prefix)) + if (const auto it = cache.find(key); it != cache.end()) { if (IsStaleFunction()(it->first)) { - to_remove.push_back(it->first); - continue; + BasePolicy::remove(it->first); } - - if (candidate_prefix.size() > best_length) + else { - best_match = it; - best_length = candidate_prefix.size(); + return it; } } - } - for (const auto & k : to_remove) - BasePolicy::remove(k); + key.prefix.pop_back(); + } - return best_match; + return cache.end(); } }; From e0e19a20ea4a775ee87c1980ced9348577ab8b9d Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 2 May 2025 18:02:00 -0300 Subject: [PATCH 28/46] idraft impl of authorization aware cache --- src/Backups/BackupIO_AzureBlobStorage.cpp | 6 +- .../AzureBlobStorage/AzureObjectStorage.cpp | 40 ++++++++++- .../AzureBlobStorage/AzureObjectStorage.h | 7 +- .../Cached/CachedObjectStorage.h | 2 + .../ObjectStorages/HDFS/HDFSObjectStorage.h | 2 + src/Disks/ObjectStorages/IObjectStorage.h | 3 + .../ObjectStorages/Local/LocalObjectStorage.h | 3 + .../ObjectStorages/ObjectStorageFactory.cpp | 3 +- .../ObjectStorages/S3/S3ObjectStorage.cpp | 7 ++ src/Disks/ObjectStorages/S3/S3ObjectStorage.h | 2 + .../ObjectStorages/Web/WebObjectStorage.h | 3 + .../Cache/ObjectStorageListObjectsCache.cpp | 15 ++--- .../Cache/ObjectStorageListObjectsCache.h | 10 +-- ...test_object_storage_list_objects_cache.cpp | 67 ++++++++++++------- .../ObjectStorage/Azure/Configuration.cpp | 3 +- .../StorageObjectStorageSource.cpp | 17 ++--- .../StorageObjectStorageSource.h | 18 ++++- 17 files changed, 154 insertions(+), 54 deletions(-) diff --git a/src/Backups/BackupIO_AzureBlobStorage.cpp b/src/Backups/BackupIO_AzureBlobStorage.cpp index 449ec587d866..e603dd540758 100644 --- a/src/Backups/BackupIO_AzureBlobStorage.cpp +++ b/src/Backups/BackupIO_AzureBlobStorage.cpp @@ -47,7 +47,8 @@ BackupReaderAzureBlobStorage::BackupReaderAzureBlobStorage( std::move(client_ptr), std::move(settings_ptr), connection_params.getContainer(), - connection_params.getConnectionURL()); + connection_params.getConnectionURL(), + connection_params.auth_method); client = object_storage->getAzureBlobStorageClient(); settings = object_storage->getSettings(); @@ -142,7 +143,8 @@ BackupWriterAzureBlobStorage::BackupWriterAzureBlobStorage( std::move(client_ptr), std::move(settings_ptr), connection_params.getContainer(), - connection_params.getConnectionURL()); + connection_params.getConnectionURL(), + connection_params.auth_method); client = object_storage->getAzureBlobStorageClient(); settings = object_storage->getSettings(); diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp index 93ceddde7498..d434e4f543fb 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp @@ -108,12 +108,14 @@ AzureObjectStorage::AzureObjectStorage( ClientPtr && client_, SettingsPtr && settings_, const String & object_namespace_, - const String & description_) + const String & description_, + AzureBlobStorage::AuthMethod auth_method_) : name(name_) , client(std::move(client_)) , settings(std::move(settings_)) , object_namespace(object_namespace_) , description(description_) + , auth_method(auth_method_) , log(getLogger("AzureObjectStorage")) { } @@ -154,6 +156,40 @@ ObjectStorageIteratorPtr AzureObjectStorage::iterate(const std::string & path_pr return std::make_shared(path_prefix, client_ptr, max_keys ? max_keys : settings_ptr->list_object_keys_size); } +std::optional AzureObjectStorage::getIdentityFingerprint() const +{ + std::optional fingerprint; + + std::visit([&fingerprint](const auto & auth) { + using T = std::decay_t; + + if constexpr (std::is_same_v) + { + auto connection_string_parts = Azure::Storage::_internal::ParseConnectionString(auth); + fingerprint = std::to_string(std::hash()(connection_string_parts.AccountName)); + } + else if constexpr (std::is_same_v>) + { + if (auth) + { + fingerprint = std::to_string(std::hash()(auth->AccountName)); + } + } + /// I am not sure what to do with the other auth methods, needs further investigation + // else if constexpr (std::is_same_v>) { + // } + // else if constexpr (std::is_same_v>) { + // } + }, auth_method); + + if (!fingerprint) + { + return std::nullopt; + } + + return getName() + fingerprint.value(); +} + void AzureObjectStorage::listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t max_keys) const { auto client_ptr = client.get(); @@ -380,7 +416,7 @@ std::unique_ptr AzureObjectStorage::cloneObjectStorage( }; auto new_client = AzureBlobStorage::getContainerClient(params, /*readonly=*/ true); - return std::make_unique(name, std::move(new_client), std::move(new_settings), new_namespace, params.endpoint.getServiceEndpoint()); + return std::make_unique(name, std::move(new_client), std::move(new_settings), new_namespace, params.endpoint.getServiceEndpoint(), params.auth_method); } } diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h index 5f91a18e4d97..33d6f0906115 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h @@ -29,7 +29,8 @@ class AzureObjectStorage : public IObjectStorage ClientPtr && client_, SettingsPtr && settings_, const String & object_namespace_, - const String & description_); + const String & description_, + AzureBlobStorage::AuthMethod auth_method_); void listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t max_keys) const override; @@ -37,6 +38,8 @@ class AzureObjectStorage : public IObjectStorage std::string getName() const override { return "AzureObjectStorage"; } + std::optional getIdentityFingerprint() const override; + ObjectStorageType getType() const override { return ObjectStorageType::Azure; } std::string getCommonKeyPrefix() const override { return ""; } @@ -116,6 +119,8 @@ class AzureObjectStorage : public IObjectStorage /// We use source url without container and prefix as description, because in Azure there are no limitations for operations between different containers. const String description; + AzureBlobStorage::AuthMethod auth_method; + LoggerPtr log; }; diff --git a/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h b/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h index 00f236c1e89d..849dca3cff01 100644 --- a/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h +++ b/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h @@ -31,6 +31,8 @@ class CachedObjectStorage final : public IObjectStorage bool exists(const StoredObject & object) const override; + std::optional getIdentityFingerprint() const override { return object_storage->getIdentityFingerprint(); } + std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h b/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h index 28b75d44f21e..40d6dcecff6d 100644 --- a/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h +++ b/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h @@ -65,6 +65,8 @@ class HDFSObjectStorage : public IObjectStorage, public HDFSErrorWrapper bool exists(const StoredObject & object) const override; + std::optional getIdentityFingerprint() const override { return std::nullopt; } + std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/IObjectStorage.h b/src/Disks/ObjectStorages/IObjectStorage.h index f99864ebb25c..8cd09e1fa852 100644 --- a/src/Disks/ObjectStorages/IObjectStorage.h +++ b/src/Disks/ObjectStorages/IObjectStorage.h @@ -126,6 +126,9 @@ class IObjectStorage virtual std::string getDescription() const = 0; + // todo arthur add docs + virtual std::optional getIdentityFingerprint() const = 0; + virtual const MetadataStorageMetrics & getMetadataStorageMetrics() const; /// Object exists or not diff --git a/src/Disks/ObjectStorages/Local/LocalObjectStorage.h b/src/Disks/ObjectStorages/Local/LocalObjectStorage.h index 50037aa3ce91..82634441c37e 100644 --- a/src/Disks/ObjectStorages/Local/LocalObjectStorage.h +++ b/src/Disks/ObjectStorages/Local/LocalObjectStorage.h @@ -28,6 +28,9 @@ class LocalObjectStorage : public IObjectStorage bool exists(const StoredObject & object) const override; + // no auth + std::optional getIdentityFingerprint() const override { return getName(); } + std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/ObjectStorageFactory.cpp b/src/Disks/ObjectStorages/ObjectStorageFactory.cpp index 69b4627dd74c..4311fa820656 100644 --- a/src/Disks/ObjectStorages/ObjectStorageFactory.cpp +++ b/src/Disks/ObjectStorages/ObjectStorageFactory.cpp @@ -307,7 +307,8 @@ void registerAzureObjectStorage(ObjectStorageFactory & factory) ObjectStorageType::Azure, config, config_prefix, name, AzureBlobStorage::getContainerClient(params, /*readonly=*/ false), std::move(azure_settings), params.endpoint.prefix.empty() ? params.endpoint.container_name : params.endpoint.container_name + "/" + params.endpoint.prefix, - params.endpoint.getServiceEndpoint()); + params.endpoint.getServiceEndpoint(), + params.auth_method); }; factory.registerObjectStorageType("azure_blob_storage", creator); diff --git a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp index 0773833712d9..da4b0d422147 100644 --- a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp +++ b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp @@ -164,6 +164,13 @@ bool S3ObjectStorage::exists(const StoredObject & object) const return S3::objectExists(*client.get(), uri.bucket, object.remote_path, {}); } +std::optional S3ObjectStorage::getIdentityFingerprint() const +{ + const auto credentials = client.get()->getCredentials(); + + return getName() + credentials.GetAWSAccessKeyId(); +} + std::unique_ptr S3ObjectStorage::readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/S3/S3ObjectStorage.h b/src/Disks/ObjectStorages/S3/S3ObjectStorage.h index f8811785ed39..1be039b5d5b3 100644 --- a/src/Disks/ObjectStorages/S3/S3ObjectStorage.h +++ b/src/Disks/ObjectStorages/S3/S3ObjectStorage.h @@ -81,6 +81,8 @@ class S3ObjectStorage : public IObjectStorage ObjectStorageType getType() const override { return ObjectStorageType::S3; } + std::optional getIdentityFingerprint() const override; + bool exists(const StoredObject & object) const override; std::unique_ptr readObject( /// NOLINT diff --git a/src/Disks/ObjectStorages/Web/WebObjectStorage.h b/src/Disks/ObjectStorages/Web/WebObjectStorage.h index c96b0bd1e724..4c5bca68edac 100644 --- a/src/Disks/ObjectStorages/Web/WebObjectStorage.h +++ b/src/Disks/ObjectStorages/Web/WebObjectStorage.h @@ -33,6 +33,9 @@ class WebObjectStorage : public IObjectStorage, WithContext bool exists(const StoredObject & object) const override; + // apparently no auth is required for this object storage type + std::optional getIdentityFingerprint() const override { return getName(); } + std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index cc70a728b005..08c4ce7a1780 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -73,21 +73,23 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy user_id_) - : bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} + : storage_identity(storage_identity_), bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} bool ObjectStorageListObjectsCache::Key::operator==(const Key & other) const { - return bucket == other.bucket && prefix == other.prefix; + return storage_identity == other.storage_identity && bucket == other.bucket && prefix == other.prefix; } size_t ObjectStorageListObjectsCache::KeyHasher::operator()(const Key & key) const { std::size_t seed = 0; + boost::hash_combine(seed, key.storage_identity); boost::hash_combine(seed, key.bucket); boost::hash_combine(seed, key.prefix); @@ -117,12 +119,10 @@ ObjectStorageListObjectsCache::ObjectStorageListObjectsCache() } void ObjectStorageListObjectsCache::set( - const std::string & bucket, - const std::string & prefix, + Key key, const std::shared_ptr & value) { - const auto key = Key{bucket, prefix, std::chrono::steady_clock::now() + std::chrono::seconds(ttl_in_seconds)}; - + key.expires_at = std::chrono::steady_clock::now() + std::chrono::seconds(ttl_in_seconds); cache.set(key, value); } @@ -131,9 +131,8 @@ void ObjectStorageListObjectsCache::clear() cache.clear(); } -std::optional ObjectStorageListObjectsCache::get(const String & bucket, const String & prefix, bool filter_by_prefix) +std::optional ObjectStorageListObjectsCache::get(const Key & input_key, bool filter_by_prefix) { - const auto input_key = Key{bucket, prefix}; auto pair = cache.getWithKey(input_key); if (!pair) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index be0291dc7c17..e3d0ef9cbf3c 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -16,11 +16,14 @@ class ObjectStorageListObjectsCache struct Key { Key( + const String & storage_identity_, const String & bucket_, const String & prefix_, const std::chrono::steady_clock::time_point & expires_at_ = std::chrono::steady_clock::now(), std::optional user_id_ = std::nullopt); + // object storage name + account name + std::string storage_identity; std::string bucket; std::string prefix; std::chrono::steady_clock::time_point expires_at; @@ -47,12 +50,9 @@ class ObjectStorageListObjectsCache using Cache = CacheBase; - void set( - const std::string & bucket, - const std::string & prefix, - const std::shared_ptr & value); + void set(Key key,const std::shared_ptr & value); - std::optional get(const String & bucket, const String & prefix, bool filter_by_prefix = true); + std::optional get(const Key & input_key, bool filter_by_prefix = true); void clear(); diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index e62540a56415..004f83443ac3 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -17,6 +17,8 @@ auto & initializeAndGetCacheInstance() static auto & cache = initializeAndGetCacheInstance(); +ObjectStorageListObjectsCache::Key default_key {"default", "test-bucket", "test-prefix/"}; + std::shared_ptr createTestValue(const std::vector& paths) { auto value = std::make_shared(); @@ -35,9 +37,9 @@ TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(bucket, prefix, value); + cache.set(default_key, value); - auto result = cache.get(bucket, prefix).value(); + auto result = cache.get(default_key).value(); ASSERT_EQ(result.size(), 2); EXPECT_EQ(result[0]->getPath(), "test-prefix/file1.txt"); EXPECT_EQ(result[1]->getPath(), "test-prefix/file2.txt"); @@ -49,7 +51,7 @@ TEST(ObjectStorageListObjectsCacheTest, CacheMiss) const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; - EXPECT_FALSE(cache.get(bucket, prefix)); + EXPECT_FALSE(cache.get(default_key)); } TEST(ObjectStorageListObjectsCacheTest, ClearCache) @@ -59,41 +61,50 @@ TEST(ObjectStorageListObjectsCacheTest, ClearCache) const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(bucket, prefix, value); + cache.set(default_key, value); cache.clear(); - EXPECT_FALSE(cache.get(bucket, prefix)); + EXPECT_FALSE(cache.get(default_key)); } TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) { cache.clear(); - const std::string bucket = "test-bucket"; - const std::string short_prefix = "parent/"; - const std::string mid_prefix = "parent/child/"; - const std::string long_prefix = "parent/child/grandchild/"; + + auto short_prefix_key = default_key; + short_prefix_key.prefix = "parent/"; + + auto mid_prefix_key = default_key; + mid_prefix_key.prefix = "parent/child/"; + + auto long_prefix_key = default_key; + long_prefix_key.prefix = "parent/child/grandchild/"; auto value = createTestValue( { "parent/child/grandchild/file1.txt", "parent/child/grandchild/file2.txt"}); - cache.set(bucket, mid_prefix, value); + cache.set(mid_prefix_key, value); - auto result1 = cache.get(bucket, mid_prefix).value(); + auto result1 = cache.get(mid_prefix_key).value(); EXPECT_EQ(result1.size(), 2); - auto result2 = cache.get(bucket, long_prefix).value(); + auto result2 = cache.get(long_prefix_key).value(); EXPECT_EQ(result2.size(), 2); - EXPECT_FALSE(cache.get(bucket, short_prefix)); + EXPECT_FALSE(cache.get(short_prefix_key)); } TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) { cache.clear(); - const std::string bucket = "test-bucket"; - const std::string short_prefix = "parent/"; + + auto key_with_short_prefix = default_key; + key_with_short_prefix.prefix = "parent/"; + + auto key_with_mid_prefix = default_key; + key_with_mid_prefix.prefix = "parent/child1/"; auto value = createTestValue({ "parent/file1.txt", @@ -101,9 +112,9 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) "parent/child2/file3.txt" }); - cache.set(bucket, short_prefix, value); + cache.set(key_with_short_prefix, value); - auto result = cache.get(bucket, "parent/child1/", true).value(); + auto result = cache.get(key_with_mid_prefix, true).value(); EXPECT_EQ(result.size(), 1); EXPECT_EQ(result[0]->getPath(), "parent/child1/file2.txt"); } @@ -115,30 +126,38 @@ TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); - cache.set(bucket, prefix, value); + cache.set(default_key, value); // Verify we can get it immediately - auto result1 = cache.get(bucket, prefix).value(); + auto result1 = cache.get(default_key).value(); EXPECT_EQ(result1.size(), 1); std::this_thread::sleep_for(std::chrono::seconds(4)); - EXPECT_FALSE(cache.get(bucket, prefix)); + EXPECT_FALSE(cache.get(default_key)); } TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) { cache.clear(); - const std::string bucket = "test-bucket"; + + auto short_prefix_key = default_key; + short_prefix_key.prefix = "a/b/"; + + auto mid_prefix_key = default_key; + mid_prefix_key.prefix = "a/b/c/"; + + auto long_prefix_key = default_key; + long_prefix_key.prefix = "a/b/c/d/"; auto short_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt", "a/b/file2.txt"}); auto mid_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt"}); - cache.set(bucket, "a/b/", short_prefix); - cache.set(bucket, "a/b/c/", mid_prefix); + cache.set(short_prefix_key, short_prefix); + cache.set(mid_prefix_key, mid_prefix); // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size - auto result = cache.get(bucket, "a/b/c/d/", false).value(); + auto result = cache.get(long_prefix_key, false).value(); EXPECT_EQ(result.size(), 2u); } diff --git a/src/Storages/ObjectStorage/Azure/Configuration.cpp b/src/Storages/ObjectStorage/Azure/Configuration.cpp index 2985be6c1fcb..011e03107700 100644 --- a/src/Storages/ObjectStorage/Azure/Configuration.cpp +++ b/src/Storages/ObjectStorage/Azure/Configuration.cpp @@ -99,7 +99,8 @@ ObjectStoragePtr StorageAzureConfiguration::createObjectStorage(ContextPtr conte connection_params.createForContainer(), std::move(settings), connection_params.getContainer(), - connection_params.getConnectionURL()); + connection_params.getConnectionURL(), + connection_params.auth_method); } static AzureBlobStorage::ConnectionParams getConnectionParams( diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index ff623c0cdce8..72270dd2d302 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -159,19 +159,20 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( else { std::shared_ptr object_iterator = nullptr; - ObjectStorageListObjectsCache * cache_ptr = nullptr; + std::unique_ptr cache_ptr; - if (local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache]) + if (const auto identity_fingerprint = object_storage->getIdentityFingerprint(); local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache] && identity_fingerprint) { auto & cache = ObjectStorageListObjectsCache::instance(); + auto cache_key = ObjectStorageListObjectsCache::Key {*identity_fingerprint, configuration->getNamespace(), configuration->getPathWithoutGlobs()}; - if (auto objects_info = cache.get(configuration->getNamespace(), configuration->getPathWithoutGlobs(), /*filter_by_prefix=*/ false)) + if (auto objects_info = cache.get(cache_key, /*filter_by_prefix*/ false)) { object_iterator = std::make_shared(std::move(*objects_info)); } else { - cache_ptr = &cache; + cache_ptr = std::make_unique(&cache, cache_key); object_iterator = object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size); } } @@ -184,7 +185,7 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( iterator = std::make_unique( object_iterator, configuration, predicate, virtual_columns, local_context, is_archive ? nullptr : read_keys, - query_settings.throw_on_zero_files_match, file_progress_callback, cache_ptr); + query_settings.throw_on_zero_files_match, file_progress_callback, std::move(cache_ptr)); } } else if (configuration->supportsFileIterator()) @@ -686,7 +687,7 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( ObjectInfos * read_keys_, bool throw_on_zero_files_match_, std::function file_progress_callback_, - ObjectStorageListObjectsCache * list_cache_) + std::unique_ptr list_cache_) : WithContext(context_) , object_storage_iterator(object_storage_iterator_) , configuration(configuration_) @@ -696,7 +697,7 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( , read_keys(read_keys_) , local_context(context_) , file_progress_callback(file_progress_callback_) - , list_cache(list_cache_) + , list_cache(std::move(list_cache_)) { if (configuration->isNamespaceWithGlobs()) { @@ -771,7 +772,7 @@ StorageObjectStorage::ObjectInfoPtr StorageObjectStorageSource::GlobIterator::ne { if (list_cache) { - list_cache->set(configuration->getNamespace(), configuration->getPathWithoutGlobs(), std::make_shared(std::move(object_list))); + list_cache->set(std::move(object_list)); } is_finished = true; return {}; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 0974b2abd80f..11169cc8b253 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -164,6 +164,20 @@ class StorageObjectStorageSource::ReadTaskIterator : public IObjectIterator class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithContext { public: + struct ListObjectsCacheWithKey + { + ListObjectsCacheWithKey(ObjectStorageListObjectsCache * cache_, const ObjectStorageListObjectsCache::Key & key_) : cache(cache_), key(key_) {} + + void set(ObjectStorageListObjectsCache::Value && value) const + { + cache->set(key, std::make_shared(std::move(value))); + } + + private: + ObjectStorageListObjectsCache * cache; + ObjectStorageListObjectsCache::Key key; + }; + GlobIterator( const ObjectStorageIteratorPtr & object_storage_iterator_, ConfigurationPtr configuration_, @@ -173,7 +187,7 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon ObjectInfos * read_keys_, bool throw_on_zero_files_match_, std::function file_progress_callback_ = {}, - ObjectStorageListObjectsCache * list_cache_ = nullptr); + std::unique_ptr list_cache_ = nullptr); ~GlobIterator() override = default; @@ -209,7 +223,7 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon const ContextPtr local_context; std::function file_progress_callback; - ObjectStorageListObjectsCache * list_cache; + std::unique_ptr list_cache; ObjectInfos object_list; }; From 45af8a507d6a01b94c05da5449e0488e7ea589c4 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 2 May 2025 19:21:51 -0300 Subject: [PATCH 29/46] rename setting --- src/Common/ProfileEvents.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Common/ProfileEvents.cpp b/src/Common/ProfileEvents.cpp index 01b82581e44f..ec7aa2eb0224 100644 --- a/src/Common/ProfileEvents.cpp +++ b/src/Common/ProfileEvents.cpp @@ -966,8 +966,8 @@ The server successfully detected this situation and will download merged part fr M(ParquetMetaDataCacheMisses, "Number of times the read from filesystem cache miss the cache.", ValueType::Number) \ M(ObjectStorageListObjectsCacheHits, "Number of times object storage list objects operation hit the cache.", ValueType::Number) \ M(ObjectStorageListObjectsCacheMisses, "Number of times object storage list objects operation miss the cache.", ValueType::Number) \ - M(ObjectStorageListObjectsCacheExactMatchHits, "Number of times the read from filesystem cache hit the cache with an exact match.", ValueType::Number) \ - M(ObjectStorageListObjectsCachePrefixMatchHits, "Number of times the read from filesystem cache miss the cache using prefix matching.", ValueType::Number) \ + M(ObjectStorageListObjectsCacheExactMatchHits, "Number of times object storage list objects operation hit the cache with an exact match.", ValueType::Number) \ + M(ObjectStorageListObjectsCachePrefixMatchHits, "Number of times object storage list objects operation miss the cache using prefix matching.", ValueType::Number) \ #ifdef APPLY_FOR_EXTERNAL_EVENTS #define APPLY_FOR_EVENTS(M) APPLY_FOR_BUILTIN_EVENTS(M) APPLY_FOR_EXTERNAL_EVENTS(M) From 7266d920cfe3012f7d7927a7962592d7116507dc Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Fri, 2 May 2025 19:24:45 -0300 Subject: [PATCH 30/46] delete copy/move constructors and assignment operators --- src/Storages/Cache/ObjectStorageListObjectsCache.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index e3d0ef9cbf3c..8fc741804bf0 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -11,6 +11,12 @@ namespace DB class ObjectStorageListObjectsCache { public: + ObjectStorageListObjectsCache(const ObjectStorageListObjectsCache &) = delete; + ObjectStorageListObjectsCache(ObjectStorageListObjectsCache &&) noexcept = delete; + + ObjectStorageListObjectsCache& operator=(const ObjectStorageListObjectsCache &) = delete; + ObjectStorageListObjectsCache& operator=(ObjectStorageListObjectsCache &&) noexcept = delete; + static ObjectStorageListObjectsCache & instance(); struct Key From 8e78b28943cb7a816aae5d64afe89c484ffc1f4b Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 5 May 2025 13:52:11 -0300 Subject: [PATCH 31/46] azure impl and fix aws --- .../registerBackupEngineAzureBlobStorage.cpp | 2 +- .../AzureBlobStorageCommon.cpp | 9 ++++++++- .../AzureBlobStorage/AzureBlobStorageCommon.h | 18 +++++++++++++++++- .../AzureBlobStorage/AzureObjectStorage.cpp | 17 ++++++++--------- .../ObjectStorages/S3/S3ObjectStorage.cpp | 2 +- .../ObjectStorage/Azure/Configuration.cpp | 2 +- 6 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/Backups/registerBackupEngineAzureBlobStorage.cpp b/src/Backups/registerBackupEngineAzureBlobStorage.cpp index a9e267db1d47..3d0a11c417b9 100644 --- a/src/Backups/registerBackupEngineAzureBlobStorage.cpp +++ b/src/Backups/registerBackupEngineAzureBlobStorage.cpp @@ -96,7 +96,7 @@ void registerBackupEngineAzureBlobStorage(BackupFactory & factory) auto account_name = args[3].safeGet(); auto account_key = args[4].safeGet(); - connection_params.auth_method = std::make_shared(account_name, account_key); + connection_params.auth_method = std::make_shared(account_name, account_key); connection_params.client_options = AzureBlobStorage::getClientOptions(*request_settings, /*for_disk=*/ true); } else diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp index 869fa32819d3..ebbbbbe4dbac 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp @@ -146,6 +146,8 @@ std::unique_ptr ConnectionParams::createForService() const { if constexpr (std::is_same_v) return std::make_unique(ServiceClient::CreateFromConnectionString(auth.toUnderType(), client_options)); + else if constexpr (std::is_same_v>) + return std::make_unique(endpoint.getServiceEndpoint(), auth->impl, client_options); else return std::make_unique(endpoint.getServiceEndpoint(), auth, client_options); }, auth_method); @@ -166,6 +168,11 @@ std::unique_ptr ConnectionParams::createForContainer() const auto raw_client = RawContainerClient::CreateFromConnectionString(auth.toUnderType(), endpoint.container_name, client_options); return std::make_unique(std::move(raw_client), endpoint.prefix); } + else if constexpr (std::is_same_v>) + { + RawContainerClient raw_client{endpoint.getContainerEndpoint(), auth->impl, client_options}; + return std::make_unique(std::move(raw_client), endpoint.prefix); + } else { RawContainerClient raw_client{endpoint.getContainerEndpoint(), auth, client_options}; @@ -369,7 +376,7 @@ AuthMethod getAuthMethod(const Poco::Util::AbstractConfiguration & config, const { if (config.has(config_prefix + ".account_key") && config.has(config_prefix + ".account_name")) { - return std::make_shared( + return std::make_shared( config.getString(config_prefix + ".account_name"), config.getString(config_prefix + ".account_key") ); diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h index d64b44809e6c..547f630a9110 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h @@ -126,9 +126,25 @@ using ServiceClient = Azure::Storage::Blobs::BlobServiceClient; using BlobClientOptions = Azure::Storage::Blobs::BlobClientOptions; using ConnectionString = StrongTypedef; +/* + * In order to implement `AzureObjectStorage::getIdentityFingerprint()` for `StorageSharedKeyCredential`, we need to + * access `account_key`. The problem is that `Azure::Storage::StorageSharedKeyCredential::AccessKey` is private and the + * class is final inside `Azure SDK`. + */ +struct StorageSharedKeyCredentialWithAccessToSecret +{ + StorageSharedKeyCredentialWithAccessToSecret(const String & account_name_, const String & account_key_) + : account_name(account_name_), account_key(account_key_), impl(std::make_shared(account_name, account_key)) + {} + + const std::string account_name; + const std::string account_key; + std::shared_ptr impl; +}; + using AuthMethod = std::variant< ConnectionString, - std::shared_ptr, + std::shared_ptr, std::shared_ptr, std::shared_ptr>; diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp index d434e4f543fb..8048cb33b1fd 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp @@ -156,30 +156,29 @@ ObjectStorageIteratorPtr AzureObjectStorage::iterate(const std::string & path_pr return std::make_shared(path_prefix, client_ptr, max_keys ? max_keys : settings_ptr->list_object_keys_size); } +/* + * Only `ConnectionString` and `StorageSharedKeyCredential` auth methods are supported for now. + */ std::optional AzureObjectStorage::getIdentityFingerprint() const { std::optional fingerprint; - std::visit([&fingerprint](const auto & auth) { + std::visit([&fingerprint](const auto & auth) + { using T = std::decay_t; if constexpr (std::is_same_v) { auto connection_string_parts = Azure::Storage::_internal::ParseConnectionString(auth); - fingerprint = std::to_string(std::hash()(connection_string_parts.AccountName)); + fingerprint = connection_string_parts.AccountName + connection_string_parts.AccountKey; } - else if constexpr (std::is_same_v>) + else if constexpr (std::is_same_v>) { if (auth) { - fingerprint = std::to_string(std::hash()(auth->AccountName)); + fingerprint = auth->account_name + auth->account_key; } } - /// I am not sure what to do with the other auth methods, needs further investigation - // else if constexpr (std::is_same_v>) { - // } - // else if constexpr (std::is_same_v>) { - // } }, auth_method); if (!fingerprint) diff --git a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp index da4b0d422147..c7c953ca8869 100644 --- a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp +++ b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp @@ -168,7 +168,7 @@ std::optional S3ObjectStorage::getIdentityFingerprint() const { const auto credentials = client.get()->getCredentials(); - return getName() + credentials.GetAWSAccessKeyId(); + return getName() + credentials.GetAWSAccessKeyId() + credentials.GetAWSSecretKey(); } std::unique_ptr S3ObjectStorage::readObject( /// NOLINT diff --git a/src/Storages/ObjectStorage/Azure/Configuration.cpp b/src/Storages/ObjectStorage/Azure/Configuration.cpp index 011e03107700..74579f9df6a9 100644 --- a/src/Storages/ObjectStorage/Azure/Configuration.cpp +++ b/src/Storages/ObjectStorage/Azure/Configuration.cpp @@ -117,7 +117,7 @@ static AzureBlobStorage::ConnectionParams getConnectionParams( { connection_params.endpoint.storage_account_url = connection_url; connection_params.endpoint.container_name = container_name; - connection_params.auth_method = std::make_shared(*account_name, *account_key); + connection_params.auth_method = std::make_shared(*account_name, *account_key); connection_params.client_options = AzureBlobStorage::getClientOptions(*request_settings, /*for_disk=*/ false); } else From 2ed102d67c5dde5487f6043bbfeadd52337523c6 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 5 May 2025 14:03:32 -0300 Subject: [PATCH 32/46] docs --- src/Core/ServerSettings.cpp | 2 +- src/Disks/ObjectStorages/IObjectStorage.h | 2 +- src/Storages/Cache/ObjectStorageListObjectsCache.h | 2 +- .../ObjectStorage/StorageObjectStorageSource.cpp | 2 +- src/Storages/ObjectStorage/StorageObjectStorageSource.h | 9 ++++++--- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/Core/ServerSettings.cpp b/src/Core/ServerSettings.cpp index b9dd72418e5f..bb60b66082b1 100644 --- a/src/Core/ServerSettings.cpp +++ b/src/Core/ServerSettings.cpp @@ -1005,7 +1005,7 @@ namespace DB )", 0) \ DECLARE(Bool, storage_shared_set_join_use_inner_uuid, false, "If enabled, an inner UUID is generated during the creation of SharedSet and SharedJoin. ClickHouse Cloud only", 0) \ DECLARE(UInt64, input_format_parquet_metadata_cache_max_size, 500000000, "Maximum size of parquet file metadata cache", 0) \ - DECLARE(UInt64, object_storage_list_objects_cache_size, 5000000, "Maximum size of ObjectStorage list objects cache in bytes. Zero means disabled.", 0) \ + DECLARE(UInt64, object_storage_list_objects_cache_size, 500000000, "Maximum size of ObjectStorage list objects cache in bytes. Zero means disabled.", 0) \ DECLARE(UInt64, object_storage_list_objects_cache_max_entries, 1000, "Maximum size of ObjectStorage list objects cache in entries. Zero means disabled.", 0) \ DECLARE(UInt64, object_storage_list_objects_cache_ttl, 3600, "Time to live of records in ObjectStorage list objects cache in seconds. Zero means unlimited", 0) \ // clang-format on diff --git a/src/Disks/ObjectStorages/IObjectStorage.h b/src/Disks/ObjectStorages/IObjectStorage.h index 8cd09e1fa852..17ad96c1e7a6 100644 --- a/src/Disks/ObjectStorages/IObjectStorage.h +++ b/src/Disks/ObjectStorages/IObjectStorage.h @@ -126,7 +126,7 @@ class IObjectStorage virtual std::string getDescription() const = 0; - // todo arthur add docs + /// Storage name + authentication unique id. Used for securing ListObjects cache virtual std::optional getIdentityFingerprint() const = 0; virtual const MetadataStorageMetrics & getMetadataStorageMetrics() const; diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 8fc741804bf0..0a68e2745f01 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -28,7 +28,7 @@ class ObjectStorageListObjectsCache const std::chrono::steady_clock::time_point & expires_at_ = std::chrono::steady_clock::now(), std::optional user_id_ = std::nullopt); - // object storage name + account name + // object storage name + account std::string storage_identity; std::string bucket; std::string prefix; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 72270dd2d302..4025b30e15af 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -172,7 +172,7 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( } else { - cache_ptr = std::make_unique(&cache, cache_key); + cache_ptr = std::make_unique(cache, cache_key); object_iterator = object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size); } } diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 11169cc8b253..18e99ab1ae8a 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -164,17 +164,20 @@ class StorageObjectStorageSource::ReadTaskIterator : public IObjectIterator class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithContext { public: + /* + * Helper struct so `GlobIterator` does not need to re-construct the cache key all the time + */ struct ListObjectsCacheWithKey { - ListObjectsCacheWithKey(ObjectStorageListObjectsCache * cache_, const ObjectStorageListObjectsCache::Key & key_) : cache(cache_), key(key_) {} + ListObjectsCacheWithKey(ObjectStorageListObjectsCache & cache_, const ObjectStorageListObjectsCache::Key & key_) : cache(cache_), key(key_) {} void set(ObjectStorageListObjectsCache::Value && value) const { - cache->set(key, std::make_shared(std::move(value))); + cache.set(key, std::make_shared(std::move(value))); } private: - ObjectStorageListObjectsCache * cache; + ObjectStorageListObjectsCache & cache; ObjectStorageListObjectsCache::Key key; }; From 28bfcfb1aea57f5a9016eee3604933cf63f958be Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 5 May 2025 15:24:44 -0300 Subject: [PATCH 33/46] fix tests --- tests/queries/0_stateless/01271_show_privileges.reference | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/queries/0_stateless/01271_show_privileges.reference b/tests/queries/0_stateless/01271_show_privileges.reference index c56b1fd8bfb0..570950553679 100644 --- a/tests/queries/0_stateless/01271_show_privileges.reference +++ b/tests/queries/0_stateless/01271_show_privileges.reference @@ -132,6 +132,7 @@ SYSTEM DROP SCHEMA CACHE ['SYSTEM DROP SCHEMA CACHE','DROP SCHEMA CACHE'] GLOBAL SYSTEM DROP FORMAT SCHEMA CACHE ['SYSTEM DROP FORMAT SCHEMA CACHE','DROP FORMAT SCHEMA CACHE'] GLOBAL SYSTEM DROP CACHE SYSTEM DROP S3 CLIENT CACHE ['SYSTEM DROP S3 CLIENT','DROP S3 CLIENT CACHE'] GLOBAL SYSTEM DROP CACHE SYSTEM DROP PARQUET METADATA CACHE ['SYSTEM DROP PARQUET METADATA CACHE'] GLOBAL SYSTEM DROP CACHE +SYSTEM DROP OBJECT STORAGE LIST OBJECTS CACHE ['SYSTEM DROP OBJECT STORAGE LIST OBJECTS CACHE'] GLOBAL SYSTEM DROP CACHE SYSTEM DROP CACHE ['DROP CACHE'] \N SYSTEM SYSTEM RELOAD CONFIG ['RELOAD CONFIG'] GLOBAL SYSTEM RELOAD SYSTEM RELOAD USERS ['RELOAD USERS'] GLOBAL SYSTEM RELOAD From dd5934ef49248e2b90e068afcd239277f0db371e Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 5 May 2025 16:15:26 -0300 Subject: [PATCH 34/46] incorporate comments on tests --- .../Cache/ObjectStorageListObjectsCache.h | 1 + ...test_object_storage_list_objects_cache.cpp | 101 +++++++++--------- 2 files changed, 53 insertions(+), 49 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 0a68e2745f01..12080c6d8f57 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -10,6 +10,7 @@ namespace DB class ObjectStorageListObjectsCache { + friend class ObjectStorageListObjectsCacheTest; public: ObjectStorageListObjectsCache(const ObjectStorageListObjectsCache &) = delete; ObjectStorageListObjectsCache(ObjectStorageListObjectsCache &&) noexcept = delete; diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index 004f83443ac3..e61d7780d02d 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -6,70 +6,73 @@ namespace DB { -auto & initializeAndGetCacheInstance() +class ObjectStorageListObjectsCacheTest : public ::testing::Test { - static auto & cache = ObjectStorageListObjectsCache::instance(); - cache.setTTL(3); - cache.setMaxCount(100); - cache.setMaxSizeInBytes(1000000); - return cache; -} - -static auto & cache = initializeAndGetCacheInstance(); +protected: + void SetUp() override + { + cache = std::unique_ptr(new ObjectStorageListObjectsCache()); + cache->setTTL(3); + cache->setMaxCount(100); + cache->setMaxSizeInBytes(1000000); + } -ObjectStorageListObjectsCache::Key default_key {"default", "test-bucket", "test-prefix/"}; + std::unique_ptr cache; + static ObjectStorageListObjectsCache::Key default_key; -std::shared_ptr createTestValue(const std::vector& paths) -{ - auto value = std::make_shared(); - for (const auto & path : paths) + static std::shared_ptr createTestValue(const std::vector& paths) { - value->push_back(std::make_shared(path)); + auto value = std::make_shared(); + for (const auto & path : paths) + { + value->push_back(std::make_shared(path)); + } + return value; } - return value; -} +}; +ObjectStorageListObjectsCache::Key ObjectStorageListObjectsCacheTest::default_key {"default", "test-bucket", "test-prefix/"}; -TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) +TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(default_key, value); + cache->set(default_key, value); - auto result = cache.get(default_key).value(); + auto result = cache->get(default_key).value(); ASSERT_EQ(result.size(), 2); EXPECT_EQ(result[0]->getPath(), "test-prefix/file1.txt"); EXPECT_EQ(result[1]->getPath(), "test-prefix/file2.txt"); } -TEST(ObjectStorageListObjectsCacheTest, CacheMiss) +TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache->get(default_key)); } -TEST(ObjectStorageListObjectsCacheTest, ClearCache) +TEST_F(ObjectStorageListObjectsCacheTest, ClearCache) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(default_key, value); - cache.clear(); + cache->set(default_key, value); + cache->clear(); - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache->get(default_key)); } -TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) +TEST_F(ObjectStorageListObjectsCacheTest, PrefixMatching) { - cache.clear(); + cache->clear(); auto short_prefix_key = default_key; short_prefix_key.prefix = "parent/"; @@ -85,20 +88,20 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) "parent/child/grandchild/file1.txt", "parent/child/grandchild/file2.txt"}); - cache.set(mid_prefix_key, value); + cache->set(mid_prefix_key, value); - auto result1 = cache.get(mid_prefix_key).value(); + auto result1 = cache->get(mid_prefix_key).value(); EXPECT_EQ(result1.size(), 2); - auto result2 = cache.get(long_prefix_key).value(); + auto result2 = cache->get(long_prefix_key).value(); EXPECT_EQ(result2.size(), 2); - EXPECT_FALSE(cache.get(short_prefix_key)); + EXPECT_FALSE(cache->get(short_prefix_key)); } -TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) +TEST_F(ObjectStorageListObjectsCacheTest, PrefixFiltering) { - cache.clear(); + cache->clear(); auto key_with_short_prefix = default_key; key_with_short_prefix.prefix = "parent/"; @@ -112,34 +115,34 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) "parent/child2/file3.txt" }); - cache.set(key_with_short_prefix, value); + cache->set(key_with_short_prefix, value); - auto result = cache.get(key_with_mid_prefix, true).value(); + auto result = cache->get(key_with_mid_prefix, true).value(); EXPECT_EQ(result.size(), 1); EXPECT_EQ(result[0]->getPath(), "parent/child1/file2.txt"); } -TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) +TEST_F(ObjectStorageListObjectsCacheTest, TTLExpiration) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); - cache.set(default_key, value); + cache->set(default_key, value); // Verify we can get it immediately - auto result1 = cache.get(default_key).value(); + auto result1 = cache->get(default_key).value(); EXPECT_EQ(result1.size(), 1); std::this_thread::sleep_for(std::chrono::seconds(4)); - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache->get(default_key)); } -TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) +TEST_F(ObjectStorageListObjectsCacheTest, BestPrefixMatch) { - cache.clear(); + cache->clear(); auto short_prefix_key = default_key; short_prefix_key.prefix = "a/b/"; @@ -153,11 +156,11 @@ TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) auto short_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt", "a/b/file2.txt"}); auto mid_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt"}); - cache.set(short_prefix_key, short_prefix); - cache.set(mid_prefix_key, mid_prefix); + cache->set(short_prefix_key, short_prefix); + cache->set(mid_prefix_key, mid_prefix); // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size - auto result = cache.get(long_prefix_key, false).value(); + auto result = cache->get(long_prefix_key, false).value(); EXPECT_EQ(result.size(), 2u); } From 0f5057e76e77f51ccd35750654bd2ac8d220e078 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Mon, 5 May 2025 16:25:53 -0300 Subject: [PATCH 35/46] remove unused code --- .../tests/gtest_object_storage_list_objects_cache.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index e61d7780d02d..83509e596b3f 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -36,8 +36,6 @@ ObjectStorageListObjectsCache::Key ObjectStorageListObjectsCacheTest::default_ke TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); cache->set(default_key, value); @@ -51,8 +49,6 @@ TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; EXPECT_FALSE(cache->get(default_key)); } @@ -60,8 +56,6 @@ TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) TEST_F(ObjectStorageListObjectsCacheTest, ClearCache) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); cache->set(default_key, value); @@ -125,8 +119,6 @@ TEST_F(ObjectStorageListObjectsCacheTest, PrefixFiltering) TEST_F(ObjectStorageListObjectsCacheTest, TTLExpiration) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); cache->set(default_key, value); From 27c4deaca8d2e97749be04447b9a54d02bb7b44a Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 10:48:03 -0300 Subject: [PATCH 36/46] Revert "remove unused code" This reverts commit 0f5057e76e77f51ccd35750654bd2ac8d220e078. --- .../tests/gtest_object_storage_list_objects_cache.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index 83509e596b3f..e61d7780d02d 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -36,6 +36,8 @@ ObjectStorageListObjectsCache::Key ObjectStorageListObjectsCacheTest::default_ke TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) { cache->clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); cache->set(default_key, value); @@ -49,6 +51,8 @@ TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) { cache->clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; EXPECT_FALSE(cache->get(default_key)); } @@ -56,6 +60,8 @@ TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) TEST_F(ObjectStorageListObjectsCacheTest, ClearCache) { cache->clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); cache->set(default_key, value); @@ -119,6 +125,8 @@ TEST_F(ObjectStorageListObjectsCacheTest, PrefixFiltering) TEST_F(ObjectStorageListObjectsCacheTest, TTLExpiration) { cache->clear(); + const std::string bucket = "test-bucket"; + const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); cache->set(default_key, value); From d789d1e6339fd051d43f67734508619a2b140a8f Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 10:48:12 -0300 Subject: [PATCH 37/46] Revert "incorporate comments on tests" This reverts commit dd5934ef49248e2b90e068afcd239277f0db371e. --- .../Cache/ObjectStorageListObjectsCache.h | 1 - ...test_object_storage_list_objects_cache.cpp | 101 +++++++++--------- 2 files changed, 49 insertions(+), 53 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 12080c6d8f57..0a68e2745f01 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -10,7 +10,6 @@ namespace DB class ObjectStorageListObjectsCache { - friend class ObjectStorageListObjectsCacheTest; public: ObjectStorageListObjectsCache(const ObjectStorageListObjectsCache &) = delete; ObjectStorageListObjectsCache(ObjectStorageListObjectsCache &&) noexcept = delete; diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index e61d7780d02d..004f83443ac3 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -6,73 +6,70 @@ namespace DB { -class ObjectStorageListObjectsCacheTest : public ::testing::Test +auto & initializeAndGetCacheInstance() { -protected: - void SetUp() override - { - cache = std::unique_ptr(new ObjectStorageListObjectsCache()); - cache->setTTL(3); - cache->setMaxCount(100); - cache->setMaxSizeInBytes(1000000); - } + static auto & cache = ObjectStorageListObjectsCache::instance(); + cache.setTTL(3); + cache.setMaxCount(100); + cache.setMaxSizeInBytes(1000000); + return cache; +} - std::unique_ptr cache; - static ObjectStorageListObjectsCache::Key default_key; +static auto & cache = initializeAndGetCacheInstance(); - static std::shared_ptr createTestValue(const std::vector& paths) +ObjectStorageListObjectsCache::Key default_key {"default", "test-bucket", "test-prefix/"}; + +std::shared_ptr createTestValue(const std::vector& paths) +{ + auto value = std::make_shared(); + for (const auto & path : paths) { - auto value = std::make_shared(); - for (const auto & path : paths) - { - value->push_back(std::make_shared(path)); - } - return value; + value->push_back(std::make_shared(path)); } -}; + return value; +} -ObjectStorageListObjectsCache::Key ObjectStorageListObjectsCacheTest::default_key {"default", "test-bucket", "test-prefix/"}; -TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) +TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) { - cache->clear(); + cache.clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache->set(default_key, value); + cache.set(default_key, value); - auto result = cache->get(default_key).value(); + auto result = cache.get(default_key).value(); ASSERT_EQ(result.size(), 2); EXPECT_EQ(result[0]->getPath(), "test-prefix/file1.txt"); EXPECT_EQ(result[1]->getPath(), "test-prefix/file2.txt"); } -TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) +TEST(ObjectStorageListObjectsCacheTest, CacheMiss) { - cache->clear(); + cache.clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; - EXPECT_FALSE(cache->get(default_key)); + EXPECT_FALSE(cache.get(default_key)); } -TEST_F(ObjectStorageListObjectsCacheTest, ClearCache) +TEST(ObjectStorageListObjectsCacheTest, ClearCache) { - cache->clear(); + cache.clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache->set(default_key, value); - cache->clear(); + cache.set(default_key, value); + cache.clear(); - EXPECT_FALSE(cache->get(default_key)); + EXPECT_FALSE(cache.get(default_key)); } -TEST_F(ObjectStorageListObjectsCacheTest, PrefixMatching) +TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) { - cache->clear(); + cache.clear(); auto short_prefix_key = default_key; short_prefix_key.prefix = "parent/"; @@ -88,20 +85,20 @@ TEST_F(ObjectStorageListObjectsCacheTest, PrefixMatching) "parent/child/grandchild/file1.txt", "parent/child/grandchild/file2.txt"}); - cache->set(mid_prefix_key, value); + cache.set(mid_prefix_key, value); - auto result1 = cache->get(mid_prefix_key).value(); + auto result1 = cache.get(mid_prefix_key).value(); EXPECT_EQ(result1.size(), 2); - auto result2 = cache->get(long_prefix_key).value(); + auto result2 = cache.get(long_prefix_key).value(); EXPECT_EQ(result2.size(), 2); - EXPECT_FALSE(cache->get(short_prefix_key)); + EXPECT_FALSE(cache.get(short_prefix_key)); } -TEST_F(ObjectStorageListObjectsCacheTest, PrefixFiltering) +TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) { - cache->clear(); + cache.clear(); auto key_with_short_prefix = default_key; key_with_short_prefix.prefix = "parent/"; @@ -115,34 +112,34 @@ TEST_F(ObjectStorageListObjectsCacheTest, PrefixFiltering) "parent/child2/file3.txt" }); - cache->set(key_with_short_prefix, value); + cache.set(key_with_short_prefix, value); - auto result = cache->get(key_with_mid_prefix, true).value(); + auto result = cache.get(key_with_mid_prefix, true).value(); EXPECT_EQ(result.size(), 1); EXPECT_EQ(result[0]->getPath(), "parent/child1/file2.txt"); } -TEST_F(ObjectStorageListObjectsCacheTest, TTLExpiration) +TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) { - cache->clear(); + cache.clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); - cache->set(default_key, value); + cache.set(default_key, value); // Verify we can get it immediately - auto result1 = cache->get(default_key).value(); + auto result1 = cache.get(default_key).value(); EXPECT_EQ(result1.size(), 1); std::this_thread::sleep_for(std::chrono::seconds(4)); - EXPECT_FALSE(cache->get(default_key)); + EXPECT_FALSE(cache.get(default_key)); } -TEST_F(ObjectStorageListObjectsCacheTest, BestPrefixMatch) +TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) { - cache->clear(); + cache.clear(); auto short_prefix_key = default_key; short_prefix_key.prefix = "a/b/"; @@ -156,11 +153,11 @@ TEST_F(ObjectStorageListObjectsCacheTest, BestPrefixMatch) auto short_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt", "a/b/file2.txt"}); auto mid_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt"}); - cache->set(short_prefix_key, short_prefix); - cache->set(mid_prefix_key, mid_prefix); + cache.set(short_prefix_key, short_prefix); + cache.set(mid_prefix_key, mid_prefix); // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size - auto result = cache->get(long_prefix_key, false).value(); + auto result = cache.get(long_prefix_key, false).value(); EXPECT_EQ(result.size(), 2u); } From fef71c050d353522b76eec5825087aa21557b0df Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 10:48:36 -0300 Subject: [PATCH 38/46] Revert "docs" This reverts commit 2ed102d67c5dde5487f6043bbfeadd52337523c6. --- src/Core/ServerSettings.cpp | 2 +- src/Disks/ObjectStorages/IObjectStorage.h | 2 +- src/Storages/Cache/ObjectStorageListObjectsCache.h | 2 +- .../ObjectStorage/StorageObjectStorageSource.cpp | 2 +- src/Storages/ObjectStorage/StorageObjectStorageSource.h | 9 +++------ 5 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/Core/ServerSettings.cpp b/src/Core/ServerSettings.cpp index bb60b66082b1..b9dd72418e5f 100644 --- a/src/Core/ServerSettings.cpp +++ b/src/Core/ServerSettings.cpp @@ -1005,7 +1005,7 @@ namespace DB )", 0) \ DECLARE(Bool, storage_shared_set_join_use_inner_uuid, false, "If enabled, an inner UUID is generated during the creation of SharedSet and SharedJoin. ClickHouse Cloud only", 0) \ DECLARE(UInt64, input_format_parquet_metadata_cache_max_size, 500000000, "Maximum size of parquet file metadata cache", 0) \ - DECLARE(UInt64, object_storage_list_objects_cache_size, 500000000, "Maximum size of ObjectStorage list objects cache in bytes. Zero means disabled.", 0) \ + DECLARE(UInt64, object_storage_list_objects_cache_size, 5000000, "Maximum size of ObjectStorage list objects cache in bytes. Zero means disabled.", 0) \ DECLARE(UInt64, object_storage_list_objects_cache_max_entries, 1000, "Maximum size of ObjectStorage list objects cache in entries. Zero means disabled.", 0) \ DECLARE(UInt64, object_storage_list_objects_cache_ttl, 3600, "Time to live of records in ObjectStorage list objects cache in seconds. Zero means unlimited", 0) \ // clang-format on diff --git a/src/Disks/ObjectStorages/IObjectStorage.h b/src/Disks/ObjectStorages/IObjectStorage.h index 17ad96c1e7a6..8cd09e1fa852 100644 --- a/src/Disks/ObjectStorages/IObjectStorage.h +++ b/src/Disks/ObjectStorages/IObjectStorage.h @@ -126,7 +126,7 @@ class IObjectStorage virtual std::string getDescription() const = 0; - /// Storage name + authentication unique id. Used for securing ListObjects cache + // todo arthur add docs virtual std::optional getIdentityFingerprint() const = 0; virtual const MetadataStorageMetrics & getMetadataStorageMetrics() const; diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 0a68e2745f01..8fc741804bf0 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -28,7 +28,7 @@ class ObjectStorageListObjectsCache const std::chrono::steady_clock::time_point & expires_at_ = std::chrono::steady_clock::now(), std::optional user_id_ = std::nullopt); - // object storage name + account + // object storage name + account name std::string storage_identity; std::string bucket; std::string prefix; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index b0e9339e749d..c6d0d7dd7765 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -172,7 +172,7 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( } else { - cache_ptr = std::make_unique(cache, cache_key); + cache_ptr = std::make_unique(&cache, cache_key); object_iterator = object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size); } } diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 18e99ab1ae8a..11169cc8b253 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -164,20 +164,17 @@ class StorageObjectStorageSource::ReadTaskIterator : public IObjectIterator class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithContext { public: - /* - * Helper struct so `GlobIterator` does not need to re-construct the cache key all the time - */ struct ListObjectsCacheWithKey { - ListObjectsCacheWithKey(ObjectStorageListObjectsCache & cache_, const ObjectStorageListObjectsCache::Key & key_) : cache(cache_), key(key_) {} + ListObjectsCacheWithKey(ObjectStorageListObjectsCache * cache_, const ObjectStorageListObjectsCache::Key & key_) : cache(cache_), key(key_) {} void set(ObjectStorageListObjectsCache::Value && value) const { - cache.set(key, std::make_shared(std::move(value))); + cache->set(key, std::make_shared(std::move(value))); } private: - ObjectStorageListObjectsCache & cache; + ObjectStorageListObjectsCache * cache; ObjectStorageListObjectsCache::Key key; }; From 6bfcb86e06f58a4668cf8c36888d5b1a2e352bb1 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 10:48:49 -0300 Subject: [PATCH 39/46] Revert "azure impl and fix aws" This reverts commit 8e78b28943cb7a816aae5d64afe89c484ffc1f4b. --- .../registerBackupEngineAzureBlobStorage.cpp | 2 +- .../AzureBlobStorageCommon.cpp | 9 +-------- .../AzureBlobStorage/AzureBlobStorageCommon.h | 18 +----------------- .../AzureBlobStorage/AzureObjectStorage.cpp | 17 +++++++++-------- .../ObjectStorages/S3/S3ObjectStorage.cpp | 2 +- .../ObjectStorage/Azure/Configuration.cpp | 2 +- 6 files changed, 14 insertions(+), 36 deletions(-) diff --git a/src/Backups/registerBackupEngineAzureBlobStorage.cpp b/src/Backups/registerBackupEngineAzureBlobStorage.cpp index 3d0a11c417b9..a9e267db1d47 100644 --- a/src/Backups/registerBackupEngineAzureBlobStorage.cpp +++ b/src/Backups/registerBackupEngineAzureBlobStorage.cpp @@ -96,7 +96,7 @@ void registerBackupEngineAzureBlobStorage(BackupFactory & factory) auto account_name = args[3].safeGet(); auto account_key = args[4].safeGet(); - connection_params.auth_method = std::make_shared(account_name, account_key); + connection_params.auth_method = std::make_shared(account_name, account_key); connection_params.client_options = AzureBlobStorage::getClientOptions(*request_settings, /*for_disk=*/ true); } else diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp index ebbbbbe4dbac..869fa32819d3 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.cpp @@ -146,8 +146,6 @@ std::unique_ptr ConnectionParams::createForService() const { if constexpr (std::is_same_v) return std::make_unique(ServiceClient::CreateFromConnectionString(auth.toUnderType(), client_options)); - else if constexpr (std::is_same_v>) - return std::make_unique(endpoint.getServiceEndpoint(), auth->impl, client_options); else return std::make_unique(endpoint.getServiceEndpoint(), auth, client_options); }, auth_method); @@ -168,11 +166,6 @@ std::unique_ptr ConnectionParams::createForContainer() const auto raw_client = RawContainerClient::CreateFromConnectionString(auth.toUnderType(), endpoint.container_name, client_options); return std::make_unique(std::move(raw_client), endpoint.prefix); } - else if constexpr (std::is_same_v>) - { - RawContainerClient raw_client{endpoint.getContainerEndpoint(), auth->impl, client_options}; - return std::make_unique(std::move(raw_client), endpoint.prefix); - } else { RawContainerClient raw_client{endpoint.getContainerEndpoint(), auth, client_options}; @@ -376,7 +369,7 @@ AuthMethod getAuthMethod(const Poco::Util::AbstractConfiguration & config, const { if (config.has(config_prefix + ".account_key") && config.has(config_prefix + ".account_name")) { - return std::make_shared( + return std::make_shared( config.getString(config_prefix + ".account_name"), config.getString(config_prefix + ".account_key") ); diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h index 547f630a9110..d64b44809e6c 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureBlobStorageCommon.h @@ -126,25 +126,9 @@ using ServiceClient = Azure::Storage::Blobs::BlobServiceClient; using BlobClientOptions = Azure::Storage::Blobs::BlobClientOptions; using ConnectionString = StrongTypedef; -/* - * In order to implement `AzureObjectStorage::getIdentityFingerprint()` for `StorageSharedKeyCredential`, we need to - * access `account_key`. The problem is that `Azure::Storage::StorageSharedKeyCredential::AccessKey` is private and the - * class is final inside `Azure SDK`. - */ -struct StorageSharedKeyCredentialWithAccessToSecret -{ - StorageSharedKeyCredentialWithAccessToSecret(const String & account_name_, const String & account_key_) - : account_name(account_name_), account_key(account_key_), impl(std::make_shared(account_name, account_key)) - {} - - const std::string account_name; - const std::string account_key; - std::shared_ptr impl; -}; - using AuthMethod = std::variant< ConnectionString, - std::shared_ptr, + std::shared_ptr, std::shared_ptr, std::shared_ptr>; diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp index 8048cb33b1fd..d434e4f543fb 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp @@ -156,29 +156,30 @@ ObjectStorageIteratorPtr AzureObjectStorage::iterate(const std::string & path_pr return std::make_shared(path_prefix, client_ptr, max_keys ? max_keys : settings_ptr->list_object_keys_size); } -/* - * Only `ConnectionString` and `StorageSharedKeyCredential` auth methods are supported for now. - */ std::optional AzureObjectStorage::getIdentityFingerprint() const { std::optional fingerprint; - std::visit([&fingerprint](const auto & auth) - { + std::visit([&fingerprint](const auto & auth) { using T = std::decay_t; if constexpr (std::is_same_v) { auto connection_string_parts = Azure::Storage::_internal::ParseConnectionString(auth); - fingerprint = connection_string_parts.AccountName + connection_string_parts.AccountKey; + fingerprint = std::to_string(std::hash()(connection_string_parts.AccountName)); } - else if constexpr (std::is_same_v>) + else if constexpr (std::is_same_v>) { if (auth) { - fingerprint = auth->account_name + auth->account_key; + fingerprint = std::to_string(std::hash()(auth->AccountName)); } } + /// I am not sure what to do with the other auth methods, needs further investigation + // else if constexpr (std::is_same_v>) { + // } + // else if constexpr (std::is_same_v>) { + // } }, auth_method); if (!fingerprint) diff --git a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp index c7c953ca8869..da4b0d422147 100644 --- a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp +++ b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp @@ -168,7 +168,7 @@ std::optional S3ObjectStorage::getIdentityFingerprint() const { const auto credentials = client.get()->getCredentials(); - return getName() + credentials.GetAWSAccessKeyId() + credentials.GetAWSSecretKey(); + return getName() + credentials.GetAWSAccessKeyId(); } std::unique_ptr S3ObjectStorage::readObject( /// NOLINT diff --git a/src/Storages/ObjectStorage/Azure/Configuration.cpp b/src/Storages/ObjectStorage/Azure/Configuration.cpp index 86caa658f79f..187a959f63a0 100644 --- a/src/Storages/ObjectStorage/Azure/Configuration.cpp +++ b/src/Storages/ObjectStorage/Azure/Configuration.cpp @@ -117,7 +117,7 @@ static AzureBlobStorage::ConnectionParams getConnectionParams( { connection_params.endpoint.storage_account_url = connection_url; connection_params.endpoint.container_name = container_name; - connection_params.auth_method = std::make_shared(*account_name, *account_key); + connection_params.auth_method = std::make_shared(*account_name, *account_key); connection_params.client_options = AzureBlobStorage::getClientOptions(*request_settings, /*for_disk=*/ false); } else From f68725abf6afd01b02e0d31a77e555b6afbf9421 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 10:49:00 -0300 Subject: [PATCH 40/46] Revert "idraft impl of authorization aware cache" This reverts commit e0e19a20ea4a775ee87c1980ced9348577ab8b9d. --- src/Backups/BackupIO_AzureBlobStorage.cpp | 6 +- .../AzureBlobStorage/AzureObjectStorage.cpp | 40 +---------- .../AzureBlobStorage/AzureObjectStorage.h | 7 +- .../Cached/CachedObjectStorage.h | 2 - .../ObjectStorages/HDFS/HDFSObjectStorage.h | 2 - src/Disks/ObjectStorages/IObjectStorage.h | 3 - .../ObjectStorages/Local/LocalObjectStorage.h | 3 - .../ObjectStorages/ObjectStorageFactory.cpp | 3 +- .../ObjectStorages/S3/S3ObjectStorage.cpp | 7 -- src/Disks/ObjectStorages/S3/S3ObjectStorage.h | 2 - .../ObjectStorages/Web/WebObjectStorage.h | 3 - .../Cache/ObjectStorageListObjectsCache.cpp | 15 +++-- .../Cache/ObjectStorageListObjectsCache.h | 10 +-- ...test_object_storage_list_objects_cache.cpp | 67 +++++++------------ .../ObjectStorage/Azure/Configuration.cpp | 3 +- .../StorageObjectStorageSource.cpp | 17 +++-- .../StorageObjectStorageSource.h | 18 +---- 17 files changed, 54 insertions(+), 154 deletions(-) diff --git a/src/Backups/BackupIO_AzureBlobStorage.cpp b/src/Backups/BackupIO_AzureBlobStorage.cpp index e603dd540758..449ec587d866 100644 --- a/src/Backups/BackupIO_AzureBlobStorage.cpp +++ b/src/Backups/BackupIO_AzureBlobStorage.cpp @@ -47,8 +47,7 @@ BackupReaderAzureBlobStorage::BackupReaderAzureBlobStorage( std::move(client_ptr), std::move(settings_ptr), connection_params.getContainer(), - connection_params.getConnectionURL(), - connection_params.auth_method); + connection_params.getConnectionURL()); client = object_storage->getAzureBlobStorageClient(); settings = object_storage->getSettings(); @@ -143,8 +142,7 @@ BackupWriterAzureBlobStorage::BackupWriterAzureBlobStorage( std::move(client_ptr), std::move(settings_ptr), connection_params.getContainer(), - connection_params.getConnectionURL(), - connection_params.auth_method); + connection_params.getConnectionURL()); client = object_storage->getAzureBlobStorageClient(); settings = object_storage->getSettings(); diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp index d434e4f543fb..93ceddde7498 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.cpp @@ -108,14 +108,12 @@ AzureObjectStorage::AzureObjectStorage( ClientPtr && client_, SettingsPtr && settings_, const String & object_namespace_, - const String & description_, - AzureBlobStorage::AuthMethod auth_method_) + const String & description_) : name(name_) , client(std::move(client_)) , settings(std::move(settings_)) , object_namespace(object_namespace_) , description(description_) - , auth_method(auth_method_) , log(getLogger("AzureObjectStorage")) { } @@ -156,40 +154,6 @@ ObjectStorageIteratorPtr AzureObjectStorage::iterate(const std::string & path_pr return std::make_shared(path_prefix, client_ptr, max_keys ? max_keys : settings_ptr->list_object_keys_size); } -std::optional AzureObjectStorage::getIdentityFingerprint() const -{ - std::optional fingerprint; - - std::visit([&fingerprint](const auto & auth) { - using T = std::decay_t; - - if constexpr (std::is_same_v) - { - auto connection_string_parts = Azure::Storage::_internal::ParseConnectionString(auth); - fingerprint = std::to_string(std::hash()(connection_string_parts.AccountName)); - } - else if constexpr (std::is_same_v>) - { - if (auth) - { - fingerprint = std::to_string(std::hash()(auth->AccountName)); - } - } - /// I am not sure what to do with the other auth methods, needs further investigation - // else if constexpr (std::is_same_v>) { - // } - // else if constexpr (std::is_same_v>) { - // } - }, auth_method); - - if (!fingerprint) - { - return std::nullopt; - } - - return getName() + fingerprint.value(); -} - void AzureObjectStorage::listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t max_keys) const { auto client_ptr = client.get(); @@ -416,7 +380,7 @@ std::unique_ptr AzureObjectStorage::cloneObjectStorage( }; auto new_client = AzureBlobStorage::getContainerClient(params, /*readonly=*/ true); - return std::make_unique(name, std::move(new_client), std::move(new_settings), new_namespace, params.endpoint.getServiceEndpoint(), params.auth_method); + return std::make_unique(name, std::move(new_client), std::move(new_settings), new_namespace, params.endpoint.getServiceEndpoint()); } } diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h index 33d6f0906115..5f91a18e4d97 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h @@ -29,8 +29,7 @@ class AzureObjectStorage : public IObjectStorage ClientPtr && client_, SettingsPtr && settings_, const String & object_namespace_, - const String & description_, - AzureBlobStorage::AuthMethod auth_method_); + const String & description_); void listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t max_keys) const override; @@ -38,8 +37,6 @@ class AzureObjectStorage : public IObjectStorage std::string getName() const override { return "AzureObjectStorage"; } - std::optional getIdentityFingerprint() const override; - ObjectStorageType getType() const override { return ObjectStorageType::Azure; } std::string getCommonKeyPrefix() const override { return ""; } @@ -119,8 +116,6 @@ class AzureObjectStorage : public IObjectStorage /// We use source url without container and prefix as description, because in Azure there are no limitations for operations between different containers. const String description; - AzureBlobStorage::AuthMethod auth_method; - LoggerPtr log; }; diff --git a/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h b/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h index 849dca3cff01..00f236c1e89d 100644 --- a/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h +++ b/src/Disks/ObjectStorages/Cached/CachedObjectStorage.h @@ -31,8 +31,6 @@ class CachedObjectStorage final : public IObjectStorage bool exists(const StoredObject & object) const override; - std::optional getIdentityFingerprint() const override { return object_storage->getIdentityFingerprint(); } - std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h b/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h index 40d6dcecff6d..28b75d44f21e 100644 --- a/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h +++ b/src/Disks/ObjectStorages/HDFS/HDFSObjectStorage.h @@ -65,8 +65,6 @@ class HDFSObjectStorage : public IObjectStorage, public HDFSErrorWrapper bool exists(const StoredObject & object) const override; - std::optional getIdentityFingerprint() const override { return std::nullopt; } - std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/IObjectStorage.h b/src/Disks/ObjectStorages/IObjectStorage.h index 8cd09e1fa852..f99864ebb25c 100644 --- a/src/Disks/ObjectStorages/IObjectStorage.h +++ b/src/Disks/ObjectStorages/IObjectStorage.h @@ -126,9 +126,6 @@ class IObjectStorage virtual std::string getDescription() const = 0; - // todo arthur add docs - virtual std::optional getIdentityFingerprint() const = 0; - virtual const MetadataStorageMetrics & getMetadataStorageMetrics() const; /// Object exists or not diff --git a/src/Disks/ObjectStorages/Local/LocalObjectStorage.h b/src/Disks/ObjectStorages/Local/LocalObjectStorage.h index 82634441c37e..50037aa3ce91 100644 --- a/src/Disks/ObjectStorages/Local/LocalObjectStorage.h +++ b/src/Disks/ObjectStorages/Local/LocalObjectStorage.h @@ -28,9 +28,6 @@ class LocalObjectStorage : public IObjectStorage bool exists(const StoredObject & object) const override; - // no auth - std::optional getIdentityFingerprint() const override { return getName(); } - std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/ObjectStorageFactory.cpp b/src/Disks/ObjectStorages/ObjectStorageFactory.cpp index 4311fa820656..69b4627dd74c 100644 --- a/src/Disks/ObjectStorages/ObjectStorageFactory.cpp +++ b/src/Disks/ObjectStorages/ObjectStorageFactory.cpp @@ -307,8 +307,7 @@ void registerAzureObjectStorage(ObjectStorageFactory & factory) ObjectStorageType::Azure, config, config_prefix, name, AzureBlobStorage::getContainerClient(params, /*readonly=*/ false), std::move(azure_settings), params.endpoint.prefix.empty() ? params.endpoint.container_name : params.endpoint.container_name + "/" + params.endpoint.prefix, - params.endpoint.getServiceEndpoint(), - params.auth_method); + params.endpoint.getServiceEndpoint()); }; factory.registerObjectStorageType("azure_blob_storage", creator); diff --git a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp index da4b0d422147..0773833712d9 100644 --- a/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp +++ b/src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp @@ -164,13 +164,6 @@ bool S3ObjectStorage::exists(const StoredObject & object) const return S3::objectExists(*client.get(), uri.bucket, object.remote_path, {}); } -std::optional S3ObjectStorage::getIdentityFingerprint() const -{ - const auto credentials = client.get()->getCredentials(); - - return getName() + credentials.GetAWSAccessKeyId(); -} - std::unique_ptr S3ObjectStorage::readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Disks/ObjectStorages/S3/S3ObjectStorage.h b/src/Disks/ObjectStorages/S3/S3ObjectStorage.h index 1be039b5d5b3..f8811785ed39 100644 --- a/src/Disks/ObjectStorages/S3/S3ObjectStorage.h +++ b/src/Disks/ObjectStorages/S3/S3ObjectStorage.h @@ -81,8 +81,6 @@ class S3ObjectStorage : public IObjectStorage ObjectStorageType getType() const override { return ObjectStorageType::S3; } - std::optional getIdentityFingerprint() const override; - bool exists(const StoredObject & object) const override; std::unique_ptr readObject( /// NOLINT diff --git a/src/Disks/ObjectStorages/Web/WebObjectStorage.h b/src/Disks/ObjectStorages/Web/WebObjectStorage.h index 4c5bca68edac..c96b0bd1e724 100644 --- a/src/Disks/ObjectStorages/Web/WebObjectStorage.h +++ b/src/Disks/ObjectStorages/Web/WebObjectStorage.h @@ -33,9 +33,6 @@ class WebObjectStorage : public IObjectStorage, WithContext bool exists(const StoredObject & object) const override; - // apparently no auth is required for this object storage type - std::optional getIdentityFingerprint() const override { return getName(); } - std::unique_ptr readObject( /// NOLINT const StoredObject & object, const ReadSettings & read_settings, diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 08c4ce7a1780..cc70a728b005 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -73,23 +73,21 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy user_id_) - : storage_identity(storage_identity_), bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} + : bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} bool ObjectStorageListObjectsCache::Key::operator==(const Key & other) const { - return storage_identity == other.storage_identity && bucket == other.bucket && prefix == other.prefix; + return bucket == other.bucket && prefix == other.prefix; } size_t ObjectStorageListObjectsCache::KeyHasher::operator()(const Key & key) const { std::size_t seed = 0; - boost::hash_combine(seed, key.storage_identity); boost::hash_combine(seed, key.bucket); boost::hash_combine(seed, key.prefix); @@ -119,10 +117,12 @@ ObjectStorageListObjectsCache::ObjectStorageListObjectsCache() } void ObjectStorageListObjectsCache::set( - Key key, + const std::string & bucket, + const std::string & prefix, const std::shared_ptr & value) { - key.expires_at = std::chrono::steady_clock::now() + std::chrono::seconds(ttl_in_seconds); + const auto key = Key{bucket, prefix, std::chrono::steady_clock::now() + std::chrono::seconds(ttl_in_seconds)}; + cache.set(key, value); } @@ -131,8 +131,9 @@ void ObjectStorageListObjectsCache::clear() cache.clear(); } -std::optional ObjectStorageListObjectsCache::get(const Key & input_key, bool filter_by_prefix) +std::optional ObjectStorageListObjectsCache::get(const String & bucket, const String & prefix, bool filter_by_prefix) { + const auto input_key = Key{bucket, prefix}; auto pair = cache.getWithKey(input_key); if (!pair) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 8fc741804bf0..6710176e19a2 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -22,14 +22,11 @@ class ObjectStorageListObjectsCache struct Key { Key( - const String & storage_identity_, const String & bucket_, const String & prefix_, const std::chrono::steady_clock::time_point & expires_at_ = std::chrono::steady_clock::now(), std::optional user_id_ = std::nullopt); - // object storage name + account name - std::string storage_identity; std::string bucket; std::string prefix; std::chrono::steady_clock::time_point expires_at; @@ -56,9 +53,12 @@ class ObjectStorageListObjectsCache using Cache = CacheBase; - void set(Key key,const std::shared_ptr & value); + void set( + const std::string & bucket, + const std::string & prefix, + const std::shared_ptr & value); - std::optional get(const Key & input_key, bool filter_by_prefix = true); + std::optional get(const String & bucket, const String & prefix, bool filter_by_prefix = true); void clear(); diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index 004f83443ac3..e62540a56415 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -17,8 +17,6 @@ auto & initializeAndGetCacheInstance() static auto & cache = initializeAndGetCacheInstance(); -ObjectStorageListObjectsCache::Key default_key {"default", "test-bucket", "test-prefix/"}; - std::shared_ptr createTestValue(const std::vector& paths) { auto value = std::make_shared(); @@ -37,9 +35,9 @@ TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(default_key, value); + cache.set(bucket, prefix, value); - auto result = cache.get(default_key).value(); + auto result = cache.get(bucket, prefix).value(); ASSERT_EQ(result.size(), 2); EXPECT_EQ(result[0]->getPath(), "test-prefix/file1.txt"); EXPECT_EQ(result[1]->getPath(), "test-prefix/file2.txt"); @@ -51,7 +49,7 @@ TEST(ObjectStorageListObjectsCacheTest, CacheMiss) const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache.get(bucket, prefix)); } TEST(ObjectStorageListObjectsCacheTest, ClearCache) @@ -61,50 +59,41 @@ TEST(ObjectStorageListObjectsCacheTest, ClearCache) const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(default_key, value); + cache.set(bucket, prefix, value); cache.clear(); - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache.get(bucket, prefix)); } TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) { cache.clear(); - - auto short_prefix_key = default_key; - short_prefix_key.prefix = "parent/"; - - auto mid_prefix_key = default_key; - mid_prefix_key.prefix = "parent/child/"; - - auto long_prefix_key = default_key; - long_prefix_key.prefix = "parent/child/grandchild/"; + const std::string bucket = "test-bucket"; + const std::string short_prefix = "parent/"; + const std::string mid_prefix = "parent/child/"; + const std::string long_prefix = "parent/child/grandchild/"; auto value = createTestValue( { "parent/child/grandchild/file1.txt", "parent/child/grandchild/file2.txt"}); - cache.set(mid_prefix_key, value); + cache.set(bucket, mid_prefix, value); - auto result1 = cache.get(mid_prefix_key).value(); + auto result1 = cache.get(bucket, mid_prefix).value(); EXPECT_EQ(result1.size(), 2); - auto result2 = cache.get(long_prefix_key).value(); + auto result2 = cache.get(bucket, long_prefix).value(); EXPECT_EQ(result2.size(), 2); - EXPECT_FALSE(cache.get(short_prefix_key)); + EXPECT_FALSE(cache.get(bucket, short_prefix)); } TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) { cache.clear(); - - auto key_with_short_prefix = default_key; - key_with_short_prefix.prefix = "parent/"; - - auto key_with_mid_prefix = default_key; - key_with_mid_prefix.prefix = "parent/child1/"; + const std::string bucket = "test-bucket"; + const std::string short_prefix = "parent/"; auto value = createTestValue({ "parent/file1.txt", @@ -112,9 +101,9 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) "parent/child2/file3.txt" }); - cache.set(key_with_short_prefix, value); + cache.set(bucket, short_prefix, value); - auto result = cache.get(key_with_mid_prefix, true).value(); + auto result = cache.get(bucket, "parent/child1/", true).value(); EXPECT_EQ(result.size(), 1); EXPECT_EQ(result[0]->getPath(), "parent/child1/file2.txt"); } @@ -126,38 +115,30 @@ TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); - cache.set(default_key, value); + cache.set(bucket, prefix, value); // Verify we can get it immediately - auto result1 = cache.get(default_key).value(); + auto result1 = cache.get(bucket, prefix).value(); EXPECT_EQ(result1.size(), 1); std::this_thread::sleep_for(std::chrono::seconds(4)); - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache.get(bucket, prefix)); } TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) { cache.clear(); - - auto short_prefix_key = default_key; - short_prefix_key.prefix = "a/b/"; - - auto mid_prefix_key = default_key; - mid_prefix_key.prefix = "a/b/c/"; - - auto long_prefix_key = default_key; - long_prefix_key.prefix = "a/b/c/d/"; + const std::string bucket = "test-bucket"; auto short_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt", "a/b/file2.txt"}); auto mid_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt"}); - cache.set(short_prefix_key, short_prefix); - cache.set(mid_prefix_key, mid_prefix); + cache.set(bucket, "a/b/", short_prefix); + cache.set(bucket, "a/b/c/", mid_prefix); // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size - auto result = cache.get(long_prefix_key, false).value(); + auto result = cache.get(bucket, "a/b/c/d/", false).value(); EXPECT_EQ(result.size(), 2u); } diff --git a/src/Storages/ObjectStorage/Azure/Configuration.cpp b/src/Storages/ObjectStorage/Azure/Configuration.cpp index 187a959f63a0..3184c1308ef1 100644 --- a/src/Storages/ObjectStorage/Azure/Configuration.cpp +++ b/src/Storages/ObjectStorage/Azure/Configuration.cpp @@ -99,8 +99,7 @@ ObjectStoragePtr StorageAzureConfiguration::createObjectStorage(ContextPtr conte connection_params.createForContainer(), std::move(settings), connection_params.getContainer(), - connection_params.getConnectionURL(), - connection_params.auth_method); + connection_params.getConnectionURL()); } static AzureBlobStorage::ConnectionParams getConnectionParams( diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index c6d0d7dd7765..4399a52daaa5 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -159,20 +159,19 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( else { std::shared_ptr object_iterator = nullptr; - std::unique_ptr cache_ptr; + ObjectStorageListObjectsCache * cache_ptr = nullptr; - if (const auto identity_fingerprint = object_storage->getIdentityFingerprint(); local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache] && identity_fingerprint) + if (local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache]) { auto & cache = ObjectStorageListObjectsCache::instance(); - auto cache_key = ObjectStorageListObjectsCache::Key {*identity_fingerprint, configuration->getNamespace(), configuration->getPathWithoutGlobs()}; - if (auto objects_info = cache.get(cache_key, /*filter_by_prefix*/ false)) + if (auto objects_info = cache.get(configuration->getNamespace(), configuration->getPathWithoutGlobs(), /*filter_by_prefix=*/ false)) { object_iterator = std::make_shared(std::move(*objects_info)); } else { - cache_ptr = std::make_unique(&cache, cache_key); + cache_ptr = &cache; object_iterator = object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size); } } @@ -185,7 +184,7 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( iterator = std::make_unique( object_iterator, configuration, predicate, virtual_columns, local_context, is_archive ? nullptr : read_keys, - query_settings.throw_on_zero_files_match, file_progress_callback, std::move(cache_ptr)); + query_settings.throw_on_zero_files_match, file_progress_callback, cache_ptr); } } else if (configuration->supportsFileIterator()) @@ -687,7 +686,7 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( ObjectInfos * read_keys_, bool throw_on_zero_files_match_, std::function file_progress_callback_, - std::unique_ptr list_cache_) + ObjectStorageListObjectsCache * list_cache_) : WithContext(context_) , object_storage_iterator(object_storage_iterator_) , configuration(configuration_) @@ -697,7 +696,7 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( , read_keys(read_keys_) , local_context(context_) , file_progress_callback(file_progress_callback_) - , list_cache(std::move(list_cache_)) + , list_cache(list_cache_) { if (configuration->isNamespaceWithGlobs()) { @@ -772,7 +771,7 @@ StorageObjectStorage::ObjectInfoPtr StorageObjectStorageSource::GlobIterator::ne { if (list_cache) { - list_cache->set(std::move(object_list)); + list_cache->set(configuration->getNamespace(), configuration->getPathWithoutGlobs(), std::make_shared(std::move(object_list))); } is_finished = true; return {}; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 11169cc8b253..0974b2abd80f 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -164,20 +164,6 @@ class StorageObjectStorageSource::ReadTaskIterator : public IObjectIterator class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithContext { public: - struct ListObjectsCacheWithKey - { - ListObjectsCacheWithKey(ObjectStorageListObjectsCache * cache_, const ObjectStorageListObjectsCache::Key & key_) : cache(cache_), key(key_) {} - - void set(ObjectStorageListObjectsCache::Value && value) const - { - cache->set(key, std::make_shared(std::move(value))); - } - - private: - ObjectStorageListObjectsCache * cache; - ObjectStorageListObjectsCache::Key key; - }; - GlobIterator( const ObjectStorageIteratorPtr & object_storage_iterator_, ConfigurationPtr configuration_, @@ -187,7 +173,7 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon ObjectInfos * read_keys_, bool throw_on_zero_files_match_, std::function file_progress_callback_ = {}, - std::unique_ptr list_cache_ = nullptr); + ObjectStorageListObjectsCache * list_cache_ = nullptr); ~GlobIterator() override = default; @@ -223,7 +209,7 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon const ContextPtr local_context; std::function file_progress_callback; - std::unique_ptr list_cache; + ObjectStorageListObjectsCache * list_cache; ObjectInfos object_list; }; From f863a6e096412b7146d14b4c2b2013c731b76fd8 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 15:43:31 -0300 Subject: [PATCH 41/46] use description as part of the key --- .../Cache/ObjectStorageListObjectsCache.cpp | 23 +++--- .../Cache/ObjectStorageListObjectsCache.h | 7 +- ...test_object_storage_list_objects_cache.cpp | 79 ++++++++++++------- .../StorageObjectStorageSource.cpp | 15 ++-- .../StorageObjectStorageSource.h | 18 ++++- 5 files changed, 89 insertions(+), 53 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index cc70a728b005..06614397a734 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -73,21 +73,23 @@ class ObjectStorageListObjectsCachePolicy : public TTLCachePolicy user_id_) - : bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} + : storage_description(storage_description_), bucket(bucket_), prefix(prefix_), expires_at(expires_at_), user_id(user_id_) {} bool ObjectStorageListObjectsCache::Key::operator==(const Key & other) const { - return bucket == other.bucket && prefix == other.prefix; + return storage_description == other.storage_description && bucket == other.bucket && prefix == other.prefix; } size_t ObjectStorageListObjectsCache::KeyHasher::operator()(const Key & key) const { std::size_t seed = 0; + boost::hash_combine(seed, key.storage_description); boost::hash_combine(seed, key.bucket); boost::hash_combine(seed, key.prefix); @@ -117,13 +119,13 @@ ObjectStorageListObjectsCache::ObjectStorageListObjectsCache() } void ObjectStorageListObjectsCache::set( - const std::string & bucket, - const std::string & prefix, + const Key & key, const std::shared_ptr & value) { - const auto key = Key{bucket, prefix, std::chrono::steady_clock::now() + std::chrono::seconds(ttl_in_seconds)}; + auto key_with_ttl = key; + key_with_ttl.expires_at = std::chrono::steady_clock::now() + std::chrono::seconds(ttl_in_seconds); - cache.set(key, value); + cache.set(key_with_ttl, value); } void ObjectStorageListObjectsCache::clear() @@ -131,10 +133,9 @@ void ObjectStorageListObjectsCache::clear() cache.clear(); } -std::optional ObjectStorageListObjectsCache::get(const String & bucket, const String & prefix, bool filter_by_prefix) +std::optional ObjectStorageListObjectsCache::get(const Key & key, bool filter_by_prefix) { - const auto input_key = Key{bucket, prefix}; - auto pair = cache.getWithKey(input_key); + const auto pair = cache.getWithKey(key); if (!pair) { @@ -144,7 +145,7 @@ std::optional ObjectStorageListObjectsCach ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCacheHits); - if (pair->key == input_key) + if (pair->key == key) { ProfileEvents::increment(ProfileEvents::ObjectStorageListObjectsCacheExactMatchHits); return *pair->mapped; @@ -163,7 +164,7 @@ std::optional ObjectStorageListObjectsCach for (const auto & object : *pair->mapped) { - if (object->relative_path.starts_with(input_key.prefix)) + if (object->relative_path.starts_with(key.prefix)) { filtered_objects.push_back(object); } diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index 6710176e19a2..b26b490be646 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -22,11 +22,13 @@ class ObjectStorageListObjectsCache struct Key { Key( + const String & storage_description_, const String & bucket_, const String & prefix_, const std::chrono::steady_clock::time_point & expires_at_ = std::chrono::steady_clock::now(), std::optional user_id_ = std::nullopt); + std::string storage_description; std::string bucket; std::string prefix; std::chrono::steady_clock::time_point expires_at; @@ -54,11 +56,10 @@ class ObjectStorageListObjectsCache using Cache = CacheBase; void set( - const std::string & bucket, - const std::string & prefix, + const Key & key, const std::shared_ptr & value); - std::optional get(const String & bucket, const String & prefix, bool filter_by_prefix = true); + std::optional get(const Key & key, bool filter_by_prefix = true); void clear(); diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index e62540a56415..b6d2fafa1717 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -17,6 +17,8 @@ auto & initializeAndGetCacheInstance() static auto & cache = initializeAndGetCacheInstance(); +ObjectStorageListObjectsCache::Key default_key {"default", "test-bucket", "test-prefix/"}; + std::shared_ptr createTestValue(const std::vector& paths) { auto value = std::make_shared(); @@ -34,10 +36,10 @@ TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - - cache.set(bucket, prefix, value); - - auto result = cache.get(bucket, prefix).value(); + + cache.set(default_key, value); + + auto result = cache.get(default_key).value(); ASSERT_EQ(result.size(), 2); EXPECT_EQ(result[0]->getPath(), "test-prefix/file1.txt"); EXPECT_EQ(result[1]->getPath(), "test-prefix/file2.txt"); @@ -49,7 +51,7 @@ TEST(ObjectStorageListObjectsCacheTest, CacheMiss) const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; - EXPECT_FALSE(cache.get(bucket, prefix)); + EXPECT_FALSE(cache.get(default_key)); } TEST(ObjectStorageListObjectsCacheTest, ClearCache) @@ -58,52 +60,61 @@ TEST(ObjectStorageListObjectsCacheTest, ClearCache) const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - - cache.set(bucket, prefix, value); + + cache.set(default_key, value); cache.clear(); - EXPECT_FALSE(cache.get(bucket, prefix)); + EXPECT_FALSE(cache.get(default_key)); } TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) { cache.clear(); - const std::string bucket = "test-bucket"; - const std::string short_prefix = "parent/"; - const std::string mid_prefix = "parent/child/"; - const std::string long_prefix = "parent/child/grandchild/"; - + + auto short_prefix_key = default_key; + short_prefix_key.prefix = "parent/"; + + auto mid_prefix_key = default_key; + mid_prefix_key.prefix = "parent/child/"; + + auto long_prefix_key = default_key; + long_prefix_key.prefix = "parent/child/grandchild/"; + auto value = createTestValue( { "parent/child/grandchild/file1.txt", "parent/child/grandchild/file2.txt"}); - cache.set(bucket, mid_prefix, value); + cache.set(mid_prefix_key, value); - auto result1 = cache.get(bucket, mid_prefix).value(); + auto result1 = cache.get(mid_prefix_key).value(); EXPECT_EQ(result1.size(), 2); - auto result2 = cache.get(bucket, long_prefix).value(); + auto result2 = cache.get(long_prefix_key).value(); EXPECT_EQ(result2.size(), 2); - EXPECT_FALSE(cache.get(bucket, short_prefix)); + EXPECT_FALSE(cache.get(short_prefix_key)); } TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) { cache.clear(); - const std::string bucket = "test-bucket"; - const std::string short_prefix = "parent/"; + + auto key_with_short_prefix = default_key; + key_with_short_prefix.prefix = "parent/"; + + auto key_with_mid_prefix = default_key; + key_with_mid_prefix.prefix = "parent/child1/"; auto value = createTestValue({ "parent/file1.txt", "parent/child1/file2.txt", "parent/child2/file3.txt" }); - - cache.set(bucket, short_prefix, value); - auto result = cache.get(bucket, "parent/child1/", true).value(); + cache.set(key_with_short_prefix, value); + + auto result = cache.get(key_with_mid_prefix, true).value(); EXPECT_EQ(result.size(), 1); EXPECT_EQ(result[0]->getPath(), "parent/child1/file2.txt"); } @@ -115,30 +126,38 @@ TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); - cache.set(bucket, prefix, value); - + cache.set(default_key, value); + // Verify we can get it immediately - auto result1 = cache.get(bucket, prefix).value(); + auto result1 = cache.get(default_key).value(); EXPECT_EQ(result1.size(), 1); std::this_thread::sleep_for(std::chrono::seconds(4)); - EXPECT_FALSE(cache.get(bucket, prefix)); + EXPECT_FALSE(cache.get(default_key)); } TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) { cache.clear(); - const std::string bucket = "test-bucket"; + + auto short_prefix_key = default_key; + short_prefix_key.prefix = "a/b/"; + + auto mid_prefix_key = default_key; + mid_prefix_key.prefix = "a/b/c/"; + + auto long_prefix_key = default_key; + long_prefix_key.prefix = "a/b/c/d/"; auto short_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt", "a/b/file2.txt"}); auto mid_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt"}); - cache.set(bucket, "a/b/", short_prefix); - cache.set(bucket, "a/b/c/", mid_prefix); + cache.set(short_prefix_key, short_prefix); + cache.set(mid_prefix_key, mid_prefix); // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size - auto result = cache.get(bucket, "a/b/c/d/", false).value(); + auto result = cache.get(long_prefix_key, false).value(); EXPECT_EQ(result.size(), 2u); } diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 4399a52daaa5..95f69a233b64 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -159,19 +159,20 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( else { std::shared_ptr object_iterator = nullptr; - ObjectStorageListObjectsCache * cache_ptr = nullptr; + std::unique_ptr cache_ptr = nullptr; if (local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache]) { auto & cache = ObjectStorageListObjectsCache::instance(); + ObjectStorageListObjectsCache::Key cache_key {object_storage->getDescription(), configuration->getNamespace(), configuration->getPathWithoutGlobs()}; - if (auto objects_info = cache.get(configuration->getNamespace(), configuration->getPathWithoutGlobs(), /*filter_by_prefix=*/ false)) + if (auto objects_info = cache.get(cache_key, /*filter_by_prefix=*/ false)) { object_iterator = std::make_shared(std::move(*objects_info)); } else { - cache_ptr = &cache; + cache_ptr = std::make_unique(cache, cache_key); object_iterator = object_storage->iterate(configuration->getPathWithoutGlobs(), query_settings.list_object_keys_size); } } @@ -184,7 +185,7 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( iterator = std::make_unique( object_iterator, configuration, predicate, virtual_columns, local_context, is_archive ? nullptr : read_keys, - query_settings.throw_on_zero_files_match, file_progress_callback, cache_ptr); + query_settings.throw_on_zero_files_match, file_progress_callback, std::move(cache_ptr)); } } else if (configuration->supportsFileIterator()) @@ -686,7 +687,7 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( ObjectInfos * read_keys_, bool throw_on_zero_files_match_, std::function file_progress_callback_, - ObjectStorageListObjectsCache * list_cache_) + std::unique_ptr list_cache_) : WithContext(context_) , object_storage_iterator(object_storage_iterator_) , configuration(configuration_) @@ -696,7 +697,7 @@ StorageObjectStorageSource::GlobIterator::GlobIterator( , read_keys(read_keys_) , local_context(context_) , file_progress_callback(file_progress_callback_) - , list_cache(list_cache_) + , list_cache(std::move(list_cache_)) { if (configuration->isNamespaceWithGlobs()) { @@ -771,7 +772,7 @@ StorageObjectStorage::ObjectInfoPtr StorageObjectStorageSource::GlobIterator::ne { if (list_cache) { - list_cache->set(configuration->getNamespace(), configuration->getPathWithoutGlobs(), std::make_shared(std::move(object_list))); + list_cache->set(std::move(object_list)); } is_finished = true; return {}; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.h b/src/Storages/ObjectStorage/StorageObjectStorageSource.h index 0974b2abd80f..69de1ac86440 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.h @@ -164,6 +164,20 @@ class StorageObjectStorageSource::ReadTaskIterator : public IObjectIterator class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithContext { public: + struct ListObjectsCacheWithKey + { + ListObjectsCacheWithKey(ObjectStorageListObjectsCache & cache_, const ObjectStorageListObjectsCache::Key & key_) : cache(cache_), key(key_) {} + + void set(ObjectStorageListObjectsCache::Value && value) const + { + cache.set(key, std::make_shared(std::move(value))); + } + + private: + ObjectStorageListObjectsCache & cache; + ObjectStorageListObjectsCache::Key key; + }; + GlobIterator( const ObjectStorageIteratorPtr & object_storage_iterator_, ConfigurationPtr configuration_, @@ -173,7 +187,7 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon ObjectInfos * read_keys_, bool throw_on_zero_files_match_, std::function file_progress_callback_ = {}, - ObjectStorageListObjectsCache * list_cache_ = nullptr); + std::unique_ptr list_cache_ = nullptr); ~GlobIterator() override = default; @@ -209,7 +223,7 @@ class StorageObjectStorageSource::GlobIterator : public IObjectIterator, WithCon const ContextPtr local_context; std::function file_progress_callback; - ObjectStorageListObjectsCache * list_cache; + std::unique_ptr list_cache; ObjectInfos object_list; }; From cbfe36d558e914db9f4d3436bf8079c8304a0881 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 15:44:42 -0300 Subject: [PATCH 42/46] Reapply "incorporate comments on tests" This reverts commit d789d1e6339fd051d43f67734508619a2b140a8f. --- .../Cache/ObjectStorageListObjectsCache.h | 1 + ...test_object_storage_list_objects_cache.cpp | 102 +++++++++--------- 2 files changed, 54 insertions(+), 49 deletions(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.h b/src/Storages/Cache/ObjectStorageListObjectsCache.h index b26b490be646..6cb6c3694d93 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.h +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.h @@ -10,6 +10,7 @@ namespace DB class ObjectStorageListObjectsCache { + friend class ObjectStorageListObjectsCacheTest; public: ObjectStorageListObjectsCache(const ObjectStorageListObjectsCache &) = delete; ObjectStorageListObjectsCache(ObjectStorageListObjectsCache &&) noexcept = delete; diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index b6d2fafa1717..d27ea23f7087 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -6,70 +6,74 @@ namespace DB { -auto & initializeAndGetCacheInstance() +class ObjectStorageListObjectsCacheTest : public ::testing::Test { - static auto & cache = ObjectStorageListObjectsCache::instance(); - cache.setTTL(3); - cache.setMaxCount(100); - cache.setMaxSizeInBytes(1000000); - return cache; -} - -static auto & cache = initializeAndGetCacheInstance(); +protected: + void SetUp() override + { + cache = std::unique_ptr(new ObjectStorageListObjectsCache()); + cache->setTTL(3); + cache->setMaxCount(100); + cache->setMaxSizeInBytes(1000000); + } -ObjectStorageListObjectsCache::Key default_key {"default", "test-bucket", "test-prefix/"}; + std::unique_ptr cache; + static ObjectStorageListObjectsCache::Key default_key; -std::shared_ptr createTestValue(const std::vector& paths) -{ - auto value = std::make_shared(); - for (const auto & path : paths) + static std::shared_ptr createTestValue(const std::vector& paths) { - value->push_back(std::make_shared(path)); + auto value = std::make_shared(); + for (const auto & path : paths) + { + value->push_back(std::make_shared(path)); + } + return value; } - return value; -} +}; +ObjectStorageListObjectsCache::Key ObjectStorageListObjectsCacheTest::default_key {"default", "test-bucket", "test-prefix/"}; -TEST(ObjectStorageListObjectsCacheTest, BasicSetAndGet) +TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(default_key, value); + cache->set(default_key, value); + + auto result = cache->get(default_key).value(); - auto result = cache.get(default_key).value(); ASSERT_EQ(result.size(), 2); EXPECT_EQ(result[0]->getPath(), "test-prefix/file1.txt"); EXPECT_EQ(result[1]->getPath(), "test-prefix/file2.txt"); } -TEST(ObjectStorageListObjectsCacheTest, CacheMiss) +TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache->get(default_key)); } -TEST(ObjectStorageListObjectsCacheTest, ClearCache) +TEST_F(ObjectStorageListObjectsCacheTest, ClearCache) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); - cache.set(default_key, value); - cache.clear(); + cache->set(default_key, value); + cache->clear(); - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache->get(default_key)); } -TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) +TEST_F(ObjectStorageListObjectsCacheTest, PrefixMatching) { - cache.clear(); + cache->clear(); auto short_prefix_key = default_key; short_prefix_key.prefix = "parent/"; @@ -85,20 +89,20 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixMatching) "parent/child/grandchild/file1.txt", "parent/child/grandchild/file2.txt"}); - cache.set(mid_prefix_key, value); + cache->set(mid_prefix_key, value); - auto result1 = cache.get(mid_prefix_key).value(); + auto result1 = cache->get(mid_prefix_key).value(); EXPECT_EQ(result1.size(), 2); - auto result2 = cache.get(long_prefix_key).value(); + auto result2 = cache->get(long_prefix_key).value(); EXPECT_EQ(result2.size(), 2); - EXPECT_FALSE(cache.get(short_prefix_key)); + EXPECT_FALSE(cache->get(short_prefix_key)); } -TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) +TEST_F(ObjectStorageListObjectsCacheTest, PrefixFiltering) { - cache.clear(); + cache->clear(); auto key_with_short_prefix = default_key; key_with_short_prefix.prefix = "parent/"; @@ -112,34 +116,34 @@ TEST(ObjectStorageListObjectsCacheTest, PrefixFiltering) "parent/child2/file3.txt" }); - cache.set(key_with_short_prefix, value); + cache->set(key_with_short_prefix, value); - auto result = cache.get(key_with_mid_prefix, true).value(); + auto result = cache->get(key_with_mid_prefix, true).value(); EXPECT_EQ(result.size(), 1); EXPECT_EQ(result[0]->getPath(), "parent/child1/file2.txt"); } -TEST(ObjectStorageListObjectsCacheTest, TTLExpiration) +TEST_F(ObjectStorageListObjectsCacheTest, TTLExpiration) { - cache.clear(); + cache->clear(); const std::string bucket = "test-bucket"; const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); - cache.set(default_key, value); + cache->set(default_key, value); // Verify we can get it immediately - auto result1 = cache.get(default_key).value(); + auto result1 = cache->get(default_key).value(); EXPECT_EQ(result1.size(), 1); std::this_thread::sleep_for(std::chrono::seconds(4)); - EXPECT_FALSE(cache.get(default_key)); + EXPECT_FALSE(cache->get(default_key)); } -TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) +TEST_F(ObjectStorageListObjectsCacheTest, BestPrefixMatch) { - cache.clear(); + cache->clear(); auto short_prefix_key = default_key; short_prefix_key.prefix = "a/b/"; @@ -153,11 +157,11 @@ TEST(ObjectStorageListObjectsCacheTest, BestPrefixMatch) auto short_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt", "a/b/file2.txt"}); auto mid_prefix = createTestValue({"a/b/c/d/file1.txt", "a/b/c/file1.txt"}); - cache.set(short_prefix_key, short_prefix); - cache.set(mid_prefix_key, mid_prefix); + cache->set(short_prefix_key, short_prefix); + cache->set(mid_prefix_key, mid_prefix); // should pick mid_prefix, which has size 2. filter_by_prefix=false so we can assert by size - auto result = cache.get(long_prefix_key, false).value(); + auto result = cache->get(long_prefix_key, false).value(); EXPECT_EQ(result.size(), 2u); } From 9092abad4427ba7093dceeb8e6659ea8191b50af Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Tue, 6 May 2025 15:52:12 -0300 Subject: [PATCH 43/46] remove unused code --- .../tests/gtest_object_storage_list_objects_cache.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp index d27ea23f7087..3b719d4df3e3 100644 --- a/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp +++ b/src/Storages/Cache/tests/gtest_object_storage_list_objects_cache.cpp @@ -36,8 +36,6 @@ ObjectStorageListObjectsCache::Key ObjectStorageListObjectsCacheTest::default_ke TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); cache->set(default_key, value); @@ -52,8 +50,6 @@ TEST_F(ObjectStorageListObjectsCacheTest, BasicSetAndGet) TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; EXPECT_FALSE(cache->get(default_key)); } @@ -61,8 +57,6 @@ TEST_F(ObjectStorageListObjectsCacheTest, CacheMiss) TEST_F(ObjectStorageListObjectsCacheTest, ClearCache) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt", "test-prefix/file2.txt"}); cache->set(default_key, value); @@ -126,8 +120,6 @@ TEST_F(ObjectStorageListObjectsCacheTest, PrefixFiltering) TEST_F(ObjectStorageListObjectsCacheTest, TTLExpiration) { cache->clear(); - const std::string bucket = "test-bucket"; - const std::string prefix = "test-prefix/"; auto value = createTestValue({"test-prefix/file1.txt"}); cache->set(default_key, value); From 057b0b580f4d3ae2627f1c2893c861e42295fff2 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Wed, 7 May 2025 10:29:20 -0300 Subject: [PATCH 44/46] add supportsListObjectsCache to enable this cache for s3-like (minio, gcs, aws) and azure only. Leaving out HDFS, Local and Web --- src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h | 2 ++ src/Disks/ObjectStorages/IObjectStorage.h | 2 ++ src/Disks/ObjectStorages/S3/S3ObjectStorage.h | 2 ++ src/Storages/ObjectStorage/StorageObjectStorageSource.cpp | 2 +- 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h index 5f91a18e4d97..d79d3117ec23 100644 --- a/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h +++ b/src/Disks/ObjectStorages/AzureBlobStorage/AzureObjectStorage.h @@ -31,6 +31,8 @@ class AzureObjectStorage : public IObjectStorage const String & object_namespace_, const String & description_); + bool supportsListObjectsCache() override { return true; } + void listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t max_keys) const override; ObjectStorageIteratorPtr iterate(const std::string & path_prefix, size_t max_keys) const override; diff --git a/src/Disks/ObjectStorages/IObjectStorage.h b/src/Disks/ObjectStorages/IObjectStorage.h index f99864ebb25c..417fa10e5212 100644 --- a/src/Disks/ObjectStorages/IObjectStorage.h +++ b/src/Disks/ObjectStorages/IObjectStorage.h @@ -276,6 +276,8 @@ class IObjectStorage #endif + virtual bool supportsListObjectsCache() { return false; } + private: mutable std::mutex throttlers_mutex; ThrottlerPtr remote_read_throttler; diff --git a/src/Disks/ObjectStorages/S3/S3ObjectStorage.h b/src/Disks/ObjectStorages/S3/S3ObjectStorage.h index f8811785ed39..7fbc119f2a39 100644 --- a/src/Disks/ObjectStorages/S3/S3ObjectStorage.h +++ b/src/Disks/ObjectStorages/S3/S3ObjectStorage.h @@ -81,6 +81,8 @@ class S3ObjectStorage : public IObjectStorage ObjectStorageType getType() const override { return ObjectStorageType::S3; } + bool supportsListObjectsCache() override { return true; } + bool exists(const StoredObject & object) const override; std::unique_ptr readObject( /// NOLINT diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 95f69a233b64..8707683a36b7 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -161,7 +161,7 @@ std::shared_ptr StorageObjectStorageSource::createFileIterator( std::shared_ptr object_iterator = nullptr; std::unique_ptr cache_ptr = nullptr; - if (local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache]) + if (local_context->getSettingsRef()[Setting::use_object_storage_list_objects_cache] && object_storage->supportsListObjectsCache()) { auto & cache = ObjectStorageListObjectsCache::instance(); ObjectStorageListObjectsCache::Key cache_key {object_storage->getDescription(), configuration->getNamespace(), configuration->getPathWithoutGlobs()}; From 49748c92802bfe0e0dd4f43e29167ceb8317e6cf Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Wed, 7 May 2025 13:49:03 -0300 Subject: [PATCH 45/46] increase default cache size --- src/Core/ServerSettings.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/ServerSettings.cpp b/src/Core/ServerSettings.cpp index b9dd72418e5f..bb60b66082b1 100644 --- a/src/Core/ServerSettings.cpp +++ b/src/Core/ServerSettings.cpp @@ -1005,7 +1005,7 @@ namespace DB )", 0) \ DECLARE(Bool, storage_shared_set_join_use_inner_uuid, false, "If enabled, an inner UUID is generated during the creation of SharedSet and SharedJoin. ClickHouse Cloud only", 0) \ DECLARE(UInt64, input_format_parquet_metadata_cache_max_size, 500000000, "Maximum size of parquet file metadata cache", 0) \ - DECLARE(UInt64, object_storage_list_objects_cache_size, 5000000, "Maximum size of ObjectStorage list objects cache in bytes. Zero means disabled.", 0) \ + DECLARE(UInt64, object_storage_list_objects_cache_size, 500000000, "Maximum size of ObjectStorage list objects cache in bytes. Zero means disabled.", 0) \ DECLARE(UInt64, object_storage_list_objects_cache_max_entries, 1000, "Maximum size of ObjectStorage list objects cache in entries. Zero means disabled.", 0) \ DECLARE(UInt64, object_storage_list_objects_cache_ttl, 3600, "Time to live of records in ObjectStorage list objects cache in seconds. Zero means unlimited", 0) \ // clang-format on From 96cf2d2999ed83b5b9a15d438bdf7bad93057d73 Mon Sep 17 00:00:00 2001 From: Arthur Passos Date: Thu, 8 May 2025 11:59:49 -0300 Subject: [PATCH 46/46] fix weight funciton --- .../Cache/ObjectStorageListObjectsCache.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp index 06614397a734..a1d0636a48ed 100644 --- a/src/Storages/Cache/ObjectStorageListObjectsCache.cpp +++ b/src/Storages/Cache/ObjectStorageListObjectsCache.cpp @@ -107,7 +107,20 @@ size_t ObjectStorageListObjectsCache::WeightFunction::operator()(const Value & v for (const auto & object : value) { - weight += object->relative_path.capacity() + sizeof(ObjectMetadata); + const auto object_metadata = object->metadata; + weight += object->relative_path.capacity() + sizeof(object_metadata); + + // variable size + if (object_metadata) + { + weight += object_metadata->etag.capacity(); + weight += object_metadata->attributes.size() * (sizeof(std::string) * 2); + + for (const auto & [k, v] : object_metadata->attributes) + { + weight += k.capacity() + v.capacity(); + } + } } return weight;