[fix](iceberg) Honor partial name mappings for legacy files#65784
Open
Gabriel39 wants to merge 3 commits into
Open
[fix](iceberg) Honor partial name mappings for legacy files#65784Gabriel39 wants to merge 3 commits into
Gabriel39 wants to merge 3 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.
| context->schema_column = build_schema_column_from_external_field(*schema_field, column->type); | ||
| column->identifier = context->schema_column->identifier; | ||
| column->name_mapping = context->schema_column->name_mapping; | ||
| column->has_name_mapping = context->schema_column->has_name_mapping; |
Contributor
There was a problem hiding this comment.
[P1] Preserve authoritative mapping state on nested projections
This preserves has_name_mapping only on the projected root. build_schema_column_from_external_field() does populate it recursively, but FileScannerV2 then rebuilds complex children through AccessPathParser::build_nested_children(), whose inherit_schema_metadata() copies only name_mapping. For s STRUCT<b INT> where nested field b has an explicit empty names list, both SELECT s and SELECT s.b therefore recreate b with has_name_mapping == false; an ID-less legacy file containing an unrelated physical s.b is then accepted by the current-name fallback instead of materializing the table child as NULL/default. Please copy the presence bit with the aliases in AccessPathParser and add whole/pruned nested coverage (including struct descendants under array/map).
| if (nameMapping != null && nameMapping.containsKey(dorisColumn.getUniqueId())) { | ||
| // for iceberg set name mapping. | ||
| root.setNameMapping(new ArrayList<>(nameMapping.get(dorisColumn.getUniqueId()))); | ||
| if (nameMapping != null && !nameMapping.isEmpty()) { |
Contributor
There was a problem hiding this comment.
[P2] Keep table mapping presence when the parsed mapping is empty
!nameMapping.isEmpty() still conflates “no Iceberg name-mapping property” with a present valid mapping whose JSON list is empty ([]). IcebergScanNode.extractNameMapping() returns an empty HashMap in both cases (and the bundled Iceberg 1.10.1 parser accepts an empty array), so this branch leaves TField.name_mapping unset for every current field. For an ID-less imported file, V1 and V2 then fall back to a same-current-name physical column instead of treating the field as missing and producing its initial default/NULL. Please preserve property presence separately from map contents (while retaining fallback when the property is absent) and add an absent-vs-[] transport/result test.
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.
| } | ||
| // An explicit empty or unmatched Iceberg mapping means the legacy field is absent; | ||
| // falling back to its current name would bind an unrelated physical column. | ||
| return false; |
Contributor
There was a problem hiding this comment.
[P1] Preserve ID matches when only part of a file schema has IDs
This strict return is used for every field once all_have_field_id sees any ID-less sibling (and V2 similarly downgrades the whole file through IcebergTableReader::_has_field_id). For a partial mapping with a (ID 1, names=[]) and a file containing a (ID 1) plus b (no ID), both scanners now enter name mode and materialize a as missing/default even though its physical ID matches. Iceberg 1.10.1's ParquetSchemaUtil.hasIds and ORCSchemaUtil.hasIds select ID projection when any field has an ID, so that ID must remain authoritative. Please preserve ID-first matching before applying strict empty/unmatched mappings in both V1 and V2 (including nested paths), and add mixed-ID Parquet/ORC coverage.
| } | ||
| if (lhs.has_identifier_name() && column_has_name(rhs, lhs.get_identifier_name())) { | ||
| return true; | ||
| if (!lhs.has_name_mapping) { |
Contributor
There was a problem hiding this comment.
[P1] Keep mapping semantics consistent through rolling upgrades
This matcher (and the parallel V1 early return) changes the meaning of the existing optional name_mapping field without a version signal. During Doris's documented BE-first rollout, the old FE already sends present-empty or unmatched mappings for explicit mapped-field entries; old BEs fall back to a matching current-name column while new BEs materialize missing/default, so one scan can combine unrelated values and NULL/defaults. After all BEs are new, the FE rollout still mixes old FEs that omit unmapped entries from partial/[] mappings with new FEs that send empty lists, making identical queries depend on the FE endpoint. Please version/gate this contract or enforce homogeneous compatible execution in both phases, and add old-FE/mixed-BE plus mixed-FE coverage.
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.
| bool all_have_field_id = true; | ||
| // Iceberg considers the schema ID-bearing when any field has an ID; requiring all IDs would | ||
| // discard authoritative matches merely because an unrelated sibling is ID-less. | ||
| bool has_field_id = false; |
Contributor
There was a problem hiding this comment.
[P1] Match ID-less wrapper projection to each file format
This computes the any-ID decision independently for each V1 struct, while V2 switches globally but matches only the current root ID. For a physical ID-less wrapper s containing legacy_a with ID 1, and table s ID 10 containing a ID 1, V1 name-matches the wrapper and returns a, whereas V2 cannot match root ID 10 and materializes the struct as missing/default. Iceberg Parquet retains the selected physical child and its nonempty wrapper; Iceberg ORC instead synthesizes the absent parent ID rather than reusing the physical wrapper, so V1's fallback is inconsistent there. Please make both scanner paths honor the format-specific projection contract, with Parquet/ORC whole and pruned nested result tests.
| column->name_mapping = schema_column->name_mapping; | ||
| // The presence bit is part of the mapping contract: an explicit empty mapping must remain | ||
| // authoritative after access-path pruning instead of enabling current-name fallback. | ||
| column->has_name_mapping = schema_column->has_name_mapping; |
Contributor
There was a problem hiding this comment.
[P1] Preserve nested Iceberg binary initial defaults
getBase64EncodedInitialDefaults() indexes nested UUID/BINARY/FIXED field IDs, ExternalUtil recursively writes the value and Base64 marker, and schema reconstruction restores them. This pruning copy then carries only aliases and authority. Once an explicit empty nested mapping correctly rejects a physical child, the V2 child mapping has no default expression and inserts the generic type default; V1's missing-child node does the same. An older file missing such an s.b therefore returns NULL/empty data instead of its decoded initial default. Please carry and materialize recursive initial defaults for struct/array/map children in both scanners, with binary/Base64 result coverage.
| } | ||
| if (!field.has_identifier_field_id()) { | ||
| return false; | ||
| if (field.has_identifier_field_id()) { |
Contributor
There was a problem hiding this comment.
[P1] Reconcile the unchanged stale-ID equality-delete test
With this new existential switch, IcebergEqualityDeleteByNameIgnoresStaleFileFieldId can no longer produce its asserted [1, 3]. Its file has ID-less legacy_id=[1,2,3] plus stale_key=[100,200,300] carrying ID 0; table current_id also has ID 0. This helper now selects BY_FIELD_ID, so both projection and delete-key lookup bind to stale_key, value 2 deletes nothing, and the result is [100,200,300]. Please explicitly resolve this compatibility contract and update the fixture/expectation (or implement the supported exception); the current full V2 reader suite is deterministically inconsistent with the patch.
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 |
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)