Repository navigation
[improvement](fe) Upgrade iceberg to 1.12.0 - #69031
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
laskoviymishka
force-pushed
the
iceberg-1.12
branch
from
October 13, 2026 13:12
63ef8b5 to
78c1dfa
Compare
laskoviymishka
marked this pull request as ready for review
October 13, 2026 13:12
laskoviymishka
requested review from
924060929,
CalvinKirs and
morningman
as code owners
October 13, 2026 13:12
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Upgrade the Iceberg Java SDK from 1.11.0 to 1.12.0. 1.12.0 brings the client side of REST catalog labels (`Labels`, `SupportsLabels`, labels on `LoadTableResponse`, see apache/iceberg#15750, apache#18045, apache#18046), which Doris needs before it can expose catalog-provided labels for iceberg tables. This change only upgrades the dependency and adapts Doris to it. - Bump `iceberg.version` 1.11.0 -> 1.12.0, and `avro.version` 1.12.1 -> 1.12.2 to stay consistent with what iceberg 1.12.0 is built against (the pom requires the two to move together). - Bump the explicit caffeine pin in fe-connector-iceberg 2.9.3 -> 3.2.4. Iceberg 1.12.0 moved to caffeine 3. The pin is a direct dependency, so it wins Maven mediation; leaving it at 2.9.3 would package an older caffeine into the iceberg plugin than iceberg-core is built against. - Re-vendor `org.apache.iceberg.DeleteFileIndex` from 1.12.0 and re-apply the Doris changes (public class/builder methods, schemasById comment, equality-field-id sets). The vendored class shadows the SDK class in the same package, and 1.12.0 changed it (an extra `specsById` constructor argument and delete/data file partition-match validation), so keeping the 1.11.0 copy would break the SDK's own callers at runtime. Since Doris passes `specsById` to the builder, the cached scan planning path now also rejects delete files whose partition spec or tuple does not match the data file, the same as upstream iceberg. - Pass `table.specs()` to `ManifestFiles.read` in `IcebergManifestCache`. Iceberg 1.12.0 removed the deprecated `read(ManifestFile, FileIO)` overload, which read the partition specs from the manifest file's own metadata (only possible for Avro manifests). This matches the delete manifest path next to it (`readDeleteManifest(..., table.specs())`) and `IcebergRemoveOrphanFilesAction`. - Keep the AWS SDK at 2.29.52 and default iceberg-aws's HTTP client to `urlconnection`. In 1.12.0, iceberg-aws's default `http-client.type` (`apache`) builds an `Apache5HttpClient` from `software.amazon.awssdk:apache5-client`, which first shipped in AWS SDK 2.32. Doris pins 2.29.52 on purpose (apache#58566: newer SDKs add default S3 integrity checksums that break S3-compatible services), so with the default every S3FileIO, Glue and STS client iceberg builds failed with `NoClassDefFoundError: software/amazon/awssdk/http/apache5/Apache5HttpClient$Builder`. `IcebergCatalogFactory.buildBaseCatalogProperties`, the base of every catalog flavor, now sets `http-client.type=urlconnection` unless the catalog sets it. S3FileIO keeps the setting when it is serialized, so the BE iceberg metadata scanner, which rebuilds the FileIO from it, uses the same client; `url-connection-client` is added to that scanner, which only bundled `apache-client` and `netty-nio-client`. An explicit `http-client.type=apache` is rejected at CREATE/ALTER CATALOG instead of failing on the first S3 or Glue call. - Report the mixed-version case clearly. FE sends iceberg system-table splits to the BE as Java-serialized iceberg objects, and iceberg 1.12.0 changed the serialized form of classes every split carries (`BaseFile`, `PartitionData`, their anonymous inner classes; 24 serializable classes changed `serialVersionUID`). Unlike 1.11.0, where only `Schema` changed and is shimmed, this cannot be bridged by remapping UIDs, so while FE and BE run different iceberg versions (a rolling upgrade) iceberg system tables cannot be read. `IcebergSerializationCompat` now turns the bare `InvalidClassException` / `ClassNotFoundException` into an error naming the cause and the BE's iceberg version, and the shim comments no longer claim a shared wire across versions. `SchemaAwareDataTableScan` (the other vendored class) needs no change: `DataTableScan`, `BaseTableScan`, `SnapshotScan` and `TableScanContext` are identical between 1.11.0 and 1.12.0. ### Release note Upgrade the Iceberg Java SDK to 1.12.0. Iceberg's AWS clients (S3FileIO, Glue, STS) now use the URLConnection HTTP client by default, and the `http-client.type=apache` catalog property is no longer accepted. During a rolling upgrade from a release on Iceberg 1.11, Iceberg system tables (`tbl$snapshots`, `tbl$files`, ...) are unavailable until FE and BE are both upgraded. ### Check List (For Author) - Test: Unit Test - Added IcebergCatalogFactoryTest#buildBaseDefaultsIcebergAwsHttpClientToUrlConnection (builds a real S3 client builder with the default) and #buildBaseKeepsAnExplicitHttpClientType, and IcebergCatalogPropertiesTest#createTimeRulesRejectTheApacheHttpClientType; IcebergCatalogOptionsSnapshotTest snapshots now include the default. - Added IcebergSerializationCompatTest#reportsATaskFromAnotherIcebergVersion (a DataFile carrying Iceberg 1.11's BaseFile UID) and #reportsAClassThisIcebergDoesNotHave. - Verified the mixed-version break outside the build: a DataFile written with iceberg 1.11.0 cannot be read with 1.12.0 and vice versa, even after rewriting every changed class UID in the stream. - `build.sh --fe` passes. Ran every iceberg FE unit test with run-fe-ut.sh (fe-connector-iceberg, fe-connector-metastore-iceberg, be-java-extensions/iceberg-metadata-scanner): 1650 tests, 0 failures, 5 skipped. - Iceberg regression suites not run. - Behavior changed: Yes. - Iceberg's AWS clients use the URLConnection HTTP client by default, so `http-client.apache.*` properties no longer apply, and `http-client.type=apache` is rejected at CREATE/ALTER CATALOG. - Iceberg scans whose delete files have a partition spec or partition tuple that does not match the data file now fail with a ValidationException (iceberg 1.12.0 behavior) instead of applying the deletes. - While FE and BE run different iceberg versions (rolling upgrade from iceberg 1.11), iceberg system tables fail with an explicit "different Iceberg version" error until both sides are upgraded. - Does this need documentation: No
laskoviymishka
force-pushed
the
iceberg-1.12
branch
from
October 13, 2026 14:02
78c1dfa to
a7d5511
Compare
4 of 16 tasks
Contributor
1. [Must fix] Give the storage connectivity probe the same HTTP client defaultWhy. The default The failure was reproduced with iceberg 1.12.0 and AWS SDK 2.29.52, using exactly the probe's properties against a closed local port. Without the key the call throws Fix. Move the default into one helper and call it from both places. A better version also copies the catalog's // IcebergCatalogFactory
/**
* iceberg-aws's default "apache" client is Apache5HttpClient since iceberg 1.12, which needs AWS SDK >= 2.32;
* Doris pins 2.29.x (#58566). Every map that ends up building an iceberg AWS client must go through this.
*/
static void applyDefaultAwsHttpClient(Map<String, String> opts) {
opts.putIfAbsent(HttpClientProperties.CLIENT_TYPE, HttpClientProperties.CLIENT_TYPE_URLCONNECTION);
} // IcebergConnector.buildStorageProbeProperties
ioProps.put("s3.remote-signing-enabled", "false");
+ IcebergCatalogFactory.applyDefaultAwsHttpClient(ioProps);Add a test written the same way as the PR's own factory test, so the probe map is checked against the shipped SDK: @Test
public void storageProbePropertiesBuildAnS3ClientOnTheShippedSdk() {
Map<String, String> ioProps = IcebergConnector.buildStorageProbeProperties(
IcebergCatalogProperties.TYPE_REST,
new FakeS3CompatibleStorageProperties("S3").endpoint("http://127.0.0.1:1")
.region("us-east-1").accessKey("ak").secretKey("sk"));
// MUTATION: drop the default -> "apache" -> NoClassDefFoundError -> red.
Assertions.assertDoesNotThrow(
() -> new HttpClientProperties(ioProps).applyHttpClientConfigurations(S3Client.builder()));
}After the fix, run the iceberg cases under |
…nnectivity probe ### What problem does this PR solve? Issue Number: None Related PR: apache#69031 Problem Summary: The iceberg 1.12.0 upgrade defaults iceberg-aws's HTTP client to `urlconnection` in `IcebergCatalogFactory.buildBaseCatalogProperties`, because iceberg-aws's default `apache` type is `Apache5HttpClient` since 1.12.0 and needs an AWS SDK newer than the 2.29.52 Doris pins (apache#58566). The CREATE CATALOG storage connectivity probe does not go through that method. `IcebergConnector.buildStorageProbeProperties` builds its own map (only `s3.*` and `client.region`) and `probeStorage` loads an `S3FileIO` from it, so the probe fell back to `apache` and building the S3 client threw `NoClassDefFoundError: software/amazon/awssdk/http/apache5/Apache5HttpClient$Builder`. That is an `Error`, not caught by the probe's `catch (Exception e)`, so it never became a "connectivity test failed" result: CREATE CATALOG with static S3 credentials failed with an unexpected exception whether the credentials were right or wrong. - Move the default into `IcebergCatalogFactory.applyDefaultAwsHttpClient` and apply it in both `buildBaseCatalogProperties` and `buildStorageProbeProperties`. - Copy the catalog's `http-client.*` settings (client type, proxy, timeouts) into the probe map, so the probe reaches storage the same way the catalog does; `s3.*` still comes only from the bound storage. The other places the connector builds AWS clients do not need it: every catalog flavor (DLF included) goes through `buildBaseCatalogProperties`; the s3tables control-plane client uses the s3tables library's own `HttpClientProperties`, whose `apache` type is the v4 `ApacheHttpClient` shipped with 2.29.52; DLF's own `S3Client` sets `UrlConnectionHttpClient` explicitly; the STS assume-role client is a plain AWS SDK builder. ### Release note None ### Check List (For Author) - Test: Unit Test - Added IcebergConnectorTestConnectionTest#storageProbePropertiesBuildAnS3ClientOnTheShippedSdk (builds a real S3 client builder from the probe map on the shipped SDK) and #storageProbeUsesTheCatalogHttpClientSettings. - `build.sh --fe` passes. Ran every iceberg FE unit test with run-fe-ut.sh (fe-connector-iceberg, fe-connector-metastore-iceberg, be-java-extensions/iceberg-metadata-scanner): 1652 tests, 0 failures, 5 skipped. - The external_table_p0/test_connection regression cases were not run (no docker regression environment available locally). - Behavior changed: No (fixes a regression of the iceberg 1.12.0 upgrade in apache#69031 before it is released) - Does this need documentation: No Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review of commit 968a0f7: one P1 and two P2 findings. The shared Avro bump breaks Hudi V2 commit readers; an empty HTTP-client type selects the unsupported Apache5 transport; current-spec manifest decoding breaks cached historical streaming scans after partition type promotion. All three verified findings are inline. No prior inline threads or existing blocking findings were present. No additional user focus was supplied.
Reviewed all 15 changed files, their callers and downstream consumers, and primary released Iceberg/Avro/Hudi/AWS sources. Three reviewers covered catalogs/dependencies, scans/task serialization, and a separate AWS runtime risk review. Static-only: no builds, tests, or source changes were performed; author-reported test results were not treated as reviewer execution evidence.
Critical checkpoint conclusions:
- Goal and proof: the 1.12 dependency/API adaptations are present, but the three inline regressions prevent full compatibility. Added tests do not cover their concrete triggers.
- Scope and reuse: adaptations are focused and reuse the shared factory; the global Avro pin also affects Hudi and needs consumer-specific treatment.
- Concurrency: local option maps introduce no shared mutation; lazy scan/cache workers and comparator loading preserve existing synchronization and TCCL boundaries. No new lock-order defect found.
- Lifecycle and initialization: readers close and cached file copies survive iterator reuse; comparator caches are scan-owned. Hudi class-based datum-reader initialization fails as described in the P1 finding.
- Configuration: existing http-client.type changes default/allowed values; CREATE and ALTER validate, and ALTER reconstructs the connector. Empty input is missed. Replay retains statement-only validation bypass.
- Compatibility: vendored DeleteFileIndex matches the released 1.12 ABI plus Doris public/equality-ID extensions; Schema's serialized fields remain unchanged. Global Avro restrictions and historical partition typing produce the inline findings.
- Thrift/protobuf: no definition or field-ID changes.
- Parallel paths: checked all seven catalog flavors, CREATE probe and lazy catalog, direct DLF/S3Tables clients, SDK/cache and synchronous/streaming/count scans, JNI system tables and native position-deletes. The cache's eager fallback masks the historical error in synchronous planning but cannot catch lazy streaming evaluation.
- Conditions and errors: upstream spec/tuple validation has concrete corruption failure paths and useful file context; unsupported literal apache has an actionable error. Empty default handling is incomplete.
- Test coverage: JUnit cases cover property defaults, literal apache rejection, probe propagation and synthetic serialization failures. Missing cases include actual Hudi V2 commit decoding, empty transport, and streamed historical INT-to-LONG identity partitions. Runtime/e2e compatibility was not executed.
- Test results: no regression goldens changed. The old serialized fixture has a null metadata file and cannot demonstrate real mixed-version task compatibility.
- Observability: existing probe/cache/streaming diagnostics retain catalog/table context and causes; the new serialization error identifies version skew. No additional metric requirement identified.
- Persistence and transactions: no journal format, snapshot commit or native transaction changes; existing snapshot selection still fences data/delete enumeration. Persisted transport values require the declared migration.
- Writes and atomicity: no new write/commit/crash-recovery path; delete-index changes validate association before use and do not mutate data.
- FE-to-BE variables: no new thrift variable; existing serialized FileIO properties carry the transport, and the scanner now packages URLConnection. Native data/position-deletes payloads stay on the existing path.
- Performance and memory: option processing is linear in property count outside scan loops; comparators are cached per spec and validation is proportional to attached deletes. No new BE allocator/tracker path. Historical synchronous fallback can reread manifests.
- Other concerns: no additional independently substantiated issue after rechecks. Explicit Apache selection (including forced REST overrides) and Apache-specific timeout keys are intentional declared migration limits, rather than additional inline bugs.
Declared upgrade limits remain: many real Java-serialized system-table tasks, including $snapshots/$files, cannot cross Iceberg 1.11/1.12 FE/BE versions; normal native data scans and native $position_deletes avoid that wire. Existing explicit apache settings must become urlconnection, with server-side migration when REST overrides force apache; Apache timeout keys must become URLConnection timeout keys. These limits were checked statically and are not claims of executed rolling-upgrade coverage.
Convergence: converged after 2 rounds. All three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS in round 2; all changed-file coverage and every candidate/risk disposition are complete. No unresolved source question or uncovered source scope remains. The execution limits and declared migration limits above remain explicit.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: None Related PR: apache#69031 Problem Summary: The iceberg 1.12.0 upgrade also raised the global `avro.version` from 1.12.1 to 1.12.2, the version iceberg 1.12.0 is built against. `avro.version` is shared: it also upgrades `fe-connector-hudi` and `be-java-extensions/hadoop-hudi-scanner`. Avro 1.12.2 forbids `SpecificDatumReader` and `java-class` schema attributes from instantiating classes outside `org.apache.avro.SERIALIZABLE_CLASSES` / `SERIALIZABLE_PACKAGES` (AVRO-4189), and throws `SecurityException: Forbidden ...` by default. Hudi reads timeline metadata exactly that way: for a Hudi 1.0 table with a completed non-legacy V2 commit, a COW incremental query reaches `CommitMetadataSerDeV2` -> `TimelineMetadataUtils.deserializeAvroMetadata` -> `new SpecificDatumReader<>(HoodieCommitMetadata.class)`, which avro 1.12.2 rejects, so the query fails on a valid table. Iceberg 1.12.0 does not need 1.12.2: every avro member referenced by the iceberg-api/core/common/aws 1.12.0 bytecode resolves against avro 1.12.1 (linkage-checked with javap, 182 members, none missing), and master already runs iceberg on avro 1.12.1. Keep `avro.version` at 1.12.1 and document why it intentionally differs from iceberg's. ### Release note None ### Check List (For Author) - Test: Manual test + Unit Test - `build.sh --fe` with the Maven build cache disabled passes, and every module packages avro-1.12.1: fe/lib, the hive/hudi/iceberg/paimon connector plugins and the BE java extensions (hadoop-hudi-scanner, iceberg-metadata-scanner, paimon-scanner, hadoop-deps). - Ran every iceberg and hudi FE unit test with run-fe-ut.sh: 1933 tests, 0 failures, 5 skipped. - A Hudi V2 commit-metadata read (COW incremental query) was not run locally. - Behavior changed: No (restores master's avro version) - Does this need documentation: No Co-authored-by: Isaac <no-reply@databricks.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#69031 Problem Summary: The iceberg 1.12.0 upgrade defaults iceberg-aws's HTTP client to `urlconnection` (`IcebergCatalogFactory.applyDefaultAwsHttpClient`) because iceberg-aws's own default, `apache`, is `Apache5HttpClient` since 1.12.0 and needs an AWS SDK newer than the 2.29.52 Doris pins. The default was applied with `putIfAbsent`, so a catalog with `http-client.type=""` (set at CREATE, or by an ALTER that clears it) kept the empty value. It passes the CREATE/ALTER check that rejects `apache`, and iceberg-aws's `HttpClientProperties` then maps the empty type back to `apache`, so the first S3/Glue call or the CREATE storage probe failed with `NoClassDefFoundError: software/amazon/awssdk/http/apache5/Apache5HttpClient$Builder`. Before the upgrade the same empty value selected the working Apache 4 client. Treat a blank `http-client.type` as unset in `applyDefaultAwsHttpClient`, which both the catalog properties and the storage probe properties go through. ### Release note None ### Check List (For Author) - Test: Unit Test - Added IcebergCatalogFactoryTest#buildBaseTreatsABlankHttpClientTypeAsUnset and IcebergConnectorTestConnectionTest#storageProbeTreatsABlankCatalogHttpClientTypeAsUnset; both build a real S3 client builder on the shipped AWS SDK. - `build.sh --fe` passes. Ran every iceberg and hudi FE unit test with run-fe-ut.sh: 1933 tests, 0 failures, 5 skipped. - Behavior changed: No (fixes a regression of the iceberg 1.12.0 upgrade in apache#69031 before it is released) - Does this need documentation: No Co-authored-by: Isaac <no-reply@databricks.com>
…pecs ### What problem does this PR solve? Issue Number: None Related PR: apache#69031 Problem Summary: iceberg 1.12.0 removed the deprecated `ManifestFiles.read(ManifestFile, FileIO)`, which read the partition spec from the manifest file's own metadata. The upgrade replaced it in `IcebergManifestCache` with `read(manifest, io, table.specs())`, which binds every manifest to the CURRENT schema's partition types. The cache-backed planner evaluates residuals and builds the delete index with the scan's specs (`SchemaAwareDataTableScan.specsFor(scan)`), bound to the scan's schema. After an identity partition column is promoted (for example `p INT` -> `BIGINT`), a scan pinned to a snapshot before the promotion binds its residual to INT while the cached partition values are `Long`, so `WHERE p = 1` on the old snapshot with the manifest cache and batch streaming failed with `IllegalArgumentException: Wrong class, expected java.lang.Integer, but was java.lang.Long` (the residual is evaluated after the streaming fallback, so it is not retried). Going back to the manifest's own spec does not help: the cache is keyed by manifest path only, so a current scan then reads the old INT values and fails the other way (`expected java.lang.Long, but was java.lang.Integer`), depending on which scan filled the cache first. - Read data and delete manifests in `IcebergManifestCache` with the scan's spec map, the same one its residuals and delete index use, so partition values always have the types the scan evaluates. - Add the bound partition type of the manifest's spec to `IcebergManifestEntryKey`, so scans whose schemas bind a manifest to different partition types get separate cached copies; scans with the same binding still share one entry, as before. ### Release note None ### Check List (For Author) - Test: Unit Test - Added IcebergScanPlanProviderTest#cachedStreamingScansKeepPartitionTypesAcrossIntToBigintPromotion: a v2 table identity-partitioned by p INT with a position delete, p promoted to BIGINT, then a historical scan (p = 1 as INT) and a current scan (p = 1 as BIGINT) streamed through one manifest cache, in both warm-up orders. It fails without this fix with the reported "expected java.lang.Integer, but was java.lang.Long", and also fails (the other way round) if manifests are decoded with their own spec. - Added IcebergManifestEntryKeyTest#differentBoundPartitionTypeAreNotEqual. - `build.sh --fe` (Maven build cache disabled) passes. Ran every iceberg and hudi FE unit test with run-fe-ut.sh: 1933 tests, 0 failures, 5 skipped. - Behavior changed: No (fixes a regression of the iceberg 1.12.0 upgrade in apache#69031 before it is released) - Does this need documentation: No Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
TPC-H: Total hot run time: 29061 ms |
Contributor
TPC-DS: Total hot run time: 152569 ms |
Contributor
ClickBench: Total hot run time: 24.57 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
|
/review |
Contributor
|
hi @laskoviymishka I will take a look at the second round AI review result. You don't need to handle it now |
Contributor
There was a problem hiding this comment.
Two P2 issues remain: real Iceberg system-table tasks lose FE/BE compatibility during rolling upgrades, and empty delete-manifest entries omit the new key's retained partition-type graph from weighted cache admission. Both are inline below. No new P0/P1 finding was verified.
Reviewed all 21 changed files at 98679abb2b68d36033e4e8fe8e10078168beb193, the related callers/consumers, and primary Iceberg 1.11/1.12 sources. This was a static review: no builds, tests, storage requests, or cluster experiments were run. Added unit-test assertions and the author's reported test runs are not executed reviewer evidence.
The three previous findings are fixed on this head and were not reposted: Hudi/Avro is addressed by retaining Avro 1.12.1 in the shared dependency management and Hudi runtimes; blank HTTP type is normalized in catalog and probe maps; historical partition typing now uses the scan-bound spec map for data/delete readers and a type-sensitive key, with both cache-fill orders covered by a new unit case. No previous P0/P1 finding still applies.
The reviewed execution chains were:
CREATE/ALTER -> property validation -> common/flavor options -> catalog/FileIO
Historical/current scan -> bound specs -> cache -> residuals/delete index -> native ranges
System-table scan -> Java task serialization -> BE assignment -> JNI deserialization -> rows
| Critical checkpoint | Conclusion |
|---|---|
| Goal and proving tests | SDK 1.12 adaptation is focused and has new HTTP/cache unit coverage; actual rolling task compatibility remains defective. |
| Change size and clarity | Changes reuse the common options builder and existing scan/spec machinery; no unrelated source changes. |
| Concurrency and locks | Immutable type keys preserve cross-query isolation; existing single-flight loading, publication fencing and deferred removal remain. Heavy manifest I/O stays outside cache compute/publication locks. No new lock-order defect verified. |
| Lifecycle and static initialization | Readers are closed and copied; probe TCCL/close boundaries remain. Same-version Schema descriptors are unchanged. The incompatible metadata-task lifecycle is reported inline. |
| Configuration and dynamic updates | No new global configuration. CREATE/ALTER validate merged catalog settings; rebuild applies the shared supported default. Explicit Apache settings require migration to URLConnection as the PR declares. |
| Binary, format and rolling compatibility | Vendored DeleteFileIndex matches SDK 1.12 apart from Doris access adaptations; Avro remains 1.12.1 and runtime closures were inspected. Mixed-version JNI task compatibility is the first finding. |
| Thrift/protobuf IDs | Not applicable: no definition changes. |
| Parallel paths | Data/delete readers, synchronous/streaming/count enumeration, all catalog flavor bases and the separate storage probe were covered. Native $position_deletes does not use the incompatible JNI wire. |
| Special conditions | Blank defaulting, missing-spec failure, historical type separation and partition validation have concrete rationale; delete checks match upstream applicability rules. |
| Functional and negative test coverage | New JUnit cases cover defaults, blanks, rejection, probe property copying and historical cache orders. Actual mixed-version non-null metadata tasks and empty-delete weighted admission need coverage. No end-to-end regression execution verified. |
| Changed test results | Unit assertions and option snapshots match intended behavior by inspection; no regression golden files changed. |
| Observability | Existing fallback warnings/cache statistics remain; new version-skew error improves diagnosis but leaves the query unavailable. No new hot-path INFO logging. |
| Transactions and persistence | No Doris journal, transaction or failover algorithm changes; catalog replay remains pure and snapshot/delete sequence selection was traced. |
| Data writes, atomicity and crashes | Existing SDK/catalog write paths remain; no new Doris mutation protocol. No additional silent wrong-result or crash defect verified. |
| FE/BE variable passing | No new field; the existing serialized task carries the changed Java graph. BE packages URLConnection for serialized FileIO properties. |
| Performance and memory | Lazy enumeration and per-spec comparator reuse remain. New key-only schema retention bypasses cache weights for empty delete payloads: second inline finding. |
| Other issues and user concerns | No additional user-provided focus. All candidates and risk items have evidence-backed dispositions; no changed file remains uncovered. |
Remaining validation limits are static inspection, arbitrary custom FileIO/server overrides, and unexecuted dependency/cluster integration. Explicit or server-forced unsupported Apache client settings remain a declared compatibility restriction; no separate speculative finding was posted for them. No unresolved candidate remains in the supported scope.
Review converged after 2 round(s).
…n its weight ### What problem does this PR solve? Issue Number: None Related PR: apache#69031 Problem Summary: `IcebergManifestEntryKey` now carries the bound partition type (a `Types.StructType`) of the manifest's spec, so scans that bind a manifest to different partition types get separate cached copies. When a byte limit applies to the manifest cache (`meta.cache.iceberg.manifest.max-weight`, `meta.cache.max-weight` or `external_meta_cache_max_weight`), every entry is admitted with its estimated weight, but `estimateManifestKey` still counted only the key object and the manifest path. For a non-empty manifest this does not matter: its files share the key's partition type instance, and the value estimate reaches it through them. A delete manifest whose entries are all DELETED (iceberg keeps it in the snapshot whose commit removed those delete files, and the cache-backed planner reads every delete manifest) yields an empty value, so nothing accounts for the partition type the key retains. Such entries were admitted below their retained size, more so for wide partition specs, and the difference stayed outside every manifest, catalog and global weight limit. - `estimateManifestKey` now adds the key's partition type, using the existing `estimateIcebergType`. A partition type shared by several keys is counted once per key, which overestimates, the safe direction for admission. ### Release note None ### Check List (For Author) - Test: Unit Test - Added IcebergManifestCacheTest#manifestKeyWeightCountsTheBoundPartitionType: with an empty delete-manifest value, an entry whose key has a 10-field partition type weighs at least nine `NestedField`s more than one with a 1-field type. It fails without the estimator change. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
|
run buildall |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed commit 703a33ca3b189514e0c4152986af8608b76a7dae: one new P2 finding for legacy S3 remote-signing compatibility. The SDK upgrade is focused, but existing remotely signed catalogs need adaptation before their metadata reads remain usable. No new P0/P1 issue was verified.
The review converged in 2 rounds. The second round returned NO_NEW_VALUABLE_FINDINGS from both complete reviewers and the dedicated risk reviewer. All 22 changed files, their related control flow and tests were covered, with every candidate and risk focus explicitly accepted, deduplicated or dismissed. No additional user-provided focus was supplied.
Existing review context was independently checked and not reposted:
- Hudi Avro P1 is fixed: the shared version remains 1.12.1. No existing P0/P1 finding still applies, so existing_blocking_comment_ids is empty.
- The blank HTTP-client default and historical partition-type issues are fixed: both catalog/probe maps normalize blank values, and data/delete readers, residuals and the index use the same scan-bound specs with typed cache keys.
- Cache-key accounting P2 is partly fixed. Direct partition fields/types are counted, but a partition predicate populates the retained StructType's Schema/name/id/accessor indices, which estimateIcebergType omits. An empty delete-manifest value cannot compensate for that graph. This remains within the original issue and is not a new inline finding.
- Rolling-upgrade task compatibility P2 still applies. Real metadata tasks carry changed BaseFile/PartitionData graphs; the Schema shim and improved exception do not make 1.11/1.12 FE/BE tasks interoperable. The author declined a fix; this is retained as a known limitation, not reposted.
Core invariants checked include pinned snapshot visibility, delete-file association, cache ownership/accounting and exception propagation. Native OLAP visible-version, MoW bitmap and BE allocator code are unchanged.
| Skill checkpoint | Static conclusion and evidence |
|---|---|
| Goal and proving tests | Iceberg SDK is raised to 1.12 with transport, delete-index, manifest-reader and diagnostics adapters; remotely signed metadata regression M2 and known mixed-version task limitation remain. Added JUnit tests support individual adapters, not a complete end-to-end upgrade proof. |
| Small, clear, focused scope | All 22 changed files relate to the SDK upgrade, adapters and tests; no unrelated implementation expansion. |
| Concurrency, lock costs/order | Concurrent queries share immutable cache keys/payloads; single-flight loads run outside publication key locks, invalidation leases fence publication. Streaming iterator has one consumer, and streaming avoids mutable query tallies. No new lock-order defect substantiated. |
| Lifecycle/static initialization | Readers are closed and copied records retained; cache reservations/invalidation lifecycle stays in the shared framework. Plugin child-first dependencies remain isolated; Schema source and UID guard are unchanged for same-version SDK. No cross-TU/static-order change. |
| Configuration/dynamic changes | Existing http-client.type now defaults blank/unset to urlconnection; apache rejected at CREATE/ALTER while of()/replay stays pure. Rebuild consumes changed catalog options. Legacy signer keys need migration (M2). No new FE/BE runtime config added. |
| Symbols/storage/rolling compatibility | Vendored DeleteFileIndex mirrors SDK 1.12 shape; Avro stays 1.12.1 and Caffeine is bundled 3.2.4. Real Java task graphs still fail across 1.11/1.12 (K5). Remote signer protocol compatibility M2. |
| Thrift/Proto branch field IDs | No gensrc changes or new protocol field IDs; not applicable. |
| Parallel code paths | Both data/delete readers use scan-bound specs across sync/stream/count. Catalog and storage-probe maps share HTTP normalization; scanner bundles URLConnection. REST/server signer property path remains a finding. |
| Special conditions | Blank-type normalization handles a concrete Apache5 default failure; spec checks fail loudly; conditional SDK partition matching matches upstream. No speculative source change added. |
| Test coverage/negative cases/modularity | Added tests inspect blank/default/explicit HTTP config, forbidden apache, probe settings, two cache warm orders, empty-key weight and serialization error causes. Remote signing, actual old/new task interoperability and populated empty-key Schema graph not proven by these tests. No builds/tests permitted or run. |
| Test result changes | No regression .out/golden changes; JUnit expectations inspected, not executed. |
| Observability | New skew exception includes SDK version and retains cause; cache failures keep existing warnings/failure metrics. No new metric requirement identified for these adapters. |
| Transactions/persistence/failover | No transaction or journal/write/replay format changes; statement validation is kept out of property reconstruction. |
| Data writes/atomicity/crash | No Doris data-write or transaction code changed; vendored delete validation protects reads and fails loudly. No new crash/leak path substantiated. |
| New FE/BE variable propagation | No new wire variable; existing serialized_split stays unchanged and therefore retains K5's mixed-version limit. HTTP choice is carried by serialized FileIO properties. |
| Performance/CPU/memory | Shared lazy scan enumeration remains; added typed-key variants are needed for correctness. K4's direct type fields are counted, populated lazy Schema graph remains a known accounting gap. avgValueSizes suspicion M1 dismissed because supported manifest reads do not retain it. |
| Other issues/user focus | No additional user focus. All candidates remain explicit; M2 is one distinct new P2, K4/K5 already covered. |
Validation was static only: no builds, tests, services or source modifications were performed. Added JUnit assertions, upstream SDK sources and dependency symbols were inspected; author-reported test results were not treated as executed evidence. Real remote signing, deployed plugin/classloader behavior, FE/BE interoperability and heap measurements remain unexecuted. The new tests do not establish remotely signed manifest reads, real mixed-version task compatibility or populated empty-key Schema accounting. No changed-file coverage gap or unresolved distinct code candidate remains.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 29260 ms |
Contributor
TPC-DS: Total hot run time: 152423 ms |
Contributor
ClickBench: Total hot run time: 24.63 s |
morningman
approved these changes
Oct 14, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
suxiaogang223
approved these changes
Oct 15, 2026
CalvinKirs
approved these changes
Oct 15, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
5 of 16 tasks
morningman
added a commit
that referenced
this pull request
Oct 27, 2026
#69135) ### What problem does this PR solve? Issue Number: #68916 Related PR: #69031 Problem Summary: The Iceberg REST catalog spec now lets a catalog return `labels` with each table load (apache/iceberg#15750): catalog-owned, per-request metadata such as ownership, classification or cost attribution, at the table level (`object-labels`) and per field (keyed by `field-id`). Labels are not part of the table metadata, have no snapshots and are never written to `metadata.json`. Iceberg 1.12.0 exposes them on the loaded table via `SupportsLabels#labels()`. Doris had no way to show them. Add a `t$labels` system table for iceberg tables in a REST catalog: ```sql SELECT * FROM rest_ctl.db.tbl$labels; scope | field_id | field_name | key | value object | NULL | NULL | owner | team-a field | 2 | email | classification | pii ``` One row per label. `field_name` is resolved against the current table schema and is NULL for a field id that is no longer in it (a dropped column). The schema mirrors iceberg's proposed `LabelsTable`. Implementation: - Keep labels when Doris rebuilds a loaded table. iceberg's REST client attaches them to the loaded `BaseTable` as a transient field; they are not table metadata, so a `BaseTable` rebuilt from its `TableOperations` must pass them explicitly. `IcebergTableCache.TableLease#snapshotReadTable` (cached reads), `IcebergStatementScope#snapshotReadTable` (statement reads) and `IcebergScanPlanProvider#wrapTableForScan` (Kerberos FileIO wrap) used constructors that reset labels to `Labels.EMPTY`, so every read saw no labels; they now pass `baseTable.labels()`. - `labels` is not a `MetadataTableType`, so it is handled next to the SDK metadata tables rather than through `MetadataTableUtils`. `IcebergConnectorMetadata` lists and resolves it only when the catalog flavor is `rest` (no other catalog returns labels), and serves its fixed schema and column handles without loading the base table. - `IcebergScanPlanProvider` plans it as one split. A new split-package helper `org.apache.iceberg.IcebergLabelsDataTask` builds an iceberg `StaticDataTask` (package-private in the SDK) holding the label rows, projected to the requested columns in BE's required-field order; its metadata `DataFile` is synthesized, so no storage request is made. The task is serialized like every other static metadata table, so BE's `IcebergSysTableJniScanner` reads it with no BE change. - The ordered system-table projection is extracted into `projectSystemTableSchema` and shared with the existing path, and the base-table load is split out of `resolveSysTable` as `resolveSysBaseTable`. - Labels are not versioned: like `$snapshots`/`$history`, the snapshot pin of the handle is ignored, and `$labels` is exempt from the resolved-empty-snapshot fence, so a table with no commit still shows its labels. Labels reflect the table load the statement used, so they follow the iceberg table cache (`meta.cache.iceberg.table.ttl-second`); REFRESH TABLE picks up new labels. Like the other system tables, a REST catalog that requires server-side scan planning rejects `$labels`. Co-authored-by: Isaac <no-reply@databricks.com> Co-authored-by: morningman <yunyou@selectdb.com>
3 of 16 tasks
morningman
added a commit
that referenced
this pull request
Nov 6, 2026
…itialize (#70164) ### What problem does this PR solve? Issue Number: None Related PR: #66913, #66914 Problem Summary: **In short.** This ports from branch-4.x #66913 the cleanup of an Iceberg `HiveCatalog` whose initialization fails after it has created its FileIO: on master, nothing closes that FileIO. With the FileIOs master configures, the leaked object holds no native resource today, so this is hygiene that keeps a FileIO with real resources from leaking once per failed catalog creation. **Background.** `DorisHiveCatalog` (fe-connector-iceberg, used for Iceberg on HMS) extends Iceberg's `HiveCatalog` and owns the FileIO the catalog creates (#66914). In Iceberg 1.12, `HiveCatalog#initialize` creates the FileIO first, then the Hive client pool, and only then the `CloseableGroup` that closes the FileIO, the KMS client and the metrics reporter. #66913, the branch-4.1 port of #66914, closes the FileIO when initialization fails; master's `DorisHiveCatalog` does not. **The problem, and what it cost.** When `CachedClientPool` fails (an unknown element in `client-pool-cache-keys`, a `clients` that is not a number) or the metrics reporter fails to load (`metrics-reporter-impl`), the catalog has a FileIO but no group: `HiveCatalog#close` closes nothing, and `DorisHiveCatalog` has not captured the FileIO yet. The catalog properties reach Iceberg as the user gave them, and `IcebergConnector` caches only a catalog that initialized, so each metadata access on such a catalog creates and drops another FileIO. Today that is only garbage: master's HMS catalogs use `HadoopFileIO`, whose `close` does nothing, and `ResolvingFileIO` and `S3FileIO` create their clients lazily. **How this PR fixes it.** When `HiveCatalog#initialize` throws, `DorisHiveCatalog#initialize` closes the FileIO the catalog created and rethrows; a failure to close is added to the initialization failure as suppressed. The FileIO is null when loading the FileIO itself failed, and then there is nothing to close. Not ported from #66913: its `close()` changes, since Iceberg 1.12 (#69031) closes the FileIO in `HiveCatalog#close` itself, and its `DorisHadoopCatalog`. The hadoop flavor has the same order (`HadoopCatalog` creates the FileIO before the lock manager), but master uses Iceberg's `HadoopCatalog`, and a subclass would change the catalog-impl mapping without freeing anything today. **Results.** | `client-pool-cache-keys` with an unknown element | Before | After | |---|---|---| | `DorisHiveCatalog#initialize` | throws `IllegalArgumentException`, the FileIO stays open | throws `IllegalArgumentException`, the FileIO is closed | Co-authored-by: 924060929 <lanhuajian@selectdb.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Upgrade the Iceberg Java SDK from 1.11.0 to 1.12.0. 1.12.0. This change only upgrades the dependency and adapts Doris to it.
iceberg.version1.11.0 -> 1.12.0, andavro.version1.12.1 -> 1.12.2 to stay consistent with what iceberg 1.12.0 is built against (the pom requires the two to move together).org.apache.iceberg.DeleteFileIndexfrom 1.12.0 and re-apply the Doris changes (public class/builder methods, schemasById comment, equality-field-id sets). The vendored class shadows the SDK class in the same package, and 1.12.0 changed it (an extraspecsByIdconstructor argument and delete/data file partition-match validation), so keeping the 1.11.0 copy would break the SDK's own callers at runtime. Since Doris passesspecsByIdto the builder, the cached scan planning path now also rejects delete files whose partition spec or tuple does not match the data file, the same as upstream iceberg.table.specs()toManifestFiles.readinIcebergManifestCache. Iceberg 1.12.0 removed the deprecatedread(ManifestFile, FileIO)overload, which read the partition specs from the manifest file's own metadata (only possible for Avro manifests). This matches the delete manifest path next to it (readDeleteManifest(..., table.specs())) andIcebergRemoveOrphanFilesAction.urlconnection. In 1.12.0, iceberg-aws's defaulthttp-client.type(apache) builds anApache5HttpClientfromsoftware.amazon.awssdk:apache5-client, which first shipped in AWS SDK 2.32. Doris pins 2.29.52 on purpose ([fix](S3)Downgrade the AWS SDK version to avoid impacting other S3-compatible services. #58566: newer SDKs add default S3 integrity checksums that break S3-compatible services), so with the default every S3FileIO, Glue and STS client iceberg builds failed withNoClassDefFoundError: software/amazon/awssdk/http/apache5/Apache5HttpClient$Builder.IcebergCatalogFactory.buildBaseCatalogProperties, the base of every catalog flavor, now setshttp-client.type=urlconnectionunless the catalog sets it. S3FileIO keeps the setting when it is serialized, so the BE iceberg metadata scanner, which rebuilds the FileIO from it, uses the same client;url-connection-clientis added to that scanner, which only bundledapache-clientandnetty-nio-client. An explicithttp-client.type=apacheis rejected at CREATE/ALTER CATALOG instead of failing on the first S3 or Glue call.SchemaAwareDataTableScan(the other vendored class) needs no change:DataTableScan,BaseTableScan,SnapshotScanandTableScanContextare identical between 1.11.0 and 1.12.0.Release note
Upgrade the Iceberg Java SDK to 1.12.0. Iceberg's AWS clients (S3FileIO, Glue, STS) now use the URLConnection HTTP client by default, and the
http-client.type=apachecatalog property is no longer accepted. Iceberg 1.12.0 no longer reads the S3 remote-signing propertiess3.signer.uriands3.signer.endpointand no longer defaults the signer endpoint tov1/aws/s3/sign, so an Iceberg REST server that enables S3 remote signing must sendsigner.uri/signer.endpointor serve the table-scoped sign endpoint. Iceberg system tables ($snapshots,$files, ...) cannot be queried while FE and BE are on different Iceberg versions (1.11 and 1.12), e.g. during a rolling upgrade; they work again once every node is upgraded.Check List (For Author)
build.sh --fepasses. Ran every iceberg FE unit test with run-fe-ut.sh (fe-connector-iceberg, fe-connector-metastore-iceberg, be-java-extensions/iceberg-metadata-scanner): 1630 tests, 0 failures, 5 skipped.http-client.apache.*properties no longer apply, andhttp-client.type=apacheis rejected at CREATE/ALTER CATALOG.s3.signer.uri/s3.signer.endpointand no longer defaults the signer endpoint tov1/aws/s3/sign. A REST catalog whose server turns on remote signing but sends only those legacy keys, or relies on that default, fails to read manifests. No compatibility shim is added: Doris does not use remote signing to read data (BE uses static or vended credentials), andsigner.uri/signer.endpointare deprecated in 1.12.0 and scheduled for removal in 1.13.0.BaseFileandPartitionData, which system table tasks carry. BE reports the version mismatch instead of a bareInvalidClassException. Normal data scans are not affected.What problem does this PR solve?
Issue Number: related #68916
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)