Export partition - allow non matching partition expressions in case we can prove the destination expression does not split the data

[arthurpassos]

Changelog category (leave one):

• Improvement

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 validated at schedule time through two mechanisms:

1. Structural match: in case the source and destination are identical, the destination expression is a subset of the source expression or the destination expression can be entirely computed using only constants and the exact values guaranteed (pinned) by the source.
2. Dynamic proof: the destination expression is monotonic over the source partition min/max range.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• Aarch64 tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• OAuth (5m)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [35605c1]

[arthurpassos]

@codex review

[chatgpt-codex-connector]

💡 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".

[ilejn]

Consider mentioning 'export' in the title and/or description

[arthurpassos]

Consider mentioning 'export' in the title and/or description

Done, I'll soon add the description

[arthurpassos]

@codex review

[chatgpt-codex-connector]

💡 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".

[Selfeer]

Hey @arthurpassos - separate from the positional/name column-matching issue. I want to confirm the intended behaviour here before I pin it in a test.

What happens

A destination with an extra column is rejected on column count:

CREATE TABLE src (id Int64, a Int32)
ENGINE = ReplicatedMergeTree('/clickhouse/tables/shard0/src', '{replica}')
ORDER BY tuple() PARTITION BY a;

CREATE TABLE dst (id Int64, a Int32, b Int32)
ENGINE = S3(..., format='Parquet', partition_strategy='hive') PARTITION BY a;

INSERT INTO src VALUES (1, 42);

ALTER TABLE src EXPORT PARTITION ID '42' TO TABLE dst;
-- Code: 20. Number of columns doesn't match (source: 2 and result: 3).

That one seems right to me.

But giving the extra column a DEFAULT changes nothing:

CREATE TABLE dst (id Int64, a Int32, b Int32 DEFAULT 42)
ENGINE = S3(..., format='Parquet', partition_strategy='hive') PARTITION BY a;

ALTER TABLE src EXPORT PARTITION ID '42' TO TABLE dst;
-- Code: 20. Number of columns doesn't match (source: 2 and result: 3).

Same error, same counts - the DEFAULT is not taken into account.

What I'd expect

I'd expect the second case to succeed, with b filled from its default:

SELECT id, a, b FROM dst;
-- 1  42  42

which is what

INSERT INTO dst (id, a)
SELECT id, a
FROM src;

would produce.

A DEFAULT column is exactly the case where the destination can supply the value itself, so requiring the source to provide it seems off.

Is the current behaviour intended? There's no data-loss risk either way since it's a loud rejection - I just want the test to assert the intended behaviour rather than whichever one I guess at.

[k-morozov]

I suggest adding setting to turn on/off this functionality.

[k-morozov]

Seems that exsits a case where term is empty. Is this expected behavior?

[Selfeer]

Can you check #2138 - this does seem like we should not be allowing the export here or have some other fix in this case.

[Selfeer]

I think jump also reported the same thing and you agreed that this is okay, @arthurpassos but please check the issue and if all is right even with the Insert and Export behavior being different - I'll just update the expectations of our tests.

[Selfeer]

Audit Review — PR #2074

PR: Altinity/ClickHouse#2074 — Export partition: allow non-matching partition expressions iff monotonicity is proved and data is not repartitioned

AI audit note: This review was generated by AI (Claude Opus 5). Static reasoning only — no runtime execution was performed.

---

Summary of findings

#	Severity	Defect	Affected path
1	🔴 High	Argument order of destination partition expressions is lost — monotonicity proof evaluates the wrong expression	Plain (wildcard) destinations
2	🟠 Medium	Iceberg commit permanently fails when no exported part is left on the committing replica	Iceberg commit
3	🟡 Low	Nullable guard misses LowCardinality(Nullable) partition key columns	Validation
4	🟡 Low	Unsupported destination term shapes surface as raw internal exceptions	Validation (error contract)
5	🟡 Low	Force re-export deletes previous export state before the new compatibility check runs	Schedule path

---

1. 🔴 High — Argument order lost, monotonicity proof evaluates the wrong expression

Anchor: src/Storages/MergeTree/ExportPartitionUtils.cpp — parsePartitionTerms (~line 586) and the argument rebuild in verifyColumnMapsToSinglePartition (~lines 722–731)

Impact: A source partition that spans multiple destination partitions can pass validation. All rows of a part are then written into the single directory computed from the part's min row — rows are silently misplaced into a wrong destination partition, and readers relying on hive/wildcard partition pruning get wrong query results.

Trigger (smallest realistic case):

• Destination: plain object storage, partition_strategy='wildcard', PARTITION BY intDiv(x, 100)
• Source: coarser key, e.g. PARTITION BY intDiv(x, 1000), with a part whose x range is [201, 350]

Why it is a defect:

PartitionTerm stores only {column, function, argument} and discards argument positions. verifyColumnMapsToSinglePartition always rebuilds the call as f(argument, column). For the trigger it therefore checks intDiv(100, x) instead of intDiv(x, 100):

• FunctionBinaryArithmetic reports the const / variable shape monotonic for strictly positive ranges;
• intDiv(100, 201) == intDiv(100, 350) == 0, so validation accepts;
• the true values are intDiv(201, 100) = 2 and intDiv(350, 100) = 3 — the part spans two destination partitions.

intDiv is non-injective, so equality of the swapped expression does not imply equality of the real one. Two more consequences of the same root cause:

• Structural match is order-insensitive too: sourceHasMatchingPartitionTerm treats source intDiv(N, x) and destination intDiv(x, N) as identical terms and skips validation entirely.
• False rejects: e.g. source range [100, 150] with destination intDiv(x, 100) maps to a single true partition, but intDiv(100, 100) != intDiv(100, 150) — a valid export is refused.

Iceberg destinations are unaffected by luck of signature: icebergTruncate / icebergBucket take the width as their first argument, and the date transforms are unary.

Fix direction: Store the full ordered argument list (or the sub-AST) in PartitionTerm, rebuild the call preserving positions, and compare terms order-sensitively.

Regression test direction: Stateless test exporting from PARTITION BY intDiv(x, 1000) into a wildcard destination with PARTITION BY intDiv(x, 100) and a part range straddling a destination boundary (must be rejected), plus an accept-case with a non-straddling range.

---

2. 🟠 Medium — Iceberg commit permanently fails when no exported part remains locally

Anchor: src/Storages/MergeTree/ExportPartitionUtils.cpp — getPartitionSourceBlockForIcebergCommit (~line 147)

Impact: An export that has already uploaded all data files retries the commit until export_merge_tree_partition_task_timeout_seconds, then transitions to FAILED. The exported files never become visible in the Iceberg table; the work is lost.

Trigger: Exported parts are merged away and cleaned up (old_parts_lifetime), and the in-memory part_references pinning is lost — e.g.:

• server restart between part export and commit (restart-tolerance is an explicit design goal of this feature); or
• the commit is picked up by a replica that no longer holds any of the manifest's parts. Single-replica setups have no peer to fall back to.

Why it is a defect: The function now requires at least one part from manifest.parts to be present in {Active, Outdated} states to derive the partition min values. NO_SUCH_DATA_PART is classified retryable, but the condition is permanent once the parts are gone — retries can never succeed. The previous code accepted any active part of the partition; that is unsound under the new relaxed matching, but the replacement forfeits liveness instead.

Fix direction: Persist the folded min/max (or the derived partition source block) in the ZooKeeper manifest at schedule time and use it at commit — removing the dependency on local parts entirely.

Regression test direction: Integration test: schedule an Iceberg export, let all source parts merge, drop old parts (or restart the node), then assert the commit still succeeds.

---

3. 🟡 Low — Nullable guard misses LowCardinality(Nullable) columns

Anchor: src/Storages/MergeTree/ExportPartitionUtils.cpp — verifyColumnMapsToSinglePartition (~line 668)

Impact: With allow_nullable_key = 1 and a LowCardinality(Nullable(T)) partition column, NULL rows form their own destination partition undetected by the min/max proof — a source partition containing NULLs plus values can be exported into a single destination partition, misplacing the NULL rows.

Why it is a defect: source_type->isNullable() is false for LowCardinality(Nullable(T)). The guard exists precisely because min/max statistics cannot rule out a NULL sub-partition, and it silently does not fire for the wrapped type.

Fix direction: Use isNullableOrLowCardinalityNullable (and unwrap LowCardinality before the cast/monotonicity checks).

Regression test direction: Stateless test with a LowCardinality(Nullable) partition key column containing NULLs; export must be rejected.

---

4. 🟡 Low — Unsupported term shapes produce raw internal exceptions

Anchor: src/Storages/MergeTree/ExportPartitionUtils.cpp — parsePartitionTerms + function rebuild in verifyColumnMapsToSinglePartition

Impact: Scheduling fails with confusing errors instead of the intended BAD_ARGUMENTS explanation. All paths are fail-closed (no data at risk), but the error contract is inconsistent, and one case is a false reject:

Destination key shape	What happens
toStartOfInterval(ts, INTERVAL 1 DAY)	nested-function argument is dropped; rebuilt call has wrong arity → NUMBER_OF_ARGUMENTS_DOESNT_MATCH
Float literal in the key	Field::safeGet<Int64> throws BAD_GET
Nested expression, e.g. toYYYYMM(toDate(ts))	error message names column ''
Iceberg void transform	mapped to tuple, which has no monotonicity → always rejected, although void never repartitions anything

Fix direction: Reject unparseable/nested terms explicitly with BAD_ARGUMENTS before building the function; special-case void as trivially single-valued.

Regression test direction: Negative stateless tests asserting BAD_ARGUMENTS for each shape; an accept-case for a void field in the Iceberg spec.

---

5. 🟡 Low — Force re-export deletes previous export state before validation

Anchor: src/Storages/StorageReplicatedMergeTree.cpp — exportPartitionToTable (tryRemoveRecursive runs before verifyPlainPartitionCompatibility)

Impact: EXPORT PARTITION ... SETTINGS export_merge_tree_partition_force_export = 1 to a plain destination with an incompatible partition key removes the existing export's ZooKeeper state (killing an in-progress export) and then throws — leaving no export scheduled at all.

Why it is a defect: Before this PR the plain-destination partition-key check ran at the top of the function, before any ZooKeeper mutation. The new check needs parts, which are collected after the destructive tryRemoveRecursive. (The Iceberg check already had this ordering, so for Iceberg destinations this is pre-existing.)

Fix direction: Collect parts and run both partition-compatibility checks before the force-removal of the previous export.

Regression test direction: Integration test: force re-export with an incompatible destination key; assert the previous export state survives.

---

Coverage summary

Scope reviewed — full PR diff (13 files):

• term parsing and structural matching; cast + transform monotonicity proof
• plain and Iceberg schedule-time gates in MergeTreeData::exportPartToTable / StorageReplicatedMergeTree::exportPartitionToTable
• Iceberg commit min/max derivation; iceberg_partition_timezone manifest plumbing
• write paths (ExportPartTask → StorageObjectStorage::import → computePartitionKey)
• commit retry/failure classification; part pinning lifecycle; test changes

Categories failed: destination-term reconstruction (argument order); commit-time part availability (restart); Nullable-wrapper gating; error-contract consistency; force-export rollback ordering.

Categories passed:

• Iceberg transform mapping and width-first argument order
• structural match type gating — including bucket being structural-only, since icebergBucketTransform exposes no monotonicity
• cast monotonicity gate — non-monotonic casts such as Int → String correctly fail closed because CAST reports no monotonicity for them
• date transforms (toYearNumSinceEpoch, toMonthNumSinceEpoch, toRelativeDayNum, toRelativeHourNum) have monotonicity via IFunctionDateOrDateTime
• unpartitioned-destination fast paths; MinMaxIndex fold soundness, including partial part sets at commit
• timezone consistency schedule → manifest → execution; manifest forward/backward JSON compatibility
• concurrency: schedule-time parts snapshot under lockParts, commit under readLockParts, immutable minmax_idx
• no sensitive-data leakage in new messages
• dismissed candidate: Field::safeGet<Int64> accepts UInt64 literals, so integer literals in partition keys parse fine

Assumptions / limits:

• Static reasoning only; no runtime execution.
• Pre-existing and unchanged by this PR: verifyExportSchemaCastable maps columns positionally while all partition checks and the hive write path match by name — coincidentally-named but positionally-different columns can validate against the wrong source column. The identical-AST fast path also does not compare column types/timezones.
• Timezone monotonicity relies on the same DST assumptions as partition pruning.

[arthurpassos]

Hey @arthurpassos - separate from the positional/name column-matching issue. I want to confirm the intended behaviour here before I pin it in a test.

What happens

A destination with an extra column is rejected on column count:

CREATE TABLE src (id Int64, a Int32)
ENGINE = ReplicatedMergeTree('/clickhouse/tables/shard0/src', '{replica}')
ORDER BY tuple() PARTITION BY a;

CREATE TABLE dst (id Int64, a Int32, b Int32)
ENGINE = S3(..., format='Parquet', partition_strategy='hive') PARTITION BY a;

INSERT INTO src VALUES (1, 42);

ALTER TABLE src EXPORT PARTITION ID '42' TO TABLE dst;
-- Code: 20. Number of columns doesn't match (source: 2 and result: 3).

That one seems right to me.

But giving the extra column a DEFAULT changes nothing:

CREATE TABLE dst (id Int64, a Int32, b Int32 DEFAULT 42)
ENGINE = S3(..., format='Parquet', partition_strategy='hive') PARTITION BY a;

ALTER TABLE src EXPORT PARTITION ID '42' TO TABLE dst;
-- Code: 20. Number of columns doesn't match (source: 2 and result: 3).

Same error, same counts - the DEFAULT is not taken into account.

What I'd expect

I'd expect the second case to succeed, with b filled from its default:

SELECT id, a, b FROM dst;
-- 1  42  42

which is what

INSERT INTO dst (id, a)
SELECT id, a
FROM src;

would produce.

A DEFAULT column is exactly the case where the destination can supply the value itself, so requiring the source to provide it seems off.

Is the current behaviour intended? There's no data-loss risk either way since it's a loud rejection - I just want the test to assert the intended behaviour rather than whichever one I guess at.

I think this deserves a separate issue

[arthurpassos]

I suggest adding setting to turn on/off this functionality.

Export partition is already experimental and back by a setting the user must opt in

[arthurpassos]

Can you check #2138 - this does seem like we should not be allowing the export here or have some other fix in this case.

it is not related to this PR, but I did push a fix: 3e60b54

[arthurpassos]

@Selfeer I would appreciate if you could "humanize" a bit more those AI reports. For example, the following issue is very hard to understand and doesn't explain what is happening: 1. 🔴 High — Argument order lost, monotonicity proof evaluates the wrong expression.

It says "anchor" pointing to some code location. Then it says the impact. Then the trigger. It is too much to read to understand the real problem. Brain energy required to process this is very high.

Instead, if you could understand the issue yourself first and then give me a few SQL instructions that repro the case and a human explanation of what's going on, that would be 100 times better.

[Selfeer]

@Selfeer I would appreciate if you could "humanize" a bit more those AI reports. For example, the following issue is very hard to understand and doesn't explain what is happening: 1. 🔴 High — Argument order lost, monotonicity proof evaluates the wrong expression.

It says "anchor" pointing to some code location. Then it says the impact. Then the trigger. It is too much to read to understand the real problem. Brain energy required to process this is very high.

Instead, if you could understand the issue yourself first and then give me a few SQL instructions that repro the case and a human explanation of what's going on, that would be 100 times better.

I agree. I'd much rather do it the way you described: perform my own investigation on the findings and raise issues as needed-and we actually do that. But even in that case, I would still have to post this exact message first.

The purpose of the audit review has always been to perform a quick review of the PR without running any tests first, share the findings with the developer, and let you decide whether they are actual issues. If they are, we then raise separate issues afterward.

I can update the skills we use for the audit review to make the output easier to read, but overall, the audit review has always been a separate part of the verification process, separate from our actual testing.

[arthurpassos]

@k-morozov hi, I have made some refactorings, fixed conflicts and updated the docs. Could you please re-review it?

[arthurpassos]

@Selfeer regarding the 5 AI findings you posted

1. Should be fixed
2. Seems like AI gibberish. At the very least, not related to this pr
3. Should be fixed
4. Outdated, that code path does not exist anymore
5. Not related to this pr