add setting to define filename pattern for part exports#1490
Open
arthurpassos wants to merge 5 commits intoantalya-26.1from
Open
add setting to define filename pattern for part exports#1490arthurpassos wants to merge 5 commits intoantalya-26.1from
arthurpassos wants to merge 5 commits intoantalya-26.1from
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.
Reviewed commit: d69971b4f2
ℹ️ 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".
| manifest.parquet_parallel_encoding = json->getValue<bool>("parquet_parallel_encoding"); | ||
| manifest.max_bytes_per_file = json->getValue<size_t>("max_bytes_per_file"); | ||
| manifest.max_rows_per_file = json->getValue<size_t>("max_rows_per_file"); | ||
| manifest.filename_pattern = json->getValue<String>("filename_pattern"); |
There was a problem hiding this comment.
Deserialization now requires filename_pattern unconditionally, but metadata written by earlier versions does not include this key. Any node that reads an older exports/.../metadata.json (for example while checking existing exports or canceling an export in StorageReplicatedMergeTree) will throw during fromJsonString, breaking in-flight export management after upgrade. Make this field optional and fall back to the default pattern when absent.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Nobody is using this feature yet, it is ok to introduce backwards incompatible changes like this. We literally have 0 users so far.
ilejn
reviewed
Mar 9, 2026
| - **Type**: `String` | ||
| - **Default**: `{part_name}_{checksum}` | ||
| - **Description**: Pattern for the filename of the exported merge tree part. The `part_name` and `checksum` are calculated and replaced on the fly. Additional macros are supported. | ||
|
|
Collaborator
There was a problem hiding this comment.
Why do we duplicate part_export.md content here?
Collaborator
Author
There was a problem hiding this comment.
Well, this is export partition (slightly different feature), and at some point there might be settings that are not supported by export partition and only by export part.
I don't have a good answer tbh.
ilejn
reviewed
Mar 9, 2026
| Macros::MacroExpansionInfo macro_info; | ||
| macro_info.table_id = storage_id; | ||
| filename = local_context->getMacros()->expand(filename, macro_info); | ||
|
|
Collaborator
There was a problem hiding this comment.
Why do we need special logic from {part_name} and {checksum}?
In other words, why we do not put it inside expand() ?
Collaborator
Author
There was a problem hiding this comment.
Because part_name and checksum are calculated on the fly based on the data part being exported. They are not meant to be extracted from macros, it would not even work tbh
arthurpassos
added
port-antalya
Collaborator
|
test_export_replicated_mt_partition_to_object_storage/test.py::test_export_partition_from_replicated_database_uses_db_shard_replica_macros test failure could be related to this PR. |
Collaborator
Author
This was an interesting one: it succeeds in all suites, except for
I vote for n1. |
Collaborator
PR #1490 Audit ReviewAI audit note: This review was generated by AI (gpt-5.3-codex). Audit update for PR #1490 (export filename pattern for part/partition export) Confirmed defectsMedium: Backward-incompatible manifest deserialization for existing partition exports
Medium:
|
Collaborator
PR #1490 CI TriagePR: Altinity/ClickHouse#1490 Summary
Detailed AnalysisPR-Caused Regressions (Both Addressed)1.
|
| Test | Occurrences |
|---|---|
02815_no_throw_in_simple_queries |
2 runs |
03206_no_exceptions_clickhouse_local |
2 runs |
03441_deltalake_clickhouse_public_datasets |
2 runs |
03441_deltalake_clickhouse_virtual_columns |
2 runs |
CI Status by Build Type
| Build | Status |
|---|---|
| Build (amd_debug) | PASS |
| Build (amd_release) | PASS |
| Build (amd_asan) | PASS |
| Build (amd_binary) | PASS |
| Build (arm_asan) | PASS |
| Build (arm_binary) | PASS |
| Build (arm_release) | PASS |
| Install packages (amd_release) | PASS |
| Install packages (arm_release) | PASS |
| Quick functional tests | PASS |
| AST fuzzer (amd_debug) | PASS |
| BuzzHouse (amd_debug) | PASS |
| Stress test (amd_debug) | PASS |
| Stress test (arm_asan) | PASS |
| Stress test (arm_asan, s3) | PASS |
| Unit tests (asan) | PASS |
| Stateless tests (amd_asan, distributed plan, parallel, 1/2) | PASS |
| Stateless tests (amd_asan, distributed plan, parallel, 2/2) | PASS |
| Stateless tests (amd_binary, ParallelReplicas, s3 storage, parallel) | PASS |
| Stateless tests (amd_debug, distributed plan, s3 storage, parallel) | PASS |
| Stateless tests (arm_binary, parallel) | PASS |
| Stateless tests (arm_asan, targeted) | PASS |
| Integration tests (amd_asan, targeted) | PASS |
| Integration tests (amd_asan, db disk, old analyzer, 1-5/6) | PASS |
| Integration tests (amd_binary, 1-5/5) | PASS |
| Integration tests (arm_binary, distributed plan, 1,2,4/4) | PASS |
Recommendations
-
No blocking PR-caused failures remain. Both PR-caused regressions have been addressed:
- Settings history updated in commit
2ca197c9 - Export partition integration test skipped for remote disk configuration in commit
4120b578
- Settings history updated in commit
-
Wait for pending integration test reruns on the latest commit to confirm the export partition test fix works correctly.
-
The
00145_aggregate_functions_statisticsflaky test is a known issue unrelated to this PR and should not block merge. -
The S3 queue AzureQueue flaky test is a known issue unrelated to this PR and should not block merge.
-
The infrastructure failure on
Stateless tests (amd_debug, distributed plan, s3 storage, sequential)should be rerun if a clean CI is needed.
Conclusion
This PR is safe to merge from a CI perspective. All actual test failures are either:
- Fixed by subsequent commits in the PR
- Pre-existing flaky tests unrelated to the changes
- Infrastructure issues
Collaborator
@arthurpassos can you check if the issues make sense? |
Collaborator
Author
1 is not relevant |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add setting to define filename pattern for part exports - helps with sharding - port of unmerged and unreviewed PR #1383
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: