Export partition - allow non matching partition expressions iff monotonicity is proved and data is not repartitioned#2074
Conversation
|
@codex review |
![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: 3f3af7c4fc
ℹ️ 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".
| castColumn({std::move(values_column), source_type, column}, destination_type), destination_type, column}; | ||
|
|
||
| const auto dest_transform_with_tz = Iceberg::parseTransformAndArgument(dest_transform, partition_timezone); | ||
| if (!destinationTransformIsConstant(*dest_transform_with_tz, cast_values, context)) |
There was a problem hiding this comment.
When this dynamic check succeeds in the replicated EXPORT PARTITION path, the proof is only for the scheduling-time parts; the manifest still persists just partition_id, and commit later calls getPartitionSourceBlockForIcebergCommit, which reads the current active part for that partition. If a coarser source partition is accepted because its current rows map to one destination partition (for example toYear(ts) into Iceberg day(ts)) and, before commit, another day is inserted and merged with the exported parts, the committed manifest can tag the already exported files with the merged/current part's representative value instead of the exported rows' value. Persist/use representative values from the validated parts, or keep these cases structural-only for replicated exports.
Useful? React with 👍 / 👎.
| { | ||
| if (const auto * id = arg->as<ASTIdentifier>()) | ||
| column = id->name(); | ||
| else if (const auto * lit = arg->as<ASTLiteral>(); lit && lit->value.getType() != Field::Types::String) |
There was a problem hiding this comment.
This drops string literals from source partition terms, so toRelativeDayNum(ts, 'UTC') is treated as the same structural transform as the destination Iceberg day(ts) transform. With iceberg_partition_timezone set differently for the export (for example America/Los_Angeles), one UTC-day source partition can contain rows from two destination days; the structural fast path then skips the dynamic min/max proof and commits them as a single Iceberg partition. Carry/compare the timezone literal, or force timezone-qualified source terms through the dynamic proof.
Useful? React with 👍 / 👎.
|
Consider mentioning 'export' in the title and/or description |
arthurpassos
changed the title
Done, I'll soon add the description |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca43cf1f6d
ℹ️ 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".
| const auto parts = storage.getDataPartsVectorInPartitionForInternalUsage( | ||
| MergeTreeDataPartState::Active, partition_id, lock); | ||
|
|
||
| if (parts.empty()) | ||
| {MergeTreeDataPartState::Active, MergeTreeDataPartState::Outdated}, partition_id, lock); |
There was a problem hiding this comment.
When an Iceberg catalog commit is retried long enough for an exported part to be merged and its outdated part to be cleaned up, this lookup finds no manifest-named part and throws NO_SUCH_DATA_PART. Retrying cannot recover it because the manifest stores only part names, not the already-validated representative partition values; with the task timeout much longer than old-part retention, a transient commit outage followed by normal merges leaves the export permanently unable to commit its already-uploaded files. Persist the representative source block/tuple in the manifest when scheduling (or retain the parts until commit) instead of relying on live old parts.
Useful? React with 👍 / 👎.
3 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
...
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: