[feature](iceberg) Support Iceberg V3 default values - #65851
Open
hubgeter wants to merge 36 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Found three blocking issues and am requesting changes.
Critical checkpoint conclusions
- Goal and behavior: The same-version initial-default design is coherent for fields retained in the selected schema, including nested struct/list/map values and binary-like types. A dropped equality-delete key loses the historical initial-default metadata that Iceberg requires for continued delete application, and recursive defaults can return backend-dependent values during a smooth BE upgrade.
- Tests and current checks: The fixture meaningfully distinguishes pre-evolution files, immutable initial defaults, later write defaults, and physical NULL/value rows across V1/V2 and Parquet/ORC. The required Check for secrets workflow nevertheless fails on the two new synthetic hex literals. Static review only: the review contract prohibited local builds and test execution.
- Scope and parallel paths: The patch is focused on Iceberg read-time projection. V1 and V2, Parquet and ORC, ordinary and historical/ref schema selection, predicate paths, and equality-delete integration were audited. The two semantic gaps are called out inline; no separate scanner-path divergence was found.
- Concurrency, lifecycle, and performance: Default and mapping state is reader-local, expression/mapping cleanup is paired across split/EOF/abort paths, and typed constants are prepared once and expanded with column operations. No additional concurrency, resource-lifetime, or asymptotic performance issue was found.
- Configuration and compatibility: No configuration default changes. The new Thrift fields are wire-tolerable, but old source BEs do not implement the recursive semantics and remain query-eligible; that mixed-version semantic incompatibility needs an explicit gate or compatible carrier.
- Conditionals and error propagation: Physical values and physical NULLs correctly take precedence, optional missing fields without defaults become NULL, and required missing fields fail. For a dropped equality key, however, V1 errors for absent metadata while V2 silently synthesizes NULL and can retain a deleted row.
- Coverage and observability: Existing tests cover the main same-version matrix, but not add-with-default -> equality delete -> schema-only drop or mixed-version scheduling. Debug metadata is useful and no logging/profile regression was found.
- Persistence and FE/BE contract: This is read-only and adds no Doris persistent-state migration. Recursive FE-to-BE field/default metadata agrees for retained fields; the missing historical equality dependency is the contract hole.
- Other/user focus: No additional user-provided focus and no other substantiated non-duplicate issue after two convergence rounds.
Contributor
TPC-H: Total hot run time: 29521 ms |
Contributor
TPC-DS: Total hot run time: 177792 ms |
Contributor
ClickBench: Total hot run time: 25.2 s |
hubgeter
force-pushed
the
feature/iceberg-v3-defaults
branch
from
July 22, 2026 18:21
e2687d3 to
77a962e
Compare
hubgeter
added a commit
to hubgeter/doris
that referenced
this pull request
Jul 22, 2026
### What problem does this PR solve? Issue Number: close apache#65806 Related PR: apache#65851 Problem Summary: Iceberg V3 distinguishes initial defaults used to read older files from write defaults used for new writes. Doris previously returned NULL for fields absent from a data file and did not propagate nested field defaults. This change carries Iceberg initial-default metadata through FE and Thrift, materializes missing primitive and complex values independently in File Scanner V1 and V2, preserves physical value and NULL precedence, and adds Spark-Iceberg generated coverage without expanding Doris Iceberg type mappings. ### Release note Support reading Iceberg V3 initial defaults for missing primitive and nested fields in File Scanner V1 and V2. ### Check List (For Author) - Test: Regression test / Unit Test - Regression test - Unit Test - Behavior changed: Yes. Missing Iceberg fields now use their schema initial defaults; physically stored values and NULLs are unchanged. - Does this need documentation: Yes. Design and rollout notes are included in this commit.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Requesting changes for two distinct equality-delete correctness regressions that can silently retain deleted rows.
Critical checkpoint conclusions
- Goal and behavior: The current head now carries historical metadata and gates recursive initial-default semantics, addressing the three earlier review threads. Two supported schema-evolution paths remain incorrect: V1 loses a dropped delete key's stable field ID when a new field reuses its name, and FE infers history from numeric schema-ID order even though Iceberg defines schema IDs as unique identifiers and stores schemas as a metadata list (https://iceberg.apache.org/spec/#schema-evolution, https://iceberg.apache.org/spec/#table-metadata).
- Scope and parallel paths: All 35 changed files and their relevant callers were reviewed. V1/V2, Parquet/ORC, ordinary and historical/ref selection, batch and non-batch delete planning, projected and hidden keys, name and field-ID mapping, and FE/BE carrier construction were checked. The V1 issue exists in both Parquet and ORC; the history-order issue reaches Doris's V2 direct-name fallback for ID-less files without an authoritative name mapping. With an explicit
schema.name-mapping.default, Iceberg's projection rule is authoritative and the stale carrier name does not cause this example (https://iceberg.apache.org/spec/#column-projection). - Concurrency and lifecycle: Reader-local mapping/default state, expression prepare/open/close behavior, split/block reuse, async batch delete planning, and cleanup paths were inspected. No additional concurrency, ownership, or lifecycle defect was substantiated.
- Configuration and upgrade compatibility: No configuration-default change was introduced. The additive Thrift fields and explicit scan-semantics capability gate address the already-raised rolling-upgrade concern; no separate current-head mixed-version issue was found.
- Conditionals and error propagation: Physical values and NULLs take precedence over defaults, missing optional fields can materialize defaults/NULL, and malformed unsupported values return errors. The two inline cases bypass the intended stable-ID semantics and silently produce a nonmatching delete value, which is why they are blocking.
- Tests and validation: The changed regression/unit coverage exercises retained fields, nested values, immutable initial defaults, binary-like encodings, and V1/V2 Parquet/ORC. It does not cover drop/re-add with the same name in V1 or non-monotonic schema IDs with rename/delete/drop in the no-mapping direct-name fallback. Static review only: the governing task prohibited local builds and test execution. At the final check, formatter, checkstyle, secret, license, and dependency checks were successful; BE/FE/Cloud/compile/performance and macOS jobs were still pending.
- Persistence, transactions, and data writes: This is a read-path change with no Doris metadata migration, transaction-boundary change, or new data-writing behavior. Snapshot/ref schema pinning and MVCC selection showed no separate issue.
- API/protocol and compatibility: Field IDs remain the required identity boundary. The FE-to-BE carrier is additive and source-compatible, but both inline bugs violate the Iceberg schema/equality-delete contract within the new behavior.
- Performance and observability: Historical metadata construction and typed constant expansion add bounded per-scan/per-reader work; no additional blocking asymptotic or profiling/logging regression was substantiated. Existing errors remain visible, while the two reported paths are especially serious because they return wrong rows without an error.
- Other and user focus: No user-provided focus points were present. The full changed-file sweep, refreshed live-comment deduplication, and independent convergence passes found no other valuable nonduplicate findings.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29253 ms |
Contributor
TPC-DS: Total hot run time: 177624 ms |
Contributor
ClickBench: Total hot run time: 25.7 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
hubgeter
added a commit
to hubgeter/doris
that referenced
this pull request
Jul 22, 2026
### What problem does this PR solve? Issue Number: close apache#65806 Related PR: apache#65851 Problem Summary: Iceberg V3 distinguishes initial defaults used to read older files from write defaults used for new writes. Doris previously returned NULL for fields absent from a data file and did not propagate nested field defaults. This change carries Iceberg initial-default metadata through FE and Thrift, materializes missing primitive and complex values independently in File Scanner V1 and V2, preserves physical value and NULL precedence, and adds Spark-Iceberg generated coverage without expanding Doris Iceberg type mappings. ### Release note Support reading Iceberg V3 initial defaults for missing primitive and nested fields in File Scanner V1 and V2. ### Check List (For Author) - Test: Regression test / Unit Test - Regression test - Unit Test - Behavior changed: Yes. Missing Iceberg fields now use their schema initial defaults; physically stored values and NULLs are unchanged. - Does this need documentation: Yes. Design and rollout notes are included in this commit.
hubgeter
force-pushed
the
feature/iceberg-v3-defaults
branch
from
July 22, 2026 21:07
77a962e to
68e53f0
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Requesting changes for three distinct current-head issues: one V2 equality-delete correctness gap and two rolling-upgrade compatibility regressions. The detailed scenarios and fix directions are attached inline.
Critical checkpoint conclusions
- Goal and proof: the PR broadly implements Iceberg V3 initial defaults across FE, V1/V2 Parquet and ORC, and equality deletes, with substantial unit and regression coverage; M1 and M2 show that the goal is not yet met for expired lineage and mixed-version execution, while M3 exposes an availability regression in the compatibility fix.
- Scope, clarity, and reuse: the production changes are cohesive and mostly reuse the existing schema mapper, expression, and scanner abstractions. The remaining history and capability decisions are not safely bounded.
- Concurrency: batch split planning is asynchronous, but the scan params/carrier are shared before BE execution and the checked backend collection is the same one used by batch and non-batch assignment. No new data race, lock-order, or blocking-under-lock issue was found.
- Lifecycle and memory/nullability: V1 prepared defaults and V2 mapping expressions are reader/file scoped, opened and reset consistently; variable-width defaults are materialized into owning columns; parent NULL and physical NULL precedence are preserved. No separate lifecycle or ownership issue was found.
- Configuration: no production configuration item was added.
- Compatibility: the additive Thrift fields are wire-compatible, but semantic mixed-version behavior is not: M2 lacks a capability/version boundary, and M3 rejects pruned queries that do not need new BE behavior.
- Parallel paths and conditionals: V1 Parquet/ORC, V2, BY_FIELD_ID/BY_NAME, batch/non-batch, current/time-travel/branch paths, and required/optional/default-vs-physical-NULL cases were traced. M1 is the V2-only hole in a fallback already present in both V1 formats.
- Tests and results: the PR adds FE, V1, V2, fixture, and four scanner/format regression lanes with deterministic ordered output, but no test covers M1-M3. Per the governing review prompt, no local build or test was run;
.worktree_initialized,thirdparty/installed, andprotocare absent. At review time style, secret, license, dependency, and large-file checks passed, while compile and unit/performance jobs were still pending. - Observability: parser and required-field failures propagate with status; no additional metric/logging requirement was identified for this read path.
- Transactions, persistence, and writes: production changes are read-path/schema-carrier changes only; no EditLog, transaction, or storage-write protocol is involved.
- FE/BE variables: recursive field metadata, optionality, binary markers, and serialized defaults are carried consistently and the two BE parsers agree; the missing rollout capability in M2 is the outstanding transport/version concern.
- Performance: defaults are parsed/prepared at mapping/reader boundaries rather than per row, and no new hot-loop allocation or material performance issue was found.
- Other issues and user focus: all five earlier inline threads were treated as known context and not duplicated. There was no additional user-provided review focus, and the final 35-file sweep found no fourth substantiated issue.
Validation was static-only as required by the review prompt.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29432 ms |
Contributor
TPC-DS: Total hot run time: 177423 ms |
Contributor
ClickBench: Total hot run time: 26.03 s |
hubgeter
added a commit
to hubgeter/doris
that referenced
this pull request
Jul 23, 2026
### What problem does this PR solve? Issue Number: close apache#65806 Related PR: apache#65851 Problem Summary: Iceberg V3 distinguishes initial defaults used to read older files from write defaults used for new writes. Doris previously returned NULL for fields absent from a data file and did not propagate nested field defaults. This change carries Iceberg initial-default metadata through FE and Thrift, materializes missing primitive and complex values independently in File Scanner V1 and V2, preserves physical value and NULL precedence, and adds Spark-Iceberg generated coverage without expanding Doris Iceberg type mappings. ### Release note Support reading Iceberg V3 initial defaults for missing primitive and nested fields in File Scanner V1 and V2. ### Check List (For Author) - Test: Regression test / Unit Test - Regression test - Unit Test - Behavior changed: Yes. Missing Iceberg fields now use their schema initial defaults; physically stored values and NULLs are unchanged. - Does this need documentation: Yes. Design and rollout notes are included in this commit.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28712 ms |
Contributor
There was a problem hiding this comment.
Request changes: three new P1 correctness issues remain on the current head: V2 can abort on a wholly absent nested-key root, branch writes analyze the wrong schema, and V1 Parquet can ignore a usable descendant field ID. Details and fix directions are inline.
Critical checkpoint conclusions:
- Goal and behavior: The PR broadly implements Iceberg V3 initial/write defaults, recursive reads, equality-delete evolution, and pinned write metadata across FE and both BE reader stacks. The three inline cases leave valid branch writes incompatible with Iceberg/Spark, crash one V2 evolution path, or silently misapply a V1 equality delete.
- Scope and parallel paths: All 78 authoritative changed paths were reviewed. V1/V2, Parquet/ORC, current/ref/history schemas, projected and hidden fields, INSERT/OVERWRITE/UPDATE/MERGE, batch/non-batch planning, and commit replay were traced. No fourth non-duplicate issue survived the final sweep.
- Conditions and error handling: Required/optional checks, physical NULL precedence, recursive defaults, and commit validation generally fail or propagate correctly. MAIN-F1 reaches a hard check through an unmaterialized constant, while MAIN-F3 selects the wrong identity mode and can return wrong rows instead of failing.
- Lifecycle and concurrency: Reader-local expressions/defaults, statement-scoped writer contexts, async planning state, transaction refresh, and final metadata CAS have bounded ownership and consistent cleanup. No additional race, lock-order, lifetime, or commit-bypass defect was substantiated.
- Compatibility and FE/BE propagation: The semantics marker and additive Thrift metadata cover the inspected mixed-version paths. MAIN-F2 is instead an engine interoperability regression: branch writes no longer use the branch-head schema exposed by Iceberg 1.10.1 Spark and by Doris branch reads.
- Tests and validation: The changed FE, BE, fixture, and regression coverage is broad and deterministic where applicable, but it omits wholly absent complex equality roots, ID-less Parquet roots with ID-bearing renamed children, and the restored branch-head interoperability contract. Static review only: the governing prompt prohibited builds and test execution.
- Performance and observability: Default materialization and schema-history work are bounded at scan/reader scope. The remaining all-column ORC normalization cost is already covered by a live thread; no new performance or observability issue was found.
- Persistence, transactions, configuration, and docs: No Doris EditLog migration or new configuration item is introduced. Iceberg remains the atomic commit boundary and the current validation wrapper closes the checked replay races. Documentation is extensive, but the branch regression now contradicts the previously documented Spark-aligned behavior.
- Existing discussion and focus: The live historical-carrier, backslash, plugin-TCCL, and ORC-cost findings were treated as duplicates and not repeated. No additional review focus was supplied, so the whole PR was reviewed.
|
|
||
| std::vector<const NullMap*> ancestor_null_maps; | ||
| for (const size_t child_index : _child_indexes) { | ||
| if (const auto* nullable = check_and_get_column<ColumnNullable>(*current); |
Contributor
There was a problem hiding this comment.
[P1] Materialize missing struct defaults before nested lookup
When the whole top-level struct is absent from an old data file, data_path is empty and missing_root_expr is a VLiteral. VLiteral returns a ColumnConst, but this loop tests it directly as ColumnNullable/ColumnStruct; the nullable check misses the wrapper and the following DORIS_CHECK(struct_column != nullptr) aborts. This is reachable for an added optional payload STRUCT<k INT> followed by an equality delete on payload.k, for both a NULL struct and a non-NULL complex initial default. Existing tests cover only a physically present parent with a missing child. Please materialize current before traversing it and add forced V2 Parquet/ORC coverage where the entire struct is absent.
| Objects.requireNonNull(branchName, "branchName should not be null"); | ||
| // Iceberg schema evolution is table-global. A branch selects the snapshot lineage that receives | ||
| // the commit, while files written by that commit use the table's current schema. | ||
| Schema schema = table.schema(); |
Contributor
There was a problem hiding this comment.
[P1] Keep branch writes on the branch-head schema
This always pins table.schema(), but Iceberg 1.10.1's Spark connector exposes SnapshotUtil.schemaFor(table, branch) as the target table schema, and Doris branch reads do the same. If branch b is still on schema A after main advances to B, Doris now rejects valid A-shaped writes and accepts B-only columns/defaults that Spark does not expose for that branch. The regression was flipped to enforce this divergence even though the earlier current-schema request was explicitly withdrawn after the Spark contract was checked. Please restore branch-head schema resolution and keep the current-schema compatibility checks at commit time.
| } | ||
| const bool use_field_ids_for_hidden_keys = | ||
| supports_iceberg_scan_semantics_v1(&get_scan_params()) | ||
| ? any_file_column_has_field_id |
Contributor
There was a problem hiding this comment.
[P1] Detect descendant IDs before choosing hidden-key mapping
This mode flag inspects only top-level Parquet fields. With an ID-less payload wrapper whose renamed child still has field ID 2, ordinary projection enters ID mode via parquet_subtree_has_field_id() and correctly binds the wrapper through that descendant, but the equality-key path enters name mode. It then misses the old child name (or an authoritative empty name mapping) and synthesizes NULL/default instead of reading the physical key, so an applicable equality delete is missed or the scan rejects a required field. Please make the hidden-key decision recursive like BuildTableInfoUtil and add a V1 Parquet equality-delete test with an ID-less root and renamed ID-bearing child.
Contributor
TPC-DS: Total hot run time: 169320 ms |
Contributor
ClickBench: Total hot run time: 23.86 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
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: None Related PR: apache#65851 Problem Summary: Missing whole-struct defaults remained const during nested equality-delete lookup, V1 Parquet hidden keys ignored descendant field IDs, and branch writes used the table current schema instead of the branch-head schema. Materialize nested default literals while retaining nullable ancestor ownership, align hidden-key ID detection with recursive schema matching, and restore branch snapshot schema pinning with current-schema compatibility validation. ### Release note Fix schema-evolved Iceberg branch writes and nested equality-delete reads. ### Check List (For Author) - Test: - Unit Test: IcebergWritePlanProviderTest (46 tests); IcebergV2ReaderTest.IcebergMissingWholeStructEqualityKeyMaterializesDefault; IcebergReaderTest.v1_parquet_uses_descendant_id_for_hidden_nested_equality_key - Regression test: Not run locally because it requires an external Iceberg environment; restored the branch-head schema scenario and kept its generated output unchanged - Static check: build-support/check-format.sh passed; build-support/run-clang-tidy.sh was attempted but is blocked by existing toolchain and repository diagnostics, including missing stddef.h and an unmatched NOLINTEND - Behavior changed: Yes. Branch-targeted writes bind to the branch-head schema, and nested equality deletes resolve evolved defaults and descendant IDs correctly. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65851 Problem Summary: Bring the PR branch through apache/master at 6ee10af and resolve the ExecuteCommandTest conflict by preserving both the prepared connector schema refresh coverage and master MVCC and scan-option reset coverage. ### Release note None ### Check List (For Author) - Test: - Unit Test: IcebergWritePlanProviderTest (46 tests); ExecuteCommandTest (7 tests) - BE Unit Test: Not run for this intermediate merge; the final head will be validated after incorporating the latest apache/master - Static check: PR-specific diff check passed; the merge imports pre-existing whitespace diagnostics from apache/master regression outputs - Behavior changed: No. This merge preserves both branch and master behavior. - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#65851 Related PR: apache#65851 Problem Summary: Bring the branch up to the latest apache/master at d14c6b9. The two new upstream changes are unrelated to the Iceberg V3 default-value implementation and merged without conflicts. ### Release note None ### Check List (For Author) - Test: Final validation is pending on the fully merged tree - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#65851 Problem Summary: The latest review fixes lacked direct assertions for the surviving rows in the V1 descendant field-ID equality-delete case and for branch schema compatibility when current required fields differ from the pinned branch schema. Assert the exact retained row IDs and cover incompatible required additions, concurrent optional-to-required evolution, and compatible required additions with initial defaults. ### Release note None ### Check List (For Author) - Test: - Unit Test: Not run after this change because local UT compilation was explicitly stopped - Static check: git diff --check and build-support/check-format.sh passed - Behavior changed: No - Does this need documentation: No
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. |
Gabriel39
reviewed
Aug 3, 2026
| table.newAppend().commit(); | ||
| table.manageSnapshots().createBranch("old_schema", table.currentSnapshot().snapshotId()).commit(); | ||
| table.updateSchema().addRequiredColumn( | ||
| "required_value", Types.IntegerType.get(), Literal.of(7)).commit(); |
Contributor
There was a problem hiding this comment.
[P1] Create a format-v3 table before adding this initial default
unpartitionedUnsortedTable(catalog) creates the table with the default format version (v2), but Iceberg permits a non-null initial default only in v3. As a result, this test fails at this commit() before it exercises validateCurrentSchema():
Tests run: 1, Failures: 0, Errors: 1
Invalid schema for v2:
- Invalid initial default for required_value: non-null default (7) is not supported until v3
I reproduced this with IcebergWritePlanProviderTest#branchWriteAllowsCurrentRequiredFieldWithInitialDefault on the current head. Please create or upgrade this fixture as format v3 (the existing formatVersionThreeTable helper demonstrates the required property), then rerun the test so it validates the intended branch-compatibility behavior.
### What problem does this PR solve? Issue Number: None Related PR: apache#65851 Problem Summary: The branch compatibility test attempted to add a non-null initial default to a format-v2 Iceberg table, so Iceberg rejected the schema update before the intended validation path ran. Reuse the existing format-v3 table fixture and its matching table name so the test reaches and verifies branch schema compatibility. ### Release note None ### Check List (For Author) - Test: - Unit Test: Not run because local UT compilation remains explicitly disabled - Static check: git diff --check passed - Behavior changed: No - Does this need documentation: No
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29020 ms |
Contributor
TPC-DS: Total hot run time: 169674 ms |
Contributor
ClickBench: Total hot run time: 23.73 s |
Contributor
There was a problem hiding this comment.
Request changes on df33c9fac9e676ca89b64cd6384de60ee9aeac45: the current implementation still has two P1 blockers at the new scan boundaries and one P2 contract mismatch. I reviewed the complete 77-path aggregate diff and deduplicated against the supplied live threads, so already-raised issues are not repeated below.
Critical checkpoint conclusions
- Goal and proof: The goal is end-to-end Iceberg V3 initial/write-default correctness across schema evolution, equality deletes, branches, and V1/V2 Parquet/ORC paths. It is not yet achieved: complex read defaults do not have a producer/consumer-compatible typed carrier, and the plugin scan path reintroduces a projection-blind rolling-upgrade fence. The PR adds broad tests, but neither real FE-to-BE complex-default transport nor a pruned nested plugin projection is covered.
- Scope and clarity: The change is necessarily broad because it spans connector metadata, Nereids analysis, Thrift, both BE scanners, and Iceberg commits. The new statement/write-context abstractions make ownership clearer, but the duplicate legacy/plugin scan contracts and stale branch documentation show the result is not yet internally coherent.
- Concurrency: I traced statement-scope pinning, shared-table avoidance, fresh mutable table loads, concurrent schema changes, and final
TableOperationsvalidation. No additional distinct lock-order or concurrency issue survived deduplication; the remaining prepared-state, authenticator, and commit-replay concerns are already covered by live threads. - Lifecycle: Analysis, sink construction, transaction begin/replay/commit, scanner preparation, repeated batches, and cleanup paths were checked. No new lifecycle leak or cross-statement state issue was found beyond the existing threads.
- Configuration: No new Doris runtime configuration is added. Existing varbinary/timestamp mapping and writer properties are threaded into the pinned contexts, but the legacy binary mapping exposes the missing recursive type marker in the first inline issue.
- Compatibility and rolling upgrade: The scan-semantics marker and source-BE rejection cover result-changing reader behavior, but the plugin provider computes that requirement from full top-level subtrees instead of the resolved nested projection, causing avoidable query rejection for the full upgrade window.
- Parallel paths: V1/V2, Parquet/ORC, legacy/plugin scans, ordinary/time-travel/ref reads, and INSERT/UPDATE/DELETE/MERGE/overwrite paths were compared. The first finding affects both scanners at their shared FE carrier boundary; the second is a genuinely different reintroduction in the plugin-provider path after the legacy path was fixed.
- Special conditions: Required/optional fields, parent NULLs, explicit NULL/default, field-ID/name mapping, historical equality keys, branch advancement, and smooth-upgrade source backends were checked. Other substantiated edge cases already have live comments and were suppressed as duplicates.
- Test coverage: Unit, regression, Docker fixture, negative, and mixed-version coverage is extensive, but the changed tests handcraft the complex JSON/binary marker instead of exercising the production carrier, and the plugin test explicitly assumes all-descendant expansion rather than testing a pruned sibling.
- Test results: This was a static-only review; I did not build or run tests. The author reports broad focused suites and ASAN builds, while an existing current-head live thread reports a reproducible failure in
branchWriteAllowsCurrentRequiredFieldWithInitialDefaultbecause its fixture remains format v2. The claimed test status therefore is not presently sufficient proof. - Observability: New failures generally include field, table, schema, branch, or backend context. I found no distinct need for another metric/log beyond issues already raised; the main remaining failures are deterministic contract violations.
- Persistence, transactions, and failover: This does not add FE EditLog state. Iceberg metadata durability remains under Iceberg transaction/CAS semantics, and the pinned bundle plus validating operations wrapper addresses replay-time metadata drift. No additional distinct failover issue was found.
- Data-write atomicity and crash behavior: Schema/spec/order/file/metrics settings are pinned and revalidated before commit, and BE failures propagate as non-OK status. No new atomicity or crash-leak defect survived the existing-thread deduplication pass.
- FE-to-BE propagation: Recursive
TFieldmetadata and scan-semantics versioning are sent on the changed paths, but complex parent JSON and binary-leaf identity are not produced in the shape the new BE decoder consumes; this is a blocking protocol-contract gap. - Performance: The changed planning/history and ORC normalization paths were reviewed for bounds, copies, and per-row work. The known ORC normalization overhead already has a live P2 thread; no duplicate is added. The plugin gate issue is primarily an availability regression rather than CPU cost.
- Other and review focus: The durable design and the current PR release note still promise table-global current schema for branch writes, while current code/tests intentionally use branch-head schema. There was no additional user-provided review focus, so the full PR was reviewed without narrowing scope.
The three inline comments below are the complete new comment set for this head. All other verified concerns are either dismissed with code evidence or already represented by existing review threads.
| const auto primitive_type = value_type->get_primitive_type(); | ||
| if (is_complex_type(primitive_type)) { | ||
| rapidjson::Document document; | ||
| document.Parse(field.initial_default_value.data(), field.initial_default_value.size()); |
Contributor
There was a problem hiding this comment.
[P1] Send a complete typed complex-default carrier
This decoder assumes the parent initial_default_value is SingleValueParser JSON, but FE still sends every non-binary value through identity(...).toHumanString(), so complex defaults do not reach it in the required form. Legacy STRING/CHAR mapping also decodes binary leaves only when their child TField has initial_default_value_is_base64, while FE sets that flag only if the child itself has a default; list/map leaves normally do not. The new BE test manually sets the flag, masking production. Please serialize complex parents as Iceberg JSON, mark every UUID/FIXED/BINARY schema leaf by type, and add an FE-to-reader test.
| // that null to ImmutableSet.copyOf. The top-level id is already present, and only nested types | ||
| // have descendant ids to add. | ||
| if (field.type().isNestedType()) { | ||
| projected.addAll(TypeUtil.getProjectedIds(field.type())); |
Contributor
There was a problem hiding this comment.
[P1] Keep the smooth-upgrade gate projection-aware
PluginDrivenScanNode reduces each selected slot to an IcebergColumnHandle containing only the top-level field ID; it drops the slot's pruned type/access path. This line then expands that field to every descendant, so SELECT s.a is rejected whenever an unselected sibling s.b has an initial default and any source BE remains, even though BE never reads b. The legacy scan path was already fixed for this availability regression. Please carry the resolved nested projection into this provider and add a planner-level unselected-sibling test.
| 1. Omitted INSERT columns, explicit `DEFAULT`, reordered/multi-row VALUES, supported | ||
| `DEFAULT(column)`, and MERGE `NOT MATCHED INSERT` consume the typed `write-default` from the | ||
| statement-pinned current table schema. A branch target selects the snapshot lineage that receives | ||
| the commit; schema evolution remains table-global, so columns added or renamed after the branch |
Contributor
There was a problem hiding this comment.
[P2] Align the branch-schema contract with the implementation
The current provider now pins SnapshotUtil.schemaFor(table, branchHead), and the changed regression intentionally rejects current-only zone until the first successful old-schema branch write advances the ref. These criteria instead promise that post-branch additions/renames are immediately writable and that branch choice affects only the commit target. Please update this section and the read matrix/release note to state the branch-head schema behavior; otherwise this durable design documents the opposite of the behavior the PR implements and tests.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29196 ms |
Contributor
TPC-DS: Total hot run time: 169921 ms |
Contributor
ClickBench: Total hot run time: 25.31 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
5 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?
Related PR: #65329
Problem Summary:
Iceberg V3 separates
initial-default, used when an older data file does not contain a field ID, fromwrite-default, used when a writer omits a known field or explicitly requestsDEFAULT. Doris previously returned NULL for absent fields and did not consume Iceberg write defaults. Equality-delete history, mixed-version readers, filtered delete planning, and concurrent schema evolution could also produce inconsistent results.This change:
initial-defaultmetadata recursively by field ID and materializes missing primitive and complex values independently in File Scanner V1 and V2 for Parquet and ORC;write-defaultvalues for omitted columns, explicitDEFAULT, UPDATE, matched MERGE, and MERGE NOT MATCHED INSERT, preserving the referenced field inDEFAULT(column)and rejecting unknown references;CREATE/ALTER syntax for authoring or evolving Iceberg default metadata remains out of scope.
Release note
Support Iceberg V3 initial defaults when reading fields absent from older data files and write defaults for omitted columns or explicit
DEFAULTvalues when writing Iceberg tables.UPDATEand matched/not-matchedMERGEresolveDEFAULT(column)from the statement-pinned Iceberg write schema. Branch-targeted writes commit to the selected branch lineage while using Iceberg’s table-global current schema; columns added or renamed after the branch point are immediately part of the writer schema. Iceberg initial/write defaults remain separate from Doris column defaults, soDESCandSHOW CREATE TABLEdo not display either value as a generic Doris default.Check List (For Author)
Test
test_iceberg_initial_defaultsiceberg_branch_tag_schema_change_extendedtest_iceberg_schema_ref_actions_matrixtest_iceberg_partition_evolution_equality_delete./build.sh --fe --be -j16./run-be-ut.shgit diff --checkBehavior changed:
Does this need documentation?
docs/iceberg-v3-default-values-design.md.Check List (For Reviewer who merge this PR)