[fix](iceberg) Fix historical scans after schema evolution - #67687
Merged
Gabriel39 merged 4 commits intoSep 14, 2026
Merged
Conversation
Adapt the branch-4.1 fix to the connector-based master layout and keep the Iceberg HiveCatalog dependency aligned with Iceberg core. (cherry picked from commit 2c84c7f)
Gabriel39
requested review from
924060929,
CalvinKirs and
morningman
as code owners
September 9, 2026 02:11
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes: five distinct correctness or compatibility issues remain on the exact reviewed head.
Review checkpoint conclusions:
- Goal and scope: the Iceberg 1.11 upgrade is focused, but it does not yet fully fix historical scans. The schema-only snapshot state and manifest-cache equality-delete path remain broken, and evolved partition metadata can be rendered incorrectly.
- Correctness, conditions, error handling, and parallel paths: the accepted findings cover a lazy streaming failure, an incorrect unified-partition slot, and the equal-snapshot schema-binding gap. Synchronous planning can fall back where streaming cannot; no additional condition or propagation defect survived review.
- Configuration and compatibility: single-version connector, shade, and DLF construction are aligned, and the null KMS argument preserves the prior unencrypted path. The FE-to-BE Java-serialized system-table task is not compatible across a rolling 1.10.1/1.11.0 deployment.
- Concurrency and lifecycle: no new shared-state, locking, resource-lifetime, or concurrency defect was found.
- Transactions, persistence, and writes: this PR introduces no transaction or data-write protocol change. Persisted metadata reload is material to the equality-delete finding; the other reload fixture is non-vacuous.
- FE/BE, memory, and null handling: the mixed-version FE/BE wire boundary is the outstanding cross-component issue. No separate BE native-memory, ownership, or null-handling issue was introduced.
- Tests and results: the new unit tests skip the schema-only failure and check only partition-row count, while the branch regression removes valid predicate coverage. The remaining expected-output changes are consistent with name-projected metadata conversion. Tests were not run because the review runner explicitly forbids builds and tests.
- Observability and performance: no separate logging, metrics, or material performance issue was found; the cache path distinction is already captured by the correctness finding.
- Focus and completeness: there was no additional user-provided focus. The entire authoritative diff and all changed files were reviewed. Two bounded review rounds, including a separate edge-risk pass, converged with no new findings after these five were accepted and duplicate-fenced.
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 16858 ms |
Contributor
TPC-DS: Total hot run time: 83082 ms |
Contributor
ClickBench: Total hot run time: 14.73 s |
### What problem does this PR solve? The Iceberg 1.11 upgrade exposed additional historical-scan issues in the connector path: metadata-only schema commits could leave scans bound to current partition specs, the manifest-cache path omitted historical equality-delete schemas, evolved partition values were read by spec-local position, and Java-serialized system-table tasks were incompatible across 1.10.1/1.11 rolling upgrades. ### How does this PR solve the problem? - Bind historical data scans and manifest-cache evaluators to the selected schema. - Preserve all table schemas when indexing equality deletes. - Read unified partition structs by partition field ID. - Add a narrowly scoped Schema serialization compatibility bridge for mixed Iceberg 1.10.1/1.11 FE and BE pairs. - Restore branch predicate coverage and add focused unit coverage for each compatibility case. ### Test - IcebergScanPlanProviderTest and IcebergPartitionUtilsTest: 228 passed. - IcebergSerializationCompatTest: 2 passed. - FE Checkstyle: 0 violations. - git diff --check: passed.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes: two P1 correctness failures and one P2 partition-metadata defect remain on the current head.
Critical checkpoint conclusions:
- Goal and tests: the Iceberg 1.11 upgrade and historical-schema repair are clear, and the new direct tests cover several original failures, but they miss the production batch estimator, the latest-pin manifest-cache state, and multiple historical buckets after a dropped source.
- Scope: the dependency bump, upstream delete-index sync, serialization bridge, scan/schema changes, partition logic, and regression outputs are related to the stated upgrade. No unrelated source change was found.
- Concurrency and lifecycle: the lazy cache iterator, close paths, per-scan state, and scanner stream lifecycle add no distinct race or resource leak.
- Configuration: version properties, shade ownership, Java 17 baseline, Avro/Caffeine/AWS closure, and DLF nullable-KMS behavior are internally consistent; no distinct configuration defect survived.
- Compatibility and rolling upgrade: the Schema-only bridge still cannot cover other changed task-graph descriptors, but that is already fully raised by existing thread 3964216048 and is not duplicated here.
- Parallel paths and conditions: synchronous cache planning and streaming batch estimation each bypass the intended schema generation in a different way; those are the two P1 comments. Branch/tag, system-table, COUNT, empty-table, and delete-index paths were separately traced with no additional distinct issue.
- Test coverage/results: the accepted issues identify missing regression oracles. No local build or test was run because this review runner explicitly prohibits builds; current CheckStyle and dependency-review checks are successful, but no broader test result is claimed.
- Observability: failures are either wrapped as query errors or silently over-pruned as described; no separate logging/profile regression was found.
- Transaction, persistence, and writes: schema-only commits, snapshot/schema pin persistence, cached manifests, equality-delete history, and DLF write construction were reviewed; no additional write/transaction defect survived.
- FE-BE contract: initial-default transport and system-table task serialization were traced. The new comments are distinct from the existing wire-format thread.
- Performance: no new hot-path regression was substantiated beyond the correctness-affecting batch/cache paths already commented.
Existing inline threads were treated as hard duplicate fences. The user supplied no additional focus beyond the full PR review. Round 2 full-coverage and risk-focused reviewers all returned NO_NEW_VALUABLE_FINDINGS after these three accepted points were recorded.
### What problem does this PR solve? Three schema-evolution paths still bypassed the selected metadata generation: batch estimation used current partition specs, manifest-cache pruning could use a snapshot schema instead of the latest MVCC schema, and partition metadata cached by snapshot alone could survive schema-only evolution and fabricate collapsed null partitions. ### How does this PR solve the problem? - Rebind partition specs for streaming batch estimation. - Preserve the handle-selected logical schema after snapshot selection. - Treat missing unified partition fields as unrepresentable and return the existing safe empty display. - Include schema and spec IDs in the raw partition cache key. ### Test - Added red/green coverage for all three review findings. - IcebergScanPlanProviderTest, IcebergPartitionUtilsTest, IcebergPartitionCacheTest, and IcebergConnectorCacheTest: 266 passed. - FE Checkstyle: 0 violations. - git diff --check: passed.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: five distinct new issues are annotated on 3f775f5c7668834424cbcbece96b7af570d23f9d. The prior rolling-wire blocker also remains unresolved.
New findings:
- [P1] Derived partition-view caches omit the partition-spec generation and can remain stale across a spec-only commit.
- [P2] The representability guard examines retained specs with no live entries and can hide valid current partitions.
- [P1] Historical-schema handling replaces the catalog-native scan and bypasses REST server planning.
- [P1] Scan-scoped credentials returned by a REST plan are never propagated to BE.
- [P1] Batch estimation and manifest-cache/count paths locally open manifests before or instead of REST server planning.
Existing hard duplicate fence: the rolling system-table wire thread is not resolved by the Schema-only bridge. Real manifest-read task graphs retain S3FileIO, whose generated UID differs across Iceberg 1.10.1/1.11.0 and fails cross-version deserialization in both directions. AllManifestsTable.ManifestListReadTask also changed its retained field layout and generated UID. I did not post a duplicate inline comment, but this remains release-blocking.
Review checkpoints:
- Goal and tests: the historical-schema goal is valid, but the added in-memory/raw-cache/
StaticDataTaskcoverage misses warm derived caches, a retained old spec with no live files, native REST server planning, plan-response-only credentials, and real mixed-version manifest tasks. - Scope and integration: the Iceberg upgrade, cache changes, system-task wire, Hive shade, DLF constructor, and vendored delete index were reviewed together. The shade/DLF changes and vendored
DeleteFileIndexmatched the expected 1.11 integration; no separate defect survived there. - Concurrency and lifecycle: no new lock-order or shutdown issue was found. The cache defect is a generation-key problem, while the FileIO defect is a scan lifecycle/credential handoff problem.
- Configuration and persistence: independently configured partition-view TTLs expose the stale key; metadata-only
updateSpecpreserves snapshot/schema IDs. REST server-planning mode is incompatible with the local estimator/cache/count shortcuts. - Compatibility and FE/BE passing: Java-serialized task graphs remain rolling-incompatible beyond
Schema, and scan-scoped FileIO properties are not carried to BE. - Parallel paths and performance: synchronous, streaming, batch-estimation, manifest-cache, and count paths were all traced. Optimizations must preserve the native server-plan contract rather than substitute local enumeration.
- Conditions and observability: the retained-spec guard reasons about all table metadata rather than the selected snapshot's live entries; its safe fallback can silently erase the partition display. Cache staleness is likewise silent, while REST/rolling-wire failures surface as access or deserialization errors.
- Writes and atomicity: no changed write/atomicity path produced a separate issue.
This was a review-only pass; I did not run Doris builds or project tests.
### What problem does this PR solve? Issue Number: None Related PR: apache#67687 Problem Summary: Iceberg partition-spec-only commits could leave derived partition caches stale because their keys covered only snapshot and schema generations. Historical specs with no live files could also hide valid current partitions. In addition, Iceberg REST server-side planning was silently routed through local manifest and table FileIO paths that cannot preserve the native scan or consume scan-scoped credentials. Include the live spec generation in derived cache keys, validate representability only for specs present in live partition rows, and reject unsupported server-side planning before local metadata access. ### Release note Iceberg REST catalogs must use client-side scan planning until Doris supports server-planned scan credentials. ### Check List (For Author) - Test: Unit Test - ConnectorMetadataCacheTest, IcebergConnectorMetadataPartitionViewCacheTest, IcebergPartitionUtilsTest, and IcebergScanPlanProviderTest - Behavior changed: Yes. Partition metadata follows spec-only evolution, and unsupported REST server-side scan planning now fails explicitly. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
There was a problem hiding this comment.
Automated review completed on exact head dab9c2ef7ad0aa720f9a52b1fa1d6451b6545232 against base b3766d781b529f89ec2dadcf53ecf712aec0ae7b.
Overall opinion: I found no new diff-caused issue that is distinct from the existing review threads, so this is a comment-only review with no new inline comments. This does not clear the existing blockers: the Schema-only rolling bridge still fails for concrete non-Schema task graphs such as $all_manifests and DLF S3FileIO, but that is exactly the full-FileScanTask/both-directions obligation already raised in the compatibility thread. The remaining spec/projection-generation and unified-row variants are likewise covered by the derived-cache thread and the raw-projection thread.
Checkpoint conclusions:
- Goal, scope, and clarity: all 24 changed files and the full authoritative diff were reviewed. The historical/current schema, delete-history, partition projection, spec-generation cache, REST rejection, dependency-upgrade, and same-version metadata-table changes are coherent on their changed paths. No extra user focus was supplied.
- Correctness and parallel paths: native, manifest-cache, streaming estimator/enumerator, synchronous, COUNT, snapshot/tag/branch, system-table, and delete-file paths were traced. No distinct schema-binding, pruning, delete-association, or REST-planning defect survived beyond the existing threads.
- Compatibility, configuration, and FE/BE values: no runtime setting or Thrift field is added.
serialized_splitremains the material FE/BE contract; its proven non-Schema mixed-version failures remain unresolved on the existing compatibility thread. The Iceberg/Hive/AWS/RoaringBitmap ABI and shade closures were checked without finding another linkage defect. - Concurrency, lifecycle, errors, and observability: cache values and compatibility constants are immutable, loaders are single-flight, scanner/manifest resources retain their ownership and close paths, and unsupported REST planning fails before local scan or credential access. No new lock-order, publication, resource-release, swallowed-error, or diagnostic issue was found.
- Persistence, writes, memory, and performance: this diff adds no Doris persistence format, transaction protocol, or data-write atomicity change. No material new hot-loop, retention, or allocation problem was substantiated.
- Tests and validation: the new tests cover the intended same-version schema/spec/delete/partition/REST paths, but the static serialization fixture does not prove real mixed-version task graphs; that gap is the existing compatibility blocker. Per the review instructions, no product build or product test was run. Review-only static and isolated serialization checks were used, and
git diff --checkis clean. At submission time the lightweight GitHub checks pass; COMPILE, FE UT, performance, and automated review checks remain pending. - Completion: three bounded review rounds completed. All final normal and risk reviewers returned
NO_NEW_VALUABLE_FINDINGS; every candidate was independently verified, dismissed on exact diff-left provenance, or duplicate-fenced. Review status: complete for this exact head.
Contributor
TPC-H: Total hot run time: 17047 ms |
Contributor
TPC-DS: Total hot run time: 82982 ms |
Contributor
ClickBench: Total hot run time: 14.94 s |
shuke987
added a commit
to shuke987/doris
that referenced
this pull request
Sep 11, 2026
### What problem does this PR solve? Problem Summary: The automated review of apache#67687 hit the 90-minute step timeout while finishing its report ([failed run](https://github.com/apache/doris/actions/runs/34488778375/job/102909763020)); its [formal review](apache#67687 (review)) was submitted about 23 seconds later. Increase the review step timeout from 90 to 120 minutes. Raise the job timeout from 208 to 238 minutes and update the budget comment, preserving 8 minutes for auth synchronization and 12 minutes for runner setup and post-job cleanup. ### Release note None ### Check List (For Author) - Test: - [x] Unit Test: all 18 existing `test_review_auth_quarantine.py` tests passed against the modified workflow using fake OSS, Codex, and GitHub services. - [x] Manual test: parsed the YAML and verified the 120-minute review limit, 218-minute pre-finalization budget, 8-minute auth sync, and 12-minute reserve within the 238-minute job limit. Confirmed no other parsed workflow changes; patch whitespace check passed. - Behavior changed: - [x] Yes. Automated reviews may run for up to 120 minutes. - Does this need documentation? - [x] No. The workflow budget comment is updated.
8 tasks
hello-stephen
pushed a commit
that referenced
this pull request
Sep 11, 2026
The automated review of #67687 hit the 90-minute step timeout while finishing its report ([failed run](https://github.com/apache/doris/actions/runs/34488778375/job/102909763020)); its [formal review](#67687 (review)) was submitted about 23 seconds later. Increase the review step timeout from 90 to 120 minutes. Raise the job timeout from 208 to 238 minutes and update the budget comment, preserving 8 minutes for auth synchronization and 12 minutes for runner setup and post-job cleanup.
924060929
approved these changes
Sep 14, 2026
morningman
approved these changes
Sep 14, 2026
morningman
added a commit
to morningman/doris
that referenced
this pull request
Sep 14, 2026
…JUnit 5 master merged apache#67687 (2026-09-14) after 630243e retired JUnit 4 from be-java-extensions on this branch, and it added one new JUnit 4 test in iceberg-metadata-scanner. On master that is fine: suppressions.xml still carries the temporary banJUnit4 exemption for be-java-extensions, parked there precisely so this PR could do the migration. This branch removed the exemption together with junit-vintage-engine, so after the merge the new test is the one JUnit 4 class left in the reactor and checkstyle stops the FE build at that module (TeamCity Compile #121562). Same mechanical conversion as 630243e: Assert -> Assertions, org.junit.Test -> org.junit.jupiter.api.Test. Both assertions are the two-argument forms, so no message argument moves. The class runs under the jupiter engine (2 tests, not silently ignored) and the banJUnit4 regex finds nothing else under fe/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01222rs932qTF8BjpDrYjUEj
This was referenced Sep 15, 2026
Open
16 tasks
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
4 participants
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 this PR does
Ports #67479 from branch-4.1 to master.
DeleteFileIndex.Issue: DORIS-28397
Tests
fe-connector-icebergreactor test run: 1390 passed, 5 skipped, 1 failed. The single failure (IcebergWritePlanProviderTest.planMergePreservesExplicitlyEmptyReadAcrossConcurrentFirstAppend) reproduces unchanged on the master baseline and is unrelated to this PR.