[fix](iceberg) Honor partial name mappings for legacy files#65784
Open
Gabriel39 wants to merge 7 commits into
Open
[fix](iceberg) Honor partial name mappings for legacy files#65784Gabriel39 wants to merge 7 commits into
Gabriel39 wants to merge 7 commits into
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Legacy Iceberg Parquet and ORC files without field IDs are resolved through name mapping. With a partial mapping, FE omitted the optional mapping metadata for unmapped fields, so both V1 and V2 BE readers fell back to the current physical name and read unrelated data. Preserve table-level mapping presence by transporting explicit empty per-field lists and make those lists authoritative in both readers, so unmapped fields materialize their default or NULL while mapped aliases and scans without name mapping retain their existing behavior.
### Release note
Fix reads of migrated Iceberg files with partial name mapping so fields omitted from the mapping materialize their default value or NULL instead of matching a physical column by the current name.
### Check List (For Author)
- Test: Unit Test
- ExternalUtilTest
- Focused V1 and V2 name-mapping BE unit tests under ASAN
- Behavior changed: Yes. Partial Iceberg name mappings are now authoritative for legacy files without field IDs.
- Does this need documentation: No
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 18, 2026 13:03
Contributor
TPC-H: Total hot run time: 28998 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed against the current head after three convergence rounds. Two blocking correctness issues remain.
Checkpoint conclusions:
- Goal/minimality: the patch fixes top-level non-empty partial mappings, but the same authoritative-presence contract is incomplete for nested V2 projections and for a present globally empty mapping. The changes are otherwise focused.
- Flow/parallel paths: traced FE extraction and Thrift transport through V1 Parquet/ORC, V2 field-id/BY_NAME mapping, nested projection, equality-delete hidden dependencies, and position-delete/deletion-vector paths. No additional reachable defect survived convergence.
- Compatibility/state: no wire type, persisted format, configuration, concurrency, locking, lifecycle, or ownership contract changes were introduced. Mixed-version descriptors retain the pre-existing behavior rather than creating a new serialization incompatibility.
- Tests: the added tests cover scalar top-level non-empty partial mappings, but not whole/pruned nested materialization or absent-versus-present-
[]transport/results. - Performance/observability: the added metadata bit has negligible cost and is included in debug output; no per-row hot-path regression was found.
- Validation: static review only, as required by the review task. Remote compile, CheckStyle, clang-format, license, secret, and dependency checks pass. The macOS BE UT workflow failed because it selected JDK 25 while the build requires JDK 17; Doris BE/FE and regression jobs were still running at submission.
Requesting changes because both findings can silently bind unrelated legacy values instead of producing the Iceberg initial default/NULL.
Contributor
TPC-DS: Total hot run time: 177114 ms |
Contributor
ClickBench: Total hot run time: 24.91 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29403 ms |
Contributor
TPC-DS: Total hot run time: 177192 ms |
Contributor
ClickBench: Total hot run time: 25.07 s |
Contributor
There was a problem hiding this comment.
Requesting changes: two P1 correctness blockers remain.
Critical checkpoint conclusions
- Goal: The patch correctly preserves authoritative Iceberg mapping presence for ordinary same-version ID-less legacy reads, but it does not preserve matching IDs in mixed-ID files and is not semantically stable across the supported rolling-upgrade phases.
- Scope and minimality: The implementation is otherwise focused on FE transport plus V1/V2 name matching; the two existing review threads for valid root
[]presence and nested presence propagation are fixed on the current head. - Concurrency: No new threads, shared mutable state, locks, or lock-order concerns are introduced.
- Lifecycle: No static-initialization, ownership, or special resource-lifecycle behavior changes.
- Configuration: No configuration item is added or changed.
- Compatibility:
TField.name_mappingremains wire-decodable, but its interpretation changes without version negotiation. Old/new BEs can split one scan during BE-first rollout, and old/new FEs can produce different plans during the following FE rollout (second inline finding). - Parallel paths: FE regular/system-table schemas, hidden equality-delete dependencies, V1/V2, Parquet/ORC, whole/pruned complex fields, and position-delete paths were traced. V1 and V2 both globally downgrade mixed-ID schemas and lose an authoritative matching ID (first inline finding).
- Conditional logic: The new mapping-presence condition is documented and propagated through the changed builders, but the surrounding all-fields-have-IDs condition is too coarse for the strict branch. No other distinct conditional-path issue survived review.
- Test coverage: Added FE and BE unit tests cover absent, empty, partial, and nested presence/helper state, but miss mixed-ID Parquet/ORC behavior, old-FE/mixed-BE and mixed-FE execution, and the resulting observable value/default behavior.
- Test results: No regression output file is changed. This review was static-only as required by the task; no local build or test was run.
- Observability: Existing parse-failure logging is sufficient for the new FE parse path; the accepted failures are silent wrong-result cases that require correctness fixes rather than additional logging.
- Persistence and transactions: No EditLog, persisted format, transaction, or failover behavior changes.
- Data writes: This is a read-path change and introduces no write atomicity or crash-recovery concern.
- FE/BE state transport: All same-version mapping send/copy sites were checked. The existing optional list and new local presence bit are propagated, but the missing semantic version is the rolling-compatibility blocker above.
- Performance and memory: The metadata boolean and small mapping copies add no material CPU, memory, allocation, or hot-path concern.
- Other issues, focus, and completion: No additional user focus was supplied. After three normal/risk-focused convergence rounds, every candidate is accepted, deduplicated, or dismissed and all final reviewers returned
NO_NEW_VALUABLE_FINDINGSon this exact two-comment set. The full review is complete.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes: three P1 correctness blockers remain.
Critical checkpoint conclusions
- Goal and tests: The patch moves V1 and V2 toward Iceberg's any-ID rule and preserves explicit mapping presence, but ID-less nested wrappers still diverge by scanner/format, nested binary initial defaults are lost during projection, and an unchanged V2 equality-delete test is deterministically contradicted by the new mode. The added tests primarily assert helper or transport state and do not cover these observable results.
- Scope and minimality: The implementation is focused on FE mapping extraction/Thrift transport and BE matching. No unrelated production changes were found.
- Concurrency, lifecycle, configuration, persistence, and writes: No new thread, lock, ownership, startup/shutdown, configuration, persisted-state, transaction, or data-write contract is introduced.
- Compatibility and protocol: The optional Thrift presence marker is produced and consumed consistently on same-version paths, and absent old-FE state retains legacy behavior. The mixed-version semantic issue remains covered by the existing rolling-upgrade thread and is not duplicated here.
- Parallel paths: V1/V2, Parquet/ORC, whole/pruned complex projections, schema-history reconstruction, hidden equality-delete dependencies, and both position-delete paths were traced. The position-delete candidate was dismissed because supported Iceberg position-delete schemas require fixed/matching IDs and the metadata-table scan does not transport the claimed authoritative mapping state.
- Conditional and null/default behavior: V1's per-struct any-ID condition and V2's global current-node matcher do not implement each format's wrapper projection contract. Separately, recursive Base64 initial-default metadata is dropped before a newly missing nested child is materialized.
- FE/BE transport: Mapping authority and nested Base64 defaults reach BE schema reconstruction. Authority now survives the changed access-path copy, but the nested default value/encoding metadata does not, which causes the second inline failure.
- Error and nullability handling: No new status-propagation defect survived review. The accepted failures are silent wrong-result/default-materialization cases rather than missing diagnostics.
- Observability: Existing debug-state additions are adequate; no separate logging or metric concern was found.
- Performance and memory: Recursive schema scans and one metadata boolean are planning/setup work with negligible cost; no per-row hot-path or allocation regression was found.
- User focus and completion: No additional user focus was supplied. After the final wording correction, all three reviewers independently returned
NO_NEW_VALUABLE_FINDINGSon this exact three-comment set and anchors. - Validation: Static review only, because the review task prohibited builds and test execution. At submission, formatting, CheckStyle, license, dependency, secret, and Cloud UT checks pass; BE UT, FE UT, compile, code-review, and performance checks are pending. The macOS BE UT workflow failed during environment setup because it selected JDK 25 while the build requires JDK 17.
The review remains blocking because all three findings can change returned Iceberg row values or leave the checked-in reader test contract inconsistent with the implementation.
Contributor
TPC-H: Total hot run time: 29674 ms |
Contributor
TPC-DS: Total hot run time: 177702 ms |
Contributor
ClickBench: Total hot run time: 25.72 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes: five P1 correctness blockers remain.
Critical checkpoint conclusions
- Goal and tests: The patch advances strict partial Iceberg name mappings, but it still returns wrong values for recursive ID-less Parquet wrappers, nested non-binary defaults, projected binary defaults, and schema-only binary additions; long VARBINARY defaults also have a use-after-free. The added unit tests do not prove these observable paths.
- Scope and minimality: The production changes are focused on FE schema transport and V1/V2 projection/default materialization. No unrelated production change was found.
- Concurrency and lifecycle: No new threads, shared mutable state, locks, lock-order, static-initialization, or service lifecycle behavior is introduced. Memory ownership is blocking, however: decoded VARBINARY data is retained through a borrowed
StringViewafter its backing string is destroyed. - Configuration, persistence, transactions, and writes: No configuration, EditLog, persisted storage format, transaction, failover, or data-write contract changes. This is a read-path change.
- Compatibility and FE/BE transport: The optional Thrift authority marker preserves old-FE fallback and is wire-compatible for BE-first rollout. Same-version default transport is incomplete: nested non-binary values are never serialized, ordinary scans can select an older schema for binary metadata, and projected roots discard correctly transported binary metadata.
- Parallel paths and conditionals: V1/V2, Parquet/ORC, whole/pruned complex projections, current/time-travel scans, and hidden equality-delete dependencies were traced. The Parquet ID-less-wrapper exception is applied only at the root; the recursive matcher remains strict. A separate V1 global-any-ID residual is real but already covered by r3609339720 and was not duplicated.
- Error and nullability handling: Invalid Base64 statuses are propagated and the new checks are invariant-oriented. The accepted failures instead silently materialize NULL/type-default/Base64 text or read freed storage, so logging cannot make them safe.
- Tests and validation: Existing additions cover mapping presence, a root ID-less wrapper, and four-byte
DataTypeStringdefaults. Missing coverage includes a second-level wrapper, nested INT/STRING/DATE defaults, projected top-level BINARY/FIXED/UUID results, a schema-only binary addition after the last data snapshot, and 16-byte VARBINARY ownership under ASan. Static review only; the task explicitly prohibited builds and test execution. - Performance and observability: The new schema-time recursion and metadata fields have negligible setup cost, and debug output covers mapping state. No material hot-path or observability issue survived review.
- User focus and completion: No additional user focus was supplied. After Round 2, both independent full reviewers and the risk-focused reviewer returned
NO_NEW_VALUABLE_FINDINGSon this exact five-comment set and anchors. The review is complete.
All five findings can change returned Iceberg values; the ownership defect can additionally crash or trip ASan.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 30037 ms |
Contributor
TPC-DS: Total hot run time: 177659 ms |
Contributor
ClickBench: Total hot run time: 25.33 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29916 ms |
Contributor
TPC-DS: Total hot run time: 178313 ms |
Contributor
ClickBench: Total hot run time: 25.68 s |
Contributor
There was a problem hiding this comment.
Request changes: three wrong-result gaps remain in the Iceberg legacy-schema compatibility work.
Findings:
- P1: an ordinary scan can combine statement-pinned S1 columns with binary-default markers from a cache-reloaded S2 table and return Base64 carrier text for an S1 STRING/CHAR-mapped binary field.
- P1: a valid authoritative
[]name mapping prevents both scanners from retaining an ID-less Parquet wrapper even when a selected descendant field ID proves the wrapper is physical. - P1: V1 loses that same kind of wrapper when an unrelated direct root has an ID, while V2 returns the physical descendant.
Critical checkpoint conclusions:
- Goal and proof: The PR correctly improves absent-versus-present mapping transport, recursive initial defaults, and several V1/V2 parity cases, but the three cases above show that the stated legacy-file correctness goal is not complete. Existing tests cover the ingredients separately, not these cross-products.
- Scope and clarity: The broad FE/Thrift/V1/V2/test scope is justified by the compatibility boundary and is otherwise focused. The remaining defects are localized to schema-source selection and Parquet wrapper matching.
- Concurrency: No new threads, locks, atomics, or lock-order risks were introduced. The relevant concurrent operation is external schema evolution plus metadata-cache invalidation during planning; the statement snapshot and later scan-node table generation can diverge as described in the first finding.
- Lifecycle: No static-initialization or resource-cycle issue was found. The non-intuitive lifetime is the statement-pinned schema ID versus the independently invalidatable Iceberg
Tableobject, which causes the first finding. - Configuration: No configuration item is added. The first wrong-result path applies when Iceberg binary logical types are mapped to Doris STRING/CHAR; VARBINARY mapping independently infers Base64 and is not affected by that specific decode symptom.
- Compatibility: Optional Thrift field 9 preserves old-plan decoding and supports the documented BE-first rollout. The already-raised mixed-FE semantic difference is not duplicated here; all three findings are reachable in a compatible new-FE/new-BE deployment.
- Parallel paths and conditions: I traced FE serialization through both scanners, Parquet versus ORC, top-level and recursive struct/array/map paths, access pruning, and equality-delete hidden keys. ORC intentionally excludes retained ID-less wrappers; the two wrapper findings are Parquet-specific omissions.
- Tests and results: Added unit tests and expected results are internally consistent, but they miss cache invalidation between MVCC capture and marker serialization, authoritative-empty mapping plus a retained wrapper, and the V1 unrelated-direct-ID wrapper result. No generated regression result changed.
- Observability and error handling: No new metric is required for schema-time mapping. New decode/parse statuses are checked and propagated; the reviewed paths do not silently discard errors.
- Transactions, persistence, and writes: No Doris EditLog, transaction, persistence, storage-format write, visible-version, or MoW path is changed.
- FE-BE variables: Mapping authority and recursive default/Base64 metadata are transported through the intended paths. The second finding is a BE consumption-order error after correct transport, not a missing wire field.
- Memory and nullable safety: Temporary variable-width defaults are copied into owning columns or literals before buffers expire; const/nullable materialization and repeated-batch row counts are consistent. No additional ownership defect was found.
- Performance: New work is schema/scan initialization work rather than per-row matching; no substantiated blocking CPU or memory regression was found.
- Other issues: No additional configuration, cleanup, data-correctness, observability, or test-result defect remained after the final whole-diff sweep. The user supplied no extra review focus.
Validation: Static review only, as required by the automated-review contract; I did not run local builds or tests. At finalization, compile, Cloud UT, style, license, secret, and dependency checks pass; macOS BE UT fails before the build because CI provides JDK 25 while the job requires JDK 17; the two coverage checks report zero-duration TeamCity failures; BE/FE UT and regression/performance jobs are still pending.
Review completion: Reviewed the unchanged head 3050fa2a2caedb13d7308fa74be4f06d4a42941e, all 27 changed files, required design/agent guidance, and all prior threads. Three independent reviewers performed normal and risk-focused convergence; every suspicious point is accepted above, dismissed with evidence, or deduplicated against an existing thread.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29366 ms |
Contributor
There was a problem hiding this comment.
Review summary
Requesting changes for two P1 correctness gaps:
- Statement-pinned Iceberg Columns can be combined with a refreshed name-mapping property, making a valid refreshed
[]unexpectedly authoritative for an already-bound statement and returning default/NULL instead of an ID-less same-name file value. - V1 Parquet can leave whole-file field-ID mode inside an already ID-matched struct and alias-bind an ID-less primitive child that Iceberg projection and V2 treat as missing.
No additional user-provided review focus was supplied.
Critical-checkpoint conclusions
- Goal and proof: The PR aims to make partial/present-empty name mappings authoritative and align V1/V2 missing-field defaults. It does not fully accomplish that goal because of the two blockers above. The new tests prove many intended branches but omit both exact reproductions.
- Scope: The production and test changes are related to the same FE-to-reader schema contract. The follow-up fixes are broad but cohesive; no unrelated source change was identified.
- Concurrency: No new threads or BE locking are introduced. The relevant concurrent lifecycle is FE statement binding versus external metadata-cache invalidation/property refresh, and M1 violates that statement boundary.
- Lifecycle/static initialization: Reader-owned columns/literals have bounded lifetimes and the changed variable-width default carriers are owned correctly. No static-initialization or release-order issue was found; the lazy
Tableversus statement snapshot lifetime is the M1 blocker. - Configuration: No configuration item is added or changed.
- Compatibility: The optional Thrift authority bit preserves old-FE/new-BE fallback and is ignored by old BEs; no storage-format or symbol incompatibility was found. The already-raised mixed-rollout concern was not duplicated.
- Parallel paths: FE serialization, V1 Parquet/ORC, V2 Parquet/ORC, nested struct/array/map projection, equality deletes, position deletes, virtual columns, and access-path pruning were checked. M2 remains only in V1 Parquet; the analogous ORC recursion includes the matched parent ID.
- Conditional checks: Present-empty mapping authority, existential any-ID selection, and Parquet's unique complex-wrapper exception were traced. M2 incorrectly recomputes the any-ID condition at a nested struct.
- Test coverage: Tests cover absent/present-empty/partial mappings, mixed IDs, wrapper retention, nested/default metadata, binary ownership, and ordinary equality-delete defaults. They lack the property-refresh race and the ID-matched-parent/all-ID-less-child V1 reader result.
- Test-result changes: No regression
.outfile is changed. Static inspection found the modified unit expectations consistent with the cases they cover. - Observability: No new metric or high-volume log is needed for these plan/setup paths; existing status/error propagation is adequate for the changed decoding paths.
- Transactions/persistence: No Doris transaction, EditLog, or persisted-state path is modified.
- Data writes: The PR changes read planning/materialization only; no write atomicity or crash-recovery path applies.
- FE-BE variables: The new authority marker and initial-default metadata are serialized and copied through root and nested paths. The missing piece is pinning mapping property presence/content to the FE statement generation.
- Performance: Added work is schema/setup traversal rather than per-row processing; no material CPU or memory regression was found.
- Other issues: Error handling, nullable/const materialization, Base64 ownership, delete paths, and duplicate coverage were reviewed. A dropped equality-delete-key limitation was identified but excluded because the aggregate diff does not introduce or worsen it.
Validation was static only, as required by the review contract; no local build or test command was run. At submission time COMPILE, CheckStyle, both Clang Formatter checks, Cloud UT, and vault_p0 pass. FE UT fails, but its TeamCity log requires authorization from this runner; macOS BE UT fails before compilation because that runner supplied JDK 25 while the job requires JDK 17. BE UT and several regression/performance jobs remain pending.
Contributor
TPC-DS: Total hot run time: 177941 ms |
Contributor
ClickBench: Total hot run time: 25.11 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
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
2 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
Related PR: N/A
Problem Summary: Legacy Iceberg Parquet and ORC files without field IDs are resolved through name mapping. With a partial mapping, FE omitted the optional mapping metadata for unmapped fields, so both V1 and V2 BE readers fell back to the current physical name and read unrelated data.
This change preserves table-level mapping presence by transporting explicit empty per-field lists and makes those lists authoritative in both readers. Unmapped fields now materialize their default or NULL, while mapped aliases and scans without name mapping retain their existing behavior.
Release note
Fix reads of migrated Iceberg files with partial name mapping so fields omitted from the mapping materialize their default value or NULL instead of matching a physical column by the current name.
Check List (For Author)
ExternalUtilTest(6 tests)