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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e055b1f46d
ℹ️ 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 slot_it = std::find(minmax_column_names.begin(), minmax_column_names.end(), column); | ||
| if (slot_it == minmax_column_names.end()) |
There was a problem hiding this comment.
When source and destination schemas contain reordered same-typed columns, this name-based lookup validates a different value from the one exported because ExportPartTask::addExportConvertingActions converts columns with MatchColumnsMode::Position. For example, with source columns (x, ts) partitioned monthly by ts and destination columns (ts, x) partitioned daily by ts, a part whose source ts values fall on one day passes this proof even when source x spans several days; the destination ts actually receives x, while the Hive path or Iceberg metadata is derived from the source column named ts. Map each destination partition column to its positional source column before checking min/max and constructing the representative block.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think we shall fix this in a separate PR
|
Hey @arthurpassos — while writing regression tests for this PR I ran into a What I tried A single CREATE TABLE bug_repro
(
id Int64,
event_time DateTime('UTC')
)
ENGINE = IcebergS3(
'http://minio:9000/warehouse/data/bug_repro/',
'<minio_root_user>',
'<minio_root_password>'
)
PARTITION BY toRelativeDayNum(event_time, 'UTC')
SETTINGS s3_retry_attempts = 1;With What happens Why I'm asking here rather than filing separately
What works (narrows it)
So the fault is specifically Questions
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38481944eb
ℹ️ 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".
| /// The written value is transform(cast(source)). A value-preserving cast is order-preserving, so | ||
| /// the endpoints bound every interior row; a lossy cast may wrap, so require it monotonic over the | ||
| /// partition's actual range, otherwise the endpoints prove nothing about the interior. | ||
| bool cast_is_monotonic = canBeSafelyCast(source_type, destination_type); |
There was a problem hiding this comment.
When an integer partition column is converted to String, canBeSafelyCast returns true because the cast preserves values, but it is not order-preserving: for a source PARTITION BY intDiv(k, 100) containing k = 2, 10, 29 and an Iceberg destination with k String PARTITION BY icebergTruncate(1, k), the endpoints cast to "2" and "29" and both truncate to "2", while the interior value 10 truncates to "1". The gate therefore accepts a source partition spanning multiple destination partitions, and the commit records the representative endpoint's partition for every exported file, producing incorrect Iceberg metadata and potentially wrong partition-pruned results. Value preservation should not bypass the cast monotonicity check.
Useful? React with 👍 / 👎.
This reverts commit 3848194.
|
@Selfeer I also wondered about this very same timezone case. As far as I could understand from apache iceberg docs and articles, the This is not crystal clear, tho. The docs are not very explicit. For this reason, I think @ianton-ru introduced the Export partition must respect it because it exists, even if it is non compliant. That's why I respect |
4 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Allow export partition through different partition expressions as long as the destination expression does not repartition the data. This is asserted through a destination expression monotonicity check on the source minmax values. Destination expression columns must be a subset of the source.
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: