Backport of #100420/#100295 - Resolve problems with paths and compatibility problems with Spark in Azure (v2)#1785
Conversation
…_spark_azure_fixes Resolve problems with paths and compatibility problems with Spark in Azure (v2)
![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: 9762404674
ℹ️ 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".
| per_file_record_counts.push_back(static_cast<Int64>(*current_file_num_rows)); | ||
| per_file_byte_sizes.push_back(static_cast<Int64>(file_bytes)); | ||
| /// todo arthur fix the wrong counter for file bytes, probably by backporting something else | ||
| per_file_byte_sizes.push_back(static_cast<Int64>(buffer_bytes)); |
There was a problem hiding this comment.
When the write buffer does not report bytes (the Azure path handled just above), buffer_bytes remains 0, so per_file_byte_sizes records 0 even though total_bytes was corrected from object metadata. This value is returned by getDataFileEntries and written to the import/export sidecar, which later becomes the Iceberg manifest file_size_in_bytes, so exported/imported Azure data files can be committed with a zero file size. Use the fallback size for both counters instead of only total_bytes.
Useful? React with 👍 / 👎.
…hunk` In `addRequestedFileLikeStorageVirtualsToChunk`, the `_row_number` handling block uses `return` instead of `continue` after adding the column to the chunk. This causes the function to exit the loop early, skipping any remaining virtual columns (e.g. `_data_lake_snapshot_version`). When a query requests both `_row_number` and another virtual column after it, the chunk has fewer columns than expected, resulting in: "Invalid number of columns in chunk pushed to OutputPort." The fix was originally in ClickHouse#100116 but was lost during merge because ClickHouse#100208 (revert of ClickHouse#99163) had reintroduced the `return` on master after the fix branch had already resolved it via a different code structure. The regression test `04050_iceberg_virtual_columns_return_bug` is already on master from ClickHouse#100116. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100177&sha=f10711f066fd101124e088ce33061de51ebae0e9&name_0=PR&name_1=Stateless%20tests%20%28amd_debug%2C%20parallel%29 ClickHouse#87890 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
arthurpassos
changed the title
|
Looks like tests |
| per_file_record_counts.push_back(static_cast<Int64>(*current_file_num_rows)); | ||
| per_file_byte_sizes.push_back(static_cast<Int64>(file_bytes)); | ||
| /// todo arthur fix the wrong counter for file bytes, probably by backporting something else | ||
| per_file_byte_sizes.push_back(static_cast<Int64>(total_bytes)); |
There was a problem hiding this comment.
Here is different behavior.
Before only new delta was pushed into per_file_byte_sizes, now - full summa.
Different, if it is not a first iteration and total_bytes was not zero.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1576bb8cd2
ℹ️ 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".
| { | ||
| String catalog_filename = metadata_path.serialize(); | ||
| if (!catalog_filename.starts_with(blob_storage_type_name)) | ||
| catalog_filename = blob_storage_type_name + "://" + blob_storage_namespace_name + "/" + catalog_filename; |
There was a problem hiding this comment.
For catalog-backed S3 tables created with the default write_full_path_in_iceberg_metadata = 0, the metadata location is now normalized to start with /, so FileNamesGenerator produces paths like /warehouse/db/table/metadata/v2.metadata.json. This branch then prepends s3://<bucket>/ without removing that leading slash, updating Glue to s3://<bucket>//warehouse/... while the file was actually written via resolve to warehouse/...; the catalog will point at a non-existent metadata object after the first insert.
Useful? React with 👍 / 👎.
|
@il9ue Hi Daniel. Can you please fix the remaining conflicts? You can identify them by the build failures, they are related to path resolution x IcebergMetadata::truncate. I believe you are the best person to do that as you have more context. |
|
Alright, let me take a look closely. |
The path resolution refactor in ClickHouse#100420/ClickHouse#100295 changed `FileNamesGenerator`'s constructor signature (5 args → 4 args, with `is_transactional` mapped to `use_uuid_in_metadata` and `config_path` removed) and removed `generateMetadataName()` in favour of `generateMetadataPathWithInfo()`. The `generateManifestList` signature also changed: first arg is now `IcebergPathResolver` instead of `FileNamesGenerator`, and manifest entry sizes is `std::vector<Int64>` instead of a scalar. Adapt the `TRUNCATE TABLE` code path (from #1655) to the new API: - Two-branch `FileNamesGenerator` construction (transactional vs. non-transactional, reading location from JSON) → single construction using `persistent_components.path_resolver.getTableLocation()` with `is_transactional` as `use_uuid_in_metadata`. - `generateMetadataName()` → `generateMetadataPathWithInfo()`, returning `GeneratedMetadataFileWithInfo { path, version, compression_method }`. - Storage paths derived via `path_resolver.resolve(path)` where raw strings were previously returned. - Catalog filename via `path_resolver.resolveForCatalog(path)`, replacing the manual `location + metadata_name` concatenation. - `generateManifestList(filename_generator, ...)` → `generateManifestList(path_resolver, ...)`. Behavior of truncate is preserved: clears data files by writing a new snapshot with empty manifests and updates the catalog pointer. The new `IcebergPathResolver` abstraction handles both transactional (full URI) and non-transactional (bare path) cases transparently. Refs: #1785, #1655
|
@arthurpassos
|
04034_iceberg_spark_style_location is failing. Could it be related? |
It's related. Test passed on antalya-26.3 and 100% failed on this branch, not flaky. |
|
@arthurpassos @il9ue
I don't know where is the bug - in PR, in test, or need to port some other PR too. |
Thanks for the investigation. I don't have time to look at it today, but I will try to investigate it as well tomorrow |
|
|
|
AI audit note: This review comment was generated by AI (gpt-5.3-codex). Audit update for PR #1785 ( Confirmed defects: High: Malformed catalog metadata location for pre-qualified Azure-style URIs String resolveForCatalog(const IcebergPathFromMetadata & metadata_path) const
{
String catalog_filename = metadata_path.serialize();
if (!catalog_filename.starts_with(blob_storage_type_name))
catalog_filename = blob_storage_type_name + "://" + blob_storage_namespace_name + "/" + catalog_filename;
return catalog_filename;
}Fix direction (short): In Coverage summary: Scope reviewed: End-to-end changed call graph in Iceberg metadata/write paths ( |
QA Verification — PR #1785 (Antalya 26.3 Iceberg path handling)Verdict✅ Approve from QA. The PR achieves its scope (Iceberg path handling, Azure file-size fallback, Spark CI triageFailures justificationStateless and Integration testsFrom
Regression testsFailing regression jobs in this PR run:
Tests to validate the functionalityNew / edited tests in the PRAll new and updated tests introduced by this PR passed across the relevant integration lanes (
Regression coverage in clickhouse-regression
ConclusionThe PR's correctness is established by the combination of three independent sources of evidence:
Taken together — direct cross-engine coverage of each change vector + no collateral on existing suites + corroborating xfail flip — the PR meets its objective (path consistency, Azure compatibility, Spark interop) without introducing regression. ✅ |
8 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
This PR addresses several issues: fixes inconsistent path handling in Iceberg caused by mixed usage of storage paths and metadata paths; enforces that Iceberg tables write down a table location which is either a URL or an absolute path; adds a fallback for counting file sizes in Azure because some ClickHouse readers don't support byte counting after traversal; version-hint.txt is now handled in a manner compatible with Spark; introduces type-level abstractions that make it harder to mix up path types in the future; adds tests for Azure and Local that verify cross-engine interoperability without intermediate uploading/downloading; fixes usage of position deletes, which previously relied on path inference heuristics where that approach is inappropriate
Backport of ClickHouse#100420 and ClickHouse#100295
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: