[fix](lakehouse) Fix Iceberg historical reads and nested schema evolution - #66007
Merged
Gabriel39 merged 5 commits intoJul 31, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
marked this pull request as ready for review
July 24, 2026 09:50
Gabriel39
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
July 24, 2026 09:50
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 23405 ms |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-DS: Total hot run time: 118395 ms |
Contributor
ClickBench: Total hot run time: 18.64 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 23945 ms |
Contributor
TPC-DS: Total hot run time: 123088 ms |
Contributor
ClickBench: Total hot run time: 18.9 s |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 22848 ms |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 30th, 2026 7:58 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29529 ms |
Contributor
TPC-DS: Total hot run time: 176965 ms |
Contributor
ClickBench: Total hot run time: 24.79 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all 40 changed files at a9d44b45502c8e6f557468583f841169a381a6e9 against the authoritative bundle and the existing review threads. I found two new issues; the other residual concerns are already covered by existing threads.
Critical-checkpoint conclusions:
- Goal and tests: The patch substantially improves historical Iceberg reads, nested schema evolution, ORC masking/lifetimes, and Struct nullability, with broad unit/regression coverage. The empty-read MERGE conflict edge and quoted nested comments remain untested and incorrect.
- Scope and clarity: Changes are generally focused, but the read/write table split introduces a sentinel boundary that needs an explicit state contract.
- Concurrency: Statement-local maps use the existing scope and transaction begin lock correctly. The material race is a concurrent first Iceberg append between empty target binding and the first writable-table load.
- Lifecycle: Frozen read tables and statement scopes have bounded statement lifetimes; no new ownership cycle or static-initialization issue was found.
- Configuration: No new configuration item is introduced. Existing strict-cast and DESCRIBE-comment settings are read through their established session paths.
- Compatibility: FE/BE Struct nullability remains rolling-compatible through the function-builder complex-type compatibility path; no storage-format or protocol incompatibility was found.
- Parallel paths: Data/system tables, branch/tag/snapshot reads, INSERT/DELETE/UPDATE/MERGE/REWRITE, new/legacy coercion reducers, and Struct/named_struct FE/BE paths were traced. Remaining generation/mask defects outside these two findings duplicate existing threads.
- Conditional logic: Snapshot-selection exclusions and nullable-mask conditions are documented and match the supported paths; the
-1empty/no-pin conflation is the missing distinction. - Test coverage: Existing tests cover current-version coercion, frozen BaseTable reads, system-table selection, ORC lifetime/masks, and changed regression matrices. They lack the two barrier/escaping cases reported inline.
- Test results: No build or test was run because the authoritative review prompt prohibits builds and test execution; this review is static.
- Observability: No additional logging or metrics are required for these deterministic planning/formatting paths.
- Transactions and persistence: No Doris EditLog change is involved. Iceberg RowDelta conflict validation is affected by F1; other transaction concerns are covered by existing review threads.
- Data writes: F1 can violate serializable MERGE semantics by missing a concurrent first append; no additional crash/leak issue was found.
- Variable propagation: The existing snapshot/schema handle fields reach scan and write paths for nonempty snapshots, but the empty sentinel loses its state before the write context.
- Performance: The clear-mask fast path and Arena ownership check avoid the suspected large allocation/double-copy costs; no distinct hot-path regression remained.
- Other issues: F2 makes nested-comment DESCRIBE type strings malformed for valid quoted/backslash comments.
Additional user focus: none was provided.
| Table loaded = IcebergStatementScope.sharedTable(session, db, tableName, | ||
| // Reuse the write planner's mutable table, never the frozen read view: newTransaction | ||
| // refreshes operations to establish a fresh OCC base and must not mutate bound read metadata. | ||
| Table loaded = IcebergStatementScope.sharedWritableTable(session, db, tableName, |
Contributor
There was a problem hiding this comment.
[P1] Preserve an explicitly empty read as the RowDelta base
An empty latest snapshot is -1, but applySnapshot leaves that handle unpinned and buildWriteContext passes -1. With this new independent writable-table load, a MERGE whose target was bound to empty T0 can load a concurrent first append S1 here; applyBeginGuards then records S1 as baseSnapshotId. The commit calls validateFromSnapshot(S1), so serializable validateNoConflictingDataFiles ignores the matching file added in S1 and the MERGE can insert a duplicate. Iceberg's BaseRowDelta uses an unset starting snapshot to check all versions, so this edge must preserve "explicitly empty" separately from "no pin" (or otherwise validate from T0). Please add an empty-bind / concurrent-first-append MERGE barrier test.
| } | ||
| // Nested docs are part of DESCRIBE output only when comments were explicitly requested. | ||
| if (showNestedComment && field.isCommentSpecified()) { | ||
| desc.append(String.format(" comment '%s'", field.getComment())); |
Contributor
There was a problem hiding this comment.
[P2] Quote nested comments in DESCRIBE type strings
With show_column_comment_in_describe=true, a valid nested comment such as owner's \path is rendered as comment 'owner's \path'. That is not a valid round-trippable SQL literal, and backslashes may also be reinterpreted. The parallel Nereids StructField.toSql() uses SqlLiteralUtils.quoteStringLiteral for this syntax. Please use equivalent SQL-literal quoting here and add coverage for quotes and backslashes.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Historical Iceberg source snapshots could leak their schema pin into connector sink binding in the same INSERT statement. This made valid writes against the latest target schema fail when a column had been renamed. Iceberg STRUCT conversion also defaulted every nested field to nullable and dropped field documentation, causing system-table schemas such as entries to disagree with Iceberg metadata.
Bind connector write targets with an explicit latest-schema lookup while retaining source snapshot pins, and carry nested field requiredness and documentation through the neutral connector type mapping.
### Release note
Fix connector writes that read historical Iceberg snapshots and preserve nested Iceberg field requiredness.
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- Behavior changed:
- [x] Yes. Connector sinks bind the latest target schema and Iceberg nested fields retain requiredness.
- Does this need documentation?
- [x] No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Gabriel39
added a commit
that referenced
this pull request
Jul 31, 2026
…sistency fixes (#66246) ### What problem does this PR solve? This backports the following lakehouse fixes to `branch-4.1`: - #66112: enforce external write correctness for Iceberg operations. - #66223: preserve pruned Paimon struct field reads. - #66007: preserve metadata generations and nested schema semantics across Iceberg, Paimon, Hudi, ORC, and complex-type materialization. The #66007 changes were adapted to the `branch-4.1` APIs while preserving the source PR's behavior. A minimal nullable-cast helper required by the backport was included instead of pulling the unrelated master refactor that originally introduced it. ### Check List - FE focused unit tests: 202 successful test executions across the initial suite and the Paimon follow-up run. - FE Maven reactor: `BUILD SUCCESS`. - BE affected production and test translation units: compiled successfully with ASAN settings. - BE #66007 ORC and complex-rematerialization tests: 18 tests passed on the exact #66007 head. - `git diff --check`: passed.
Contributor
TPC-H: Total hot run time: 29493 ms |
Contributor
TPC-DS: Total hot run time: 178284 ms |
Contributor
ClickBench: Total hot run time: 24.99 s |
Contributor
There was a problem hiding this comment.
Requesting changes for two distinct issues identified in the current head; details are inline.
Critical checkpoint conclusions:
- Goal and proof: The change substantially addresses historical Iceberg generation/schema selection, nested requiredness propagation, and ORC masked-collection payload lifetime, with focused unit/regression coverage in the diff. It is not complete because trivial collection children and strict pre-cast Struct children still fail reachable cases.
- Scope and focus: The 44-file change is broad but coherent around those contracts. No additional user-provided review focus was supplied, so the whole PR was reviewed.
- Concurrency: Iceberg statement-scope reads, mutable write helpers, transaction refresh, sink binding, and optimistic commit ordering were traced. The remaining T0/T1 write-generation interval and empty-snapshot RowDelta base are already represented by existing live threads; no additional distinct concurrency finding is duplicated here.
- Lifecycle and memory: ORC row batches, compacted temporary columns, borrowed versus Arena-owned strings, empty/absent positions, and
Writer::addlifetime were checked. The current Arena lifetime and packed-coordinate paths are sound after excluding existing live ORC findings; no static-initialization or ownership-cycle issue was found. - Configuration: No new configuration item is added.
- Compatibility: The FE/BE Struct child-nullability difference is tolerated by the nested return-type compatibility helper, and
FunctionStructmaterializes the FE-supplied layout, so both rolling directions were verified safe; no new incompatibility remains. - Parallel paths and conditions: Non-trivial ARRAY/MAP rematerializers now project ancestor masks, but the trivial-child alignment path still drops them (MAIN-2). Strict-aware common-type and outer-Struct checks were updated, but a pre-cast child remains mode-blind nullable (MAIN-3). Other adjacent paths were either sound or already covered by live threads.
- Tests and result files: The diff adds substantial FE/BE and Iceberg regression coverage, and changed result files were checked against their query shapes. Missing coverage corresponds directly to the two inline findings. Per the review task, this was a static review: no build or test command was run; the worktree initialization marker and
thirdparty/installedare also absent. - Observability: No new logs or metrics are required for these local conversion/materialization paths; existing errors propagate with actionable messages. No silent
Statusdiscard was found. - Transactions, persistence, and writes: Iceberg INSERT/OVERWRITE/DELETE/UPDATE/MERGE/REWRITE planning and transaction boundaries were reviewed. No Doris EditLog or new persistent state is introduced. Existing live threads already cover the outstanding Iceberg generation and empty-base write correctness concerns.
- FE/BE propagation: No new session variable or thrift field is introduced. The changed Struct function result layouts remain executable across mixed versions through the existing compatibility and FE-layout materialization paths.
- Performance and remaining risks: Clear-mask allocation, collection compaction, COW detachment, and duplicate Arena-copy paths were checked; no new non-duplicate performance issue remains. The final missed-risk sweep found no additional actionable issue beyond MAIN-2 and MAIN-3 and the already-open live threads.
| } | ||
| RETURN_IF_ERROR(_align_column_nullability(column, mapping.table_type)); | ||
| RETURN_IF_ERROR( | ||
| _align_column_nullability(column, mapping.table_type, nullable_parent_null_map)); |
Contributor
There was a problem hiding this comment.
[P1] Project inherited masks for trivial collection children
This inherited mask is still lost when mapping.is_trivial and the mapped child is an ARRAY or MAP. For example, a nullable Struct can be rematerialized because one sibling changed while an identical collection sibling remains trivial; this call passes the Struct row mask into _align_column_nullability, whose ARRAY/MAP branches recurse into entries without projecting that row-coordinate mask through the offsets. A NULL Struct row with a populated physical collection can then expose a required descendant's hidden NULL placeholder and fail with Default expression produced NULL for non-nullable table column. Please project the inherited mask at the ARRAY/MAP branches too, and add a mapper-driven test with a non-trivial Struct parent and a trivial collection child.
| // the field nullable here would reject safe casts into required target fields. | ||
| structFields.add(new StructField(nameLiteral.getStringValue(), | ||
| children.get(i + 1).getDataType(), true, "")); | ||
| children.get(i + 1).getDataType(), children.get(i + 1).nullable(), "")); |
Contributor
There was a problem hiding this comment.
[P2] Keep strict pre-cast Struct children required
This now copies the child expression's nullability, but Cast.nullable() is still mode-blind. With enable_strict_cast=true, named_struct('metric', CAST(required_string AS INT)) therefore declares metric nullable because STRING-to-INT can return NULL in loose mode, even though strict execution aborts bad rows and returns a required INT for successful rows. A subsequent strict cast or INSERT into STRUCT<metric:INT NOT NULL> is rejected by CheckCast's declared-source-nullable guard before its new strict exemption is reached. Please make scalar-Cast/Struct-child nullability strict-aware (while keeping TryCast nullable), and cover pre-cast children in both struct and named_struct.
morningman
approved these changes
Jul 31, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
reviewed
Jul 31, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
FE review at head 312272e732dd0e7ace0111060b0611577749eca5.
The overall direction is sound: statement-frozen Iceberg read metadata is now separated from mutable write operations, historical predicates use the pinned schema, unsupported metadata-table snapshot selection is fenced, and most Struct requiredness/coercion paths are covered.
I found one additional planning-cost issue inline. I did not duplicate these three existing actionable threads, which still apply to the current head:
- Empty-table MERGE loses the distinction between an explicitly empty read and no read pin: #66007 (comment)
- Nested DESCRIBE comments are not SQL-literal quoted: #66007 (comment)
- Strict pre-cast Struct children remain incorrectly nullable: #66007 (comment)
The earlier unresolved FE threads about nested NOT NULL display, non-strict fallible required-field casts, legacy CASE/set-operation nullability, frozen read generations, and strict common Struct types are fixed by later commits and their added tests.
| } | ||
|
|
||
| private static Column connectorSinkTargetColumn(PluginDrivenExternalTable table, String name) { | ||
| return sinkTargetFullSchema(table).stream() |
Contributor
There was a problem hiding this comment.
[P2] Resolve the latest target schema only once per sink bind
connectorSinkTargetColumn calls sinkTargetFullSchema(table) for every user column and every static-partition name, then linearly scans the returned list. An explicit list of M columns over an N-column table therefore becomes O(M×N), replacing the previous map lookup.
The more expensive case is row-level DML: PluginDrivenExternalTable.getFullSchema(Optional.empty()) passes through appendSyntheticWriteColumns; while the hidden-column gate is open, that calls fetchSyntheticWriteColumns, resolves the connector handle again, and the Iceberg getTableHandle implementation executes catalogOps.tableExists. A wide DML statement can consequently repeat a remote catalog existence probe per target column.
Please load the latest target schema once at the start of sink binding, build one case-insensitive name map, and pass that schema/map through canonicalization, explicit-column selection, and static-partition materialization. An invocation-count test should verify that one bind calls getFullSchema(Optional.empty()) only once regardless of the number of target columns.
924060929
approved these changes
Jul 31, 2026
yiguolei
approved these changes
Jul 31, 2026
hubgeter
added a commit
to hubgeter/doris
that referenced
this pull request
Jul 31, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65851, apache#66007 Problem Summary: Master introduced frozen historical reads, current-schema branch writes, nullable parent-mask propagation, and ORC ownership fixes while this PR adds Iceberg V3 initial and write defaults. Resolve the overlapping FE and BE paths semantically: keep historical reads separate from mutable writer metadata, pin the current table writer schema while targeting a branch lineage, compose parent masks with recursive default materialization, and retain both test suites. ### Release note Branch-targeted Iceberg writes use the table current schema while committing to the selected branch lineage. ### Check List (For Author) - Test: Unit Test - 262 focused Iceberg connector tests passed - 34 focused FE sink and row-level DML tests passed - C++ format and connector import-boundary checks passed - Behavior changed: Yes (branch writes use the table-global current schema, matching current master) - Does this need documentation: Yes (updated docs/iceberg-v3-default-values-design.md)
4 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
6 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 problem does this PR solve?
Issue Number: N/A
Problem Summary:
The connector-plugin refactor on current master already provides the generic relation-local MVCC framework, per-statement connector metadata funnel, snapshot-aware Iceberg/Paimon/Hudi handles, and connector write contexts. The former cross-connector planning and frozen-table implementation from this PR is therefore no longer needed.
The remaining problems on current master are narrower:
What is changed?
Iceberg historical reads
Nested schema evolution
ORC safety
Tests
Behavior change
Historical Iceberg reads now remain bound to the selected schema generation. Nested requiredness is preserved through FE planning and BE materialization; physical NULL placeholders are accepted only when an enclosing nullable value hides them.
Release note
Fix Iceberg historical-read metadata consistency and preserve nested field requiredness during external-table schema evolution.
Check List (For Author)
Verification