Antalya 26.3 Backport of #99935 - Support more flexible data paths validation in iceberg

[mkmkme]

Note for reviewer

While applying this backport, I faced conflicts related to the fact that upstream introduced IcebergPath.cpp in 755a11b. I found the old place of the same code in Utils.cpp and fixed it there instead.

Also, with just these changes, stateless test 04033 was still failing, so I had to apply an additional check in getProperFilePathFromMetadataInfo which is aligned with the state of IcebergPath.cpp in the upstream.

All the added tests are passing locally for me.

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Now ClickHouse should properly handle spark-style tables (where we have full absolute path for each file or relative path to common table path) (ClickHouse#99935 by @alesapin)

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All with Aarch64
• 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)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [c8f3d6c]

[ianton-ru]

As I see this change is from ClickHouse#100420.
Do we want to backport this huge PR too?

[alsugiliazova]

Verification: PR #1769

Title: Antalya 26.3 Backport of ClickHouse#99935 — Support more flexible data paths validation in Iceberg
Base: antalya-26.3 ← backports/antalya-26.3/99935
Head: a879f09795f351cdf22af4a3e4ce7b9d5281ea23
Size: +143 / -3 across 7 files · State: OPEN · Mergeable

Changes

Production fix (1 file): src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp

• getMetadataFileAndVersion() — replaces unchecked find_first_of calls (which returned npos on garbage names and caused std::length_error) with explicit bounds checks; throws BAD_ARGUMENTS with a clear message for malformed filenames.
• getProperFilePathFromMetadataInfo() — tightens the prefix-match branch to require a real / boundary and adds an explicit BAD_ARGUMENTS guard (avoids unsigned underflow / std::out_of_range when data_path is shorter than the table-location prefix). This is the actual “support flexible data paths” fix for Spark-style table relocations.

New tests (3 stateless tests, 6 files):

• 04033_iceberg_mismatched_location.sh — reproduces the previous std::out_of_range from IcebergPathResolver::resolve when metadata location is much longer than the data path; expects clean BAD_ARGUMENTS.
• 04034_iceberg_spark_style_location.sh — positive test for Spark-style different-scheme/bucket location (s3a://spark-bucket/...); ClickHouse must strip the Spark table-location prefix and read the parquet correctly (SELECT returns 42).
• 04061_iceberg_bad_metadata_file_name.sh — fuzzer regression: garbage iceberg_metadata_file_path = '.*' previously hit std::length_error; now returns BAD_ARGUMENTS.

PR-added tests — all GREEN

3 tests × 6 stateless jobs each = 18 OK runs, 0 failures. (1 SKIPPED per test in Fast test, as expected via Tags: no-fasttest.)

Test	Stateless jobs runs	Status
04033_iceberg_mismatched_location	6	all OK
04034_iceberg_spark_style_location	6	all OK
04061_iceberg_bad_metadata_file_name	6	all OK

Jobs covered: amd_asan distributed-plan parallel, amd_debug distributed-plan s3 storage parallel, amd_debug parallel, arm_asan azure parallel, arm_asan targeted, arm_binary parallel.

Both error paths (BAD_ARGUMENTS on bad metadata file name + mismatched location) and the positive Spark-style path resolution have clean coverage.

CI overview (head commit)

• PR test workflow: 43 success / 51 skipped / 6 failure (job-level), but only 1 real test FAIL row in DB.
• Regression workflow: 30 success / 66 skipped / 4 failure (chronic baseline).
• One pending action_required job (queue/auth, not a real failure).

PR test-workflow failures

Check	Verdict
Stateless tests (arm_asan, azure, sequential, 2/2) — 03443_shared_storage_snapshots	Pre-existing flake on antalya-26.3. 29 fails / 17 PRs in last 30 days. Unrelated to this PR (no Iceberg/path code involved).
Stateless tests (amd_debug, distributed plan, s3 storage, sequential)	Job-level error — 0 test rows recorded in DB for this job. Infra/run-level failure, not a test regression.

Regression-workflow failures (chronic baseline on antalya-26.3)

Suite	Fails
Swarms (Aarch64 + Release)	226
Parquet (Aarch64 + Release)	34
S3Export partition (Aarch64 + Release)	20
S3Export part (Aarch64 + Release)	16

Same fingerprint as sibling antalya-26.3 PRs (1770, 1767, 1762, 1759, 1748, …). No new failure modes.

Caveat — partial frontport

This PR lands on antalya-26.3 while companion features from antalya-26.1 are still being frontported in parallel. Some regression suites still rely on settings/CLI args not yet wired in this base. Final re-verify is recommended once the rest of the bundle lands.

Verdict

Safe to merge.

• All 3 newly added stateless tests pass 100% (18/18 stateless runs), covering both the new BAD_ARGUMENTS guards and the positive Spark-style path resolution.
• Only test-level failure is 03443_shared_storage_snapshots, a well-documented pre-existing flake.
• The other 5 GitHub-level red checks are job-level/infra errors and the recurring antalya-26.3 chronic baseline.

[alsugiliazova]

Audit: PR #1769 — Antalya 26.3 Backport of #99935 — Support more flexible data paths validation in iceberg

AI audit note: This review comment was generated by AI (Cursor agent, audit-review skill).

Scope reviewed

Single-commit backport (PR head a879f09) of upstream ClickHouse/ClickHouse#99935. Author note states the backport was adapted from upstream's IcebergPath.cpp (introduced in upstream after the Antalya fork) by applying the changes in src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp instead, plus an extra if (data_path.size() < prefix.size()) throw … guard aligned with IcebergPath.cpp.

Changes

1. Utils.cpp::getMetadataFileAndVersion — rewrite the two filename branches with explicit npos / degenerate-position guards (test 04061).
2. Utils.cpp::getProperFilePathFromMetadataInfo — replace the second predicate table_location.ends_with(common_path) with a child-boundary check (data_path.size() == table_location.size() || data_path[table_location.size()] == '/'), and add a size guard before data_path.substr(prefix.size()) in the “different directories” branch (tests 04033, 04034).
3. New stateless tests 04033_iceberg_mismatched_location.{sh,reference}, 04034_iceberg_spark_style_location.{sh,reference}, 04061_iceberg_bad_metadata_file_name.{sh,reference} — fuzzer & Spark-style location regressions.

Call graph (in scope)

Step	Location
Latest metadata pick	Iceberg::getLatestMetadataFileAndVersion / iceberg_metadata_file_path / version-hint paths → getMetadataFileAndVersion(path)
Filename → version	getMetadataFileAndVersion parses the leading numeric portion of the filename
Path resolution from manifest entries	getProperFilePathFromMetadataInfo(data_path, common_path, table_location) (called by Iceberg readers when manifest entries hold absolute / cross-scheme paths)
Filename emission	Iceberg::FileNamesGenerator::generateMetadataName — two formats depending on use_uuid_in_metadata (constructor-bound to catalog != nullptr && catalog->isTransactional()): vN.metadata.json and vN-<uuid>.metadata.json

Transition matrix

Stage	Pre-fix	Post-fix
Parse vN.metadata.json	find_first_of(".-") → version = "N"	find_first_of('.') → version = "N" (✓)
Parse vN-<uuid>.metadata.json (the form Antalya FileNamesGenerator emits when the catalog is transactional, line 116 of FileNamesGenerator.cpp: fmt::format("{}v{}-{}{}.metadata.json", …))	find_first_of(".-") finds - after N → version = "N" (✓)	find_first_of('.') finds the dot before metadata → version_str = "N-<uuid>" → fails std::all_of(isdigit) → throws BAD_ARGUMENTS ❌
Parse N-<uuid>.metadata.json (no v prefix)	find_first_of('-') → version = "N"	same; with new npos / dash_pos == 0 guards (✓)
Parse fuzzer input '.*'	find_first_of(".-") finds . at 0 → String(begin, begin) empty → fails isdigit (BAD_ARGUMENTS) — actually safe by coincidence; but '-*' and similar would underflow	dot_pos == 0 ⇒ <= 1 ⇒ explicit BAD_ARGUMENTS (✓ improvement)
getProperFilePathFromMetadataInfo first branch — sibling-prefix data_path = ".../tableX/...", table_location = ".../table"	starts_with true + accidental ends_with(common_path) could spuriously strip tableX portion	New child-boundary check requires next char to be / or end-of-string (✓ improvement)
getProperFilePathFromMetadataInfo second branch — data_path.size() < prefix.size()	data_path.substr(prefix.size()) throws std::out_of_range	Explicit BAD_ARGUMENTS with diagnostic context (✓ improvement)

Confirmed defects

1. getMetadataFileAndVersion no longer parses Antalya vN-<uuid>.metadata.json files (the format Antalya itself writes under transactional catalogs)

Severity: High — Iceberg tables managed by transactional catalogs (REST / Glue / etc.) become unreadable after this PR; every read or follow-up write hits the new getMetadataFileAndVersion path and throws BAD_ARGUMENTS: Bad metadata file name: 'vN-<uuid>.metadata.json'. Expected vN.metadata.json or N-<uuid>.metadata.json …. This silently regresses Altinity/ClickHouse#1640, which had explicitly added the dash to the v-branch search set (see tmp/pr1640.diff):

-        version_str = String(file_name.begin() + 1, file_name.begin() + file_name.find_first_of('.'));
+        version_str = String(file_name.begin() + 1, file_name.begin() + file_name.find_first_of(".-"));

PR #1769 reverts this to find_first_of('.') (without the dash), even though the very same Utils.cpp keeps the doc comment that describes the format the new code can no longer parse:

    /// v<V>.metadata.json
    /// v<V>-<random-uuid>.metadata.json - generated by FileNamesGenerator::generateMetadataName with use_uuid_in_metadata flag
    if (file_name.starts_with('v'))
        version_str = String(file_name.begin() + 1, file_name.begin() + file_name.find_first_of(".-"));

Confirmed emission path in this tree (uuid branch always includes the leading v):

    else
    {
        auto uuid_str = uuid_generator.createRandom().toString();
        auto res = Result{
            .path_in_metadata = fmt::format("{}v{}-{}{}.metadata.json", metadata_dir, initial_version, uuid_str, compression_suffix),
            .path_in_storage = fmt::format("{}v{}-{}{}.metadata.json", storage_metadata_dir, initial_version, uuid_str, compression_suffix),
        };
        initial_version++;
        return res;
    }

use_uuid_in_metadata is wired to catalog != nullptr && catalog->isTransactional() at every Iceberg FileNamesGenerator(…) call site (IcebergWrites.cpp ×2, IcebergMetadata.cpp ×2). Trigger: any Iceberg write or follow-up read against a transactional catalog (REST / Iceberg-Glue / etc.) on antalya-26.3 once this PR lands. Post-fix find_first_of('.') returns the dot before metadata, so version_str becomes "N-<uuid>" and fails the next-line std::all_of(isdigit) check.

• Impact: Iceberg tables with transactional catalogs and uuid-suffixed metadata become unreadable; CTAS/INSERT subsequent reads, iceberg_metadata_file_path overrides, and version-hint resolutions all throw BAD_ARGUMENTS.
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp::getMetadataFileAndVersion, v-branch.
• Trigger: any SELECT/INSERT/ALTER on an Iceberg table whose latest metadata filename matches vN-<uuid>.metadata.json (the only form a transactional-catalog session of this tree writes).
• Why defect: behavior contradicts the function's own doc comment listing v<V>-<random-uuid>.metadata.json as a valid input; reverts the explicit fix from #1640; reduces the accepted-filename set vs the pre-PR tree.
• Fix direction (short): restore the dash in the v-branch search set — auto delim_pos = file_name.find_first_of(".-"); if (delim_pos == String::npos || delim_pos <= 1) throw …; version_str = String(file_name.begin() + 1, file_name.begin() + delim_pos); (and update the BAD_ARGUMENTS message to also list vN-<uuid>.metadata.json). Alternatively, change FileNamesGenerator::generateMetadataName to drop the leading v in the uuid branch, but that breaks compatibility with already-written tables.
• Regression test direction (short): stateless test that exercises an Iceberg write via a transactional catalog (or a synthetic vN-<uuid>.metadata.json file constructed in S3) and asserts a clean SELECT round-trip.

Other categories (no defect)

Category	Outcome
Old std::length_error on find_first_of(...) == npos (e.g. fuzzer input '.*')	Fixed for both branches with explicit npos/degenerate-index throws
Sibling-prefix false match in first branch of getProperFilePathFromMetadataInfo (table vs tableX)	Fixed by child-boundary check
Second-branch std::out_of_range when data_path.size() < prefix.size()	Fixed by explicit BAD_ARGUMENTS with diagnostic
Spark-style absolute-path tables (different scheme/bucket in location)	Now works (test 04034)
Catalog vs filesystem-only Iceberg	No new code path beyond the listed branches
Lifetime / iterator invalidation / data races	None — pure string parsing
Exception safety	Only throws added; no resource acquired before the new guards

C++ bug classes (delta)

Class	Assessment
std::out_of_range / std::length_error via iterator + npos	Fixed by explicit npos guards
Sibling-prefix accidental match	Fixed by child-boundary check
Iterator / reference invalidation	None
Data races, deadlocks	None
UB / integer wraparound (size_t underflow in prefix.size() arithmetic)	Fixed for the second branch via explicit size guard
Filename-format regression introduced by reverting #1640	See defect above

Test review

Test	Coverage
04033_iceberg_mismatched_location.sh	Forces table location to a much longer path than the manifest-list entries; pre-fix → std::out_of_range; post-fix → BAD_ARGUMENTS
04034_iceberg_spark_style_location.sh	Spark-style metadata where location = s3a://spark-bucket/… and manifest entries use the full Spark prefix; positive end-to-end SELECT
04061_iceberg_bad_metadata_file_name.sh	Fuzzer input iceberg_metadata_file_path = '.*' → BAD_ARGUMENTS instead of std::length_error
Missing coverage	No test for the vN-<uuid>.metadata.json filename produced by this branch's own FileNamesGenerator under a transactional catalog — that's why the regression slips through

---

Audit update for PR #1769 (Iceberg path validation backport)

Confirmed defects

High: getMetadataFileAndVersion regression — rejects Antalya's own vN-<uuid>.metadata.json metadata filenames after this PR

• Impact: Iceberg tables that use a transactional catalog (REST / Glue) become unreadable; all subsequent SELECT/INSERT/ALTER throw BAD_ARGUMENTS.
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp::getMetadataFileAndVersion, v-prefix branch.
• Trigger: any Iceberg operation against a tree that wrote metadata as vN-<uuid>.metadata.json (the format FileNamesGenerator::generateMetadataName emits whenever catalog != nullptr && catalog->isTransactional()).
• Why defect: silently reverts the dash-aware search set added by #1640 while keeping the doc comment that claims the format is supported; the new BAD_ARGUMENTS error message also omits vN-<uuid>.metadata.json from the list of accepted forms.
• Fix direction: in the v-branch, use find_first_of(".-") (or split into v-then-N-then-./- logic) and extend the error message to include all three accepted forms.
• Regression test direction: a stateless test that constructs (or causes a write of) a vN-<uuid>.metadata.json file and asserts that getMetadataFileAndVersion and a follow-up SELECT succeed.

Coverage summary

Item	Detail
Scope reviewed	The two Utils.cpp hunks (filename parser, path resolver), three new stateless tests, the catalog/FileNamesGenerator emission paths, and the upstream IcebergPath.cpp::resolve the author cited as the reference.
Categories failed	Filename-format compatibility regression (high).
Categories passed	npos hardening in both branches of getMetadataFileAndVersion; sibling-prefix tightening and size-guard in getProperFilePathFromMetadataInfo; Spark-style positive case; non-introduction of new lifetime / race / UB; tests added for the cases that motivated the PR.
Assumptions / limits	Static audit. Upstream master itself emits vN-<uuid>.metadata.json from FileNamesGenerator::generateMetadataPathWithInfo, so this audit's defect description may also apply upstream; that is out of scope for the Antalya backport, but worth flagging upstream separately. Reviewer's already-raised concern that the size-guard hunk overlaps with upstream #100420 is a scope question, not a correctness defect.

---

References

• PR Antalya 26.3 Backport of #99935 - Support more flexible data paths validation in iceberg #1769: Antalya 26.3 Backport of #99935 - Support more flexible data paths validation in iceberg #1769
• PR head commit: a879f09
• Upstream: Support more flexible data paths validation in iceberg ClickHouse/ClickHouse#99935
• Earlier Antalya fix that PR Antalya 26.3 Backport of #99935 - Support more flexible data paths validation in iceberg #1769 inadvertently reverts: 26.3 Antalya port - Alternative syntax for cluster functions #1640
• Reviewer's note about overlap with upstream Resolve problems with paths and compatibility problems with Spark in Azure (v2) ClickHouse/ClickHouse#100420: Resolve problems with paths and compatibility problems with Spark in Azure (v2) ClickHouse/ClickHouse#100420
• Upstream issue motivating the original fix: Iceberg std::out_of_range ClickHouse/ClickHouse#92348

[mkmkme]

@alsugiliazova I've fixed the regression. Could you verify one more time please?
@ianton-ru could you have a look at the additional commit I've pushed?

Thanks!

[alsugiliazova]

Re-audit after developer fix — commit c8f3d6c

AI audit note: This re-audit was generated by AI (Cursor agent, audit-review skill) after the developer pushed a follow-up commit that addresses the High-severity finding above.

Scope of the re-audit

Single follow-up commit c8f3d6c "fix the vN-.metadata.json regression in getMetadataFileAndVersion", touching only src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp (+6/-6). No other files changed; the three stateless tests from a879f09 remain in place.

Updated transition matrix for getMetadataFileAndVersion v-branch

Input filename	delim_pos	version_str	isdigit	Outcome
v1.metadata.json	2 (.)	"1"	pass	OK
v123.metadata.json	4 (.)	"123"	pass	OK
v1-<uuid>.metadata.json	2 (-)	"1"	pass	OK (defect fixed)
v123-<uuid>.metadata.json	4 (-)	"123"	pass	OK (defect fixed)
v.json	1 (.)	—	—	BAD_ARGUMENTS via delim_pos <= 1
v-uuid.metadata.json	1 (-)	—	—	BAD_ARGUMENTS via delim_pos <= 1
vfoo (no . or -)	npos	—	—	BAD_ARGUMENTS via npos
vABC.metadata.json	4 (.)	"ABC"	fail	BAD_ARGUMENTS via isdigit
'.*' (fuzzer)	—	—	—	rejected earlier by isTemporaryMetadataFile / starts_with('v') not matched, then else branch's dash_pos == npos → BAD_ARGUMENTS

All previously broken cases now succeed; all previously rejected malformed cases continue to throw BAD_ARGUMENTS (no narrowing of guards). The npos/degenerate-position hardening from the first commit is preserved.

C++ bug classes (delta after the fix)

Class	Before fix commit	After fix commit
Filename-format regression — Antalya transactional-catalog vN-<uuid>.metadata.json rejected	High defect	Resolved — find_first_of(".-") restored, guard delim_pos <= 1 still rejects v./v- degenerate cases
BAD_ARGUMENTS message lists all accepted forms	Inaccurate (only two of three)	Accurate (all three: vN, vN-<uuid>, N-<uuid>)
std::out_of_range / std::length_error via iterator + npos	Fixed	Fixed (unchanged)
Sibling-prefix accidental match in getProperFilePathFromMetadataInfo	Fixed	Fixed (unchanged)
size_t underflow in second branch of getProperFilePathFromMetadataInfo	Fixed	Fixed (unchanged)
Iterator / reference invalidation, data races, deadlocks	None	None

Test review delta

Test	Status
04033_iceberg_mismatched_location.sh	unchanged — still asserts BAD_ARGUMENTS for short data_path vs long table_location
04034_iceberg_spark_style_location.sh	unchanged — still asserts positive Spark-style absolute-path SELECT
04061_iceberg_bad_metadata_file_name.sh	unchanged — still asserts BAD_ARGUMENTS for iceberg_metadata_file_path = '.*'
New regression test for vN-<uuid>.metadata.json	Still missing — the fix commit modifies only Utils.cpp; no stateless or integration test asserts a clean round-trip for the filename form Antalya itself emits under transactional catalogs. Recommend adding such a test (e.g. construct a synthetic v2-<uuid>.metadata.json in S3 and assert SELECT succeeds, or wire an IcebergCatalog integration test against the existing REST mock with a transactional configuration).

Coverage summary (re-audit)

Item	Detail
Scope reviewed	The single Utils.cpp hunk in commit c8f3d6c, plus re-verification that no test files were added/removed alongside it.
Categories failed	None (the previously High-severity filename-format regression is resolved).
Categories passed	Filename parsing for all three documented forms; npos/degenerate-position hardening; error-message completeness; non-introduction of new lifetime / race / UB / exception-safety issues.
Assumptions / limits	Static audit. Runtime verification (e.g. exercising a transactional catalog write that emits vN-<uuid>.metadata.json and asserting a subsequent SELECT succeeds) is recommended before merge, and would close the "missing coverage" item above. Upstream IcebergPath.cpp was not re-checked in this re-audit — if the same regression exists upstream it is out of scope for the Antalya backport but worth a separate report.

Verdict

The previously identified High-severity defect (Antalya vN-<uuid>.metadata.json rejected by getMetadataFileAndVersion) is fixed by commit c8f3d6c. No new defects were introduced. The only remaining audit item is a non-blocking test-coverage gap: there is no regression test for the vN-<uuid>.metadata.json filename form. The PR is otherwise safe to merge from a correctness perspective.

Additional references

• Fix commit: c8f3d6c

[alsugiliazova]

Verification: PR #1769 (re-verified after latest commit)

PR-added tests — all GREEN on new head

3 tests × 6 stateless jobs each = 18 OK runs, 0 failures. (1 SKIPPED per test in Fast test, as expected via Tags: no-fasttest.)

Test	Stateless runs	Status
04033_iceberg_mismatched_location	6	all OK
04034_iceberg_spark_style_location	6	all OK
04061_iceberg_bad_metadata_file_name	6	all OK

Jobs covered: amd_asan distributed-plan parallel, amd_debug distributed-plan s3 storage parallel, amd_debug parallel, arm_asan azure parallel, arm_asan targeted, arm_binary parallel.

The new vN-<uuid> parse path is implicitly exercised by every existing read-from-Iceberg stateless test that runs on this head (the regression that broke the previous CI is gone).

CI overview (latest head)

• PR test workflow: 48 success / 48 skipped / 0 failure — fully GREEN at the test workflow level.
• Regression workflow: 28 success / 68 skipped / 4 failure (chronic antalya-26.3 baseline).
• One pending action_required job (queue/auth).

Test-level failures in DB

Zero. No test_status='FAIL' rows on this commit. The previously seen 03443_shared_storage_snapshots flake and the amd_debug distributed-plan sequential job-level error are no longer present on this run.

Regression-workflow failures (chronic baseline on antalya-26.3)

Suite	Result
Swarms (Aarch64 + Release)	chronic baseline
Parquet (Aarch64 + Release)	chronic baseline
S3Export partition (Aarch64 + Release)	chronic baseline
S3Export part (Aarch64 + Release)	chronic baseline

Verdict

Safe to merge — improved since the previous verification.