Preserve the entire format settings object in export part manifest #1144
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| destination_file_path, | ||
| manifest.overwrite_file_if_exists, | ||
| format_settings, | ||
| context_copy); |
There was a problem hiding this comment.
The call to destination_storage->import now passes format_settings and context_copy, but both identifiers are undefined in this function after the struct switched to storing the whole FormatSettings. This code will not compile. The import call should use the settings carried in manifest.format_settings (wrapped in an optional to match the new signature) and pass an existing context (local_context or a freshly created copy) instead of relying on removed variables.
Useful? React with 👍 / 👎.
| const StorageID & destination_storage_id_, | ||
| const DataPartPtr & data_part_, | ||
| bool overwrite_file_if_exists_, | ||
| bool parallel_formatting_, | ||
| bool parallel_formatting_parquet_, | ||
| std::size_t max_threads_) | ||
| const FormatSettings & format_settings_) | ||
| : destination_storage_id(destination_storage_id_), | ||
| data_part(data_part_), | ||
| overwrite_file_if_exists(overwrite_file_if_exists_), | ||
| parallel_formatting(parallel_formatting_), | ||
| parquet_parallel_encoding(parallel_formatting_parquet_), | ||
| max_threads(max_threads_), | ||
| format_settings(format_settings_) | ||
| create_time(time(nullptr)) {} | ||
|
|
||
| StorageID destination_storage_id; | ||
| DataPartPtr data_part; | ||
| bool overwrite_file_if_exists; | ||
| bool parallel_formatting; | ||
| /// parquet has a different setting for parallel formatting | ||
| bool parquet_parallel_encoding; | ||
| std::size_t max_threads; | ||
| FormatSettings format_settings; |
There was a problem hiding this comment.
MergeTreeExportManifest now stores a FormatSettings instance by value, but the header does not include the type’s definition. Any translation unit that includes MergeTreeData.h (and thus this header) will see an unknown type and fail to compile. Include <Formats/FormatFactory.h> or another header that defines FormatSettings so the struct can be instantiated.
Useful? React with 👍 / 👎.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Preserve the entire format settings object in export part manifest
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: