use parquet metadata cache for parquetmetadata format as well #636
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arthurpassos
commented
Feb 18, 2025
| namespace DB | ||
| { | ||
|
|
||
| ParquetFileMetaDataCache::ParquetFileMetaDataCache() |
Collaborator
Author
There was a problem hiding this comment.
This looks too silly
Collaborator
There was a problem hiding this comment.
Why do we need this singleton here? Can't the cache be just a member somewhere?
Collaborator
Author
There was a problem hiding this comment.
It is used by two classes: ParquetBlockInputFormat and ParquetMetadataInputFormat
arthurpassos
commented
Feb 18, 2025
| } | ||
| } | ||
|
|
||
| static std::shared_ptr<parquet::FileMetaData> getFileMetadata( |
Collaborator
Author
There was a problem hiding this comment.
There are two implementations of getFileMetadata. One for ParquetBlockInputFormat.cpp and one for ParquetMetadataInputFormat. I am still not sure if I should leave it duplicated or put it somewhere.. The logic is pretty much the same, but requires a couple of arguments to be passed
Collaborator
There was a problem hiding this comment.
I do not see a lot of duplication (as code is really a bit different). But it depends on whether we are planning to keep this here or move it to upstream later.
If we are keeping it here, I would avoid unnecessary refactoring in this case -- this code does not look like it will change a lot, but changing code in more places may cause additional conflicts
arthurpassos
force-pushed
the
project-antalya-24.12.2_metadata_cache_fixes_1
branch
from
175a172 to
cb8d032
Compare
Collaborator
|
This is an automated comment for commit 8abf0b1 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
Collaborator
Author
|
ParquetSchemaReader also calls One thing to note is that Therefore, we have the following cases: when reading a single file: it would be benefitial to add the caching logic there as well, because later on when reading multiple files: wouldn't make a big difference since that code is meant to read the schema, and the schema for a single file suffices for the whole bunch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous implementation #586 only applied caching to blockinput format. In order to re-use and share the cache, some refactoring had to be done. Cache initialization was moved to
Server.cpp.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Use ParquetMetadataCache for ParquetMetadata format as well.
Documentation entry for user-facing changes
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:
Only specified batches in multi-batch jobs: