Parquet File Metadata caching implementation #541

shiyer7474 · 2024-12-01T02:42:18Z

Implements Parquet Metadata caching.

Changelog category (leave one):

New Feature

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

Modify your CI run:

NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step

Include tests (required builds will be added automatically):

Exclude tests:

Extra options:

do not test (only style check)
disable merge-commit (no merge from master before tests)
disable CI cache (job reuse)

Only specified batches in multi-batch jobs:

1
2
3
4

altinity-robot · 2024-12-01T05:41:56Z

This is an automated comment for commit 5a7a8ad with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Sign aarch64	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ error
Sign release	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	❌ error
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure

Successful checks

Check name	Description	Status
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Ready for release	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success

svb-alt · 2024-12-06T02:13:43Z

resolves #480
resolves #481

arthurpassos · 2024-12-06T14:15:57Z

src/Processors/Formats/Impl/ParquetBlockInputFormat.cpp

+{
+    if (!use_metadata_cache || !metadata_cache_key.length())
+    {
+        ProfileEvents::increment(ProfileEvents::ParquetMetaDataCacheMisses);


Do we really want metadata cache miss metrics being incremented when use_metadata_cache=false?

I coded the increment in the first pass, but removed it so that users need not see a new alarming "miss" metric if they are unaware/disabled the in-memory cache feature (and satisfied with the existing local disk caching feature). Let me know what you think.

I am sorry, I did not quite follow your explanation. As far as I can tell (by reading the code), whenever initializeIfNeeded() is called (and it is called once per file, iirc), it'll try to fetch metadata either from cache or from input source.

If cache is disabled, it'll still increment the cache miss. It doesn't look right to me at a first glance, but if there has been a discussion and this was the chosen design, that's ok.

Am I missing something?

Correct, I mixed up my revisions and reasoning. I have removed the increment if cache is disabled. Thanks!

arthurpassos · 2024-12-06T14:19:15Z

src/Processors/Formats/Impl/ParquetBlockInputFormat.cpp

+ParquetFileMetaDataCache::ParquetFileMetaDataCache(UInt64 max_cache_entries)
+    : CacheBase(max_cache_entries) {}
+
+ParquetFileMetaDataCache *  ParquetFileMetaDataCache::instance(UInt64 max_cache_entries)


I understand this has to be a singleton, but what if the user performs queries with different values for parquet_metadata_cache_max_entries?

I am still nto sure how to fix this problem AND if we need to fix it at all

We need to document that the cache will be sized to the setting value at "first time use"! Dynamic resizing is not possible. I have seen some settings documented like that.

Dynamic resize of cache, maybe upsize only, can be implemented later.

https://github.com/Altinity/ClickHouse/pull/541/files#r1890373736

arthurpassos · 2024-12-06T14:19:29Z

tests/queries/0_stateless/03262_parquet_s3_metadata_cache.sql

@@ -0,0 +1,28 @@
+-- Tags: no-parallel, no-fasttest


can you also add a few tests for parquet_metadata_cache_max_entries?

That would have to be an integration test, maybe with 10s or 100's of parquet files. I can add it in another PR.

If local files also benefited from metadata cache, an integration test wouldn't be needed I suppose. But doesn't look like we want to do it

For local Parquet files, OS file cache will be in effect.

arthurpassos · 2024-12-06T14:23:56Z

src/Processors/Formats/Impl/ParquetBlockInputFormat.h

+
+private:
+    ParquetFileMetaDataCache(UInt64 max_cache_entries);
+    static std::mutex mutex;


It appears to me CacheBase already has a mutex https://github.com/ClickHouse/ClickHouse/blob/94709d7a5a1ede65a9eed92ff9cbf73e28e62561/src/Common/CacheBase.h#L245.

Why do we need another one? And why is it static?

I think I am missing something. ParquetFileMetaDataCache is a static instance, shared among all instances. Why do you need to make this mutex static as well?

Yes, basic double checked locking pattern of singleton initialization.

I will replace this with the modern equivalent that uses std::once_only.

Check my comment in https://github.com/Altinity/ClickHouse/pull/541/files#r1875950244. I might be missing something, tho

src/Processors/Formats/Impl/ParquetBlockInputFormat.h

arthurpassos · 2024-12-09T13:03:59Z

src/Processors/Formats/Impl/ParquetBlockInputFormat.cpp

-    {
-        std::lock_guard lock(mutex);
+    static std::once_flag once;
+    std::call_once(once, [&] {


Ok, let's take a step back. IIRC, static initialization in c++ 11 or greater is guaranteed to happen only once and it is thread safe.

From the cpp standard:

such a variable is initialized the first time control passes through its declaration; such a variable is considered initialized upon the completion of its initialization. [...] If control enters the declaration concurrently while the variable is being initialized, the concurrent execution shall wait for completion of the initialization.

So, wouldn't something like the below suffice?:

static ParquetFileMetaDataCache instance(max_cache_entries); return instance;

src/Core/Settings.h

…ame setting

arthurpassos · 2024-12-18T14:54:11Z

src/Core/ServerSettings.h

@@ -166,7 +166,8 @@ namespace DB
    M(String, mutation_workload, "default", "Name of workload to be used to access resources for all mutations (may be overridden by a merge tree setting)", 0) \
    M(Bool, prepare_system_log_tables_on_startup, false, "If true, ClickHouse creates all configured `system.*_log` tables before the startup. It can be helpful if some startup scripts depend on these tables.", 0) \
    M(UInt64, config_reload_interval_ms, 2000, "How often clickhouse will reload config and check for new changes", 0) \
-    M(Bool, disable_insertion_and_mutation, false, "Disable all insert/alter/delete queries. This setting will be enabled if someone needs read-only nodes to prevent insertion and mutation affect reading performance.", 0)
+    M(Bool, disable_insertion_and_mutation, false, "Disable all insert/alter/delete queries. This setting will be enabled if someone needs read-only nodes to prevent insertion and mutation affect reading performance.", 0) \
+    M(UInt64, input_format_parquet_metadata_cache_max_entries, 100000, "Maximum number of parquet file metadata to cache.", 0)


Turned it into a server settings, makes more sense as it can't be changed at runtime

Parquet File Metadata caching implementation

d63fd14

arthurpassos reviewed Dec 6, 2024

View reviewed changes

src/Processors/Formats/Impl/ParquetBlockInputFormat.h Outdated Show resolved Hide resolved

arthurpassos reviewed Dec 6, 2024

View reviewed changes

src/Processors/Formats/Impl/ParquetBlockInputFormat.h Outdated Show resolved Hide resolved

Code review comments

415b351

arthurpassos reviewed Dec 9, 2024

View reviewed changes

shiyer7474 and others added 2 commits December 18, 2024 03:12

More code review

b928250

Rename setting on code

f8a2ad9

arthurpassos reviewed Dec 18, 2024

View reviewed changes

src/Core/Settings.h Show resolved Hide resolved

arthurpassos added 5 commits December 18, 2024 11:24

avoid 1 extra s3 call to get file size in case it is not needed & ren…

3a992b0

…ame setting

add comment

a78f188

update test setting name

9add7d8

minor update to test

465e96e

make parquet_metadata_cache_max_entries a server setting

861bdf5

arthurpassos reviewed Dec 18, 2024

View reviewed changes

settings history

5a7a8ad

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parquet File Metadata caching implementation #541

Parquet File Metadata caching implementation #541

shiyer7474 commented Dec 1, 2024

altinity-robot commented Dec 1, 2024 •

edited

Loading

svb-alt commented Dec 6, 2024

arthurpassos Dec 6, 2024

shiyer7474 Dec 9, 2024

arthurpassos Dec 9, 2024

shiyer7474 Dec 18, 2024

arthurpassos Dec 6, 2024

shiyer7474 Dec 9, 2024

arthurpassos Dec 18, 2024

arthurpassos Dec 6, 2024

shiyer7474 Dec 9, 2024

arthurpassos Dec 9, 2024

shiyer7474 Dec 18, 2024

arthurpassos Dec 6, 2024

shiyer7474 Dec 9, 2024

arthurpassos Dec 9, 2024

shiyer7474 Dec 18, 2024

arthurpassos Dec 9, 2024

shiyer7474 Dec 18, 2024

arthurpassos Dec 18, 2024

Parquet File Metadata caching implementation #541

Are you sure you want to change the base?

Parquet File Metadata caching implementation #541

Conversation

shiyer7474 commented Dec 1, 2024

Changelog category (leave one):

Modify your CI run:

Include tests (required builds will be added automatically):

Exclude tests:

Extra options:

Only specified batches in multi-batch jobs:

altinity-robot commented Dec 1, 2024 • edited Loading

svb-alt commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altinity-robot commented Dec 1, 2024 •

edited

Loading