[fix](iceberg) Honor partial name mappings for legacy files#65784
Open
Gabriel39 wants to merge 12 commits into
Open
[fix](iceberg) Honor partial name mappings for legacy files#65784Gabriel39 wants to merge 12 commits into
Gabriel39 wants to merge 12 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 for four silent read-correctness gaps in the Iceberg legacy-file paths. The inline comments cover branch default-marker pinning, V1 ORC container recursion, V1 position_deletes file-wide ID selection, and V2 Parquet wrapper projection.
Critical checkpoint conclusions:
- Goal and proof: The PR aims to honor partial Iceberg name mappings and initial defaults consistently in V1/V2 legacy-file reads. It does not yet fully accomplish that goal because M1-M4 leave reachable schemas with carrier text, unrelated name-bound values, or synthesized NULL/defaults. The added tests are useful but omit those four shapes.
- Scope and clarity: The change is cohesive around schema mapping/default transport, though its duplicated V1/V2 and ordinary/system-table implementations require the parallel-path fixes called out inline.
- Concurrency: No new locks or concurrently mutated BE state were added. I checked statement binding versus cache refresh; ordinary mapping/default pinning is coherent, but M1 still selects branch marker metadata from a different schema generation.
- Lifecycle: No static-initialization or ownership cycle issue was found. The new variable-width default paths copy borrowed
StringViewdata into owning columns or literals while the decode buffer is alive. - Configuration: No new runtime configuration item is introduced.
- Compatibility: The optional Thrift mapping-authority marker preserves old-FE/new-BE fallback and is safe for the documented BE-first rollout. No additional storage-format or symbol incompatibility was found.
- Parallel paths: These are the main blockers: M2 loses file-wide ID mode in V1 ORC ARRAY/MAP recursion; M3 loses full-file evidence in the still-dispatched V1 position-delete reader; M4 omits Parquet wrapper projection in the V2 position-delete reader.
- Conditions: The authoritative absent/empty mapping distinction and any-ID conditions are documented and generally propagated, but the recursive/container and specialized-reader gates in M2-M4 are incomplete.
- Test coverage: Unit coverage spans mapping presence, defaults, mixed IDs, readers, and ownership. Missing cases are an old branch plus schema-only binary default, ORC array/map-contained ID-less leaves, V1 delete files with IDs only outside
row, and V2 delete rows with an ID-less Parquet wrapper. - Test results: I reviewed the changed assertions and expected values and found no incorrect expected result. Per the review environment contract, I did not run builds or tests, so this conclusion is static-only.
- Observability: Existing reader profiles/status errors and FE logging are sufficient for these metadata-read paths; no new metric or INFO log is required.
- Persistence and transactions: This PR changes read-side metadata caches and scan schemas, not EditLog, transaction, or data-write behavior; crash atomicity and master failover are not applicable.
- FE-BE variables: The new mapping-presence and Base64-marker metadata are sent through the production Iceberg serializer and consumed in both BE readers. Rolling compatibility is sound; M1 is a schema-selection defect rather than a missing serialization path.
- Performance: Schema walks and immutable mapping copies are metadata-sized; no material CPU, memory-accounting, or I/O regression was found.
- Other issues: After deduplication against all existing threads and convergence review, no additional actionable finding remains beyond the four inline comments.
User focus: no additional focus points were supplied; the full changed-file set was reviewed.
Validation: static review only; builds and tests were intentionally not run under the review contract.
Contributor
FE UT Coverage ReportIncrement line coverage |
16 tasks
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review status: complete; requesting changes for one P1 data-correctness issue.
The patch otherwise establishes the intended authoritative Iceberg name-mapping behavior across FE metadata pinning, the optional FE/BE protocol flag, V1/V2 Parquet and ORC readers, initial defaults, and data/equality/position-delete paths. The remaining issue is that V2 performs ID-less Parquet wrapper discovery using the access-path-pruned table subtree. If the projected child is absent from an older file and only an unprojected physical sibling supplies the descendant ID that identifies the wrapper, V2 drops the physical parent and returns NULL instead of the child's Iceberg initial default. V1 retains the wrapper from the complete schema and returns the expected result.
Critical checkpoint conclusions:
- Goal and proof: The implementation largely meets the legacy-file name-mapping goal, and the added FE/BE tests cover absent versus present-empty mappings, nested/default/binary cases, file-wide ID selection, both formats, and delete readers. The unprojected-sibling/default case in the inline finding is not covered and prevents full correctness.
- Scope and focus: Although the patch is broad because it closes multiple reader-generation and format-specific gaps, its changes remain focused on schema identity, metadata transport, default materialization, and corresponding tests.
- Concurrency and thread safety: No new mutable shared state, thread entry, lock, atomic, or lock-order dependency is introduced. Statement-pinned snapshot/cache values are immutable inputs to scan planning.
- Error handling: Changed
Statusresults are propagated, invariant-only checks fail loudly, and I found no swallowed error or speculative recovery path. - Memory safety and BE nullability: The binary/default paths copy temporary variable-width values into owned columns/literals, and complex rematerialization preserves nullable parent maps. I found no separate ownership, const/nullable-shape, or lifetime defect beyond the wrong missing-root choice in the inline finding.
- Lifecycle/static initialization: No new special destruction lifecycle, circular ownership, or cross-translation-unit static initialization dependency is introduced.
- Configuration: No configuration item or dynamic-reload behavior is added.
- Compatibility: The new Thrift field is optional and the absent-field fallback preserves legacy behavior. The previously discussed mixed-version capability caveat is already covered by existing review thread
r3609339722and was not duplicated here. - Parallel paths: V1/V2, Parquet/ORC, ordinary reads, equality deletes, and position-delete system-table readers were traced. They are otherwise aligned; the accepted finding is the remaining V2/V1 divergence.
- Special conditions: File-wide existential ID mode, authoritative mapping presence, and the Parquet-only ID-less-wrapper exception are documented and format-gated. Wrapper discovery still needs the complete schema identity view when access-path pruning removes identifying siblings.
- Test coverage: The patch adds substantial FE and BE unit/result coverage, including negative/empty mapping cases and nested complex structures. A V2 reader-result test for a newly defaulted projected child whose only identifying physical descendant is an unprojected sibling is required.
- Test results: No result file was changed. Per the review contract, I performed static review only and did not run local builds or tests. The macOS BE UT workflow did not reach compilation/tests because its runner supplied JDK 25 while the job requires JDK 17.
- Observability: This is deterministic local schema mapping; existing scan errors/debug state are sufficient, and no new metric or INFO log is warranted.
- Transactions and persistence: No edit-log, replay, transaction, or persisted Doris metadata format is changed.
- Data writes: The patch changes read-time interpretation only; it adds no write, commit, rollback, or atomicity path.
- FE-to-BE variables: Mapping aliases, authority presence, nested initial-default payloads, Base64 markers, and schema IDs are transported through the relevant scan-schema paths. The optional-field rollout behavior is noted above.
- Performance: Added work is schema/setup-time traversal and mapping construction rather than row-loop work. I found no material CPU, memory, or allocation regression in the changed hot paths.
- Other issues: No additional actionable finding remained after deduplication against existing review threads.
User focus: .code-review.sKMLu3/review_focus.txt specified no additional focus points.
| const ColumnDefinition* wrapper = nullptr; | ||
| for (const auto& candidate : file_schema) { | ||
| if (candidate.has_identifier_field_id() || candidate.children.empty() || | ||
| !has_shared_descendant_field_id(table_column, candidate)) { |
Contributor
There was a problem hiding this comment.
[P1] Keep full-schema IDs available for wrapper discovery
table_column has already been pruned to the requested access path here. For table s#10<a#1 initial-default=7,b#2>, file s(no id)<b#2>, and projection s.a, file-wide ID mode is active, but this helper sees only a#1; it cannot use physical b#2 to identify s. V2 therefore marks the root missing and returns NULL instead of materializing a=7 under non-null s, while V1's full TField subtree retains the wrapper. Please keep a full-schema identity view for this lookup and add a V2 result test with nullable parent rows.
Contributor
TPC-H: Total hot run time: 29623 ms |
Contributor
TPC-DS: Total hot run time: 178220 ms |
Contributor
ClickBench: Total hot run time: 25.83 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Found two P1 rolling-upgrade correctness blockers. The final steady-state Iceberg semantics are otherwise coherent across the changed V1/V2 readers, but old FE descriptors do not version the new mixed-ID projection rule or the new interpretation of existing Base64-default fields, so a BE-first rollout can return different values from old and upgraded BEs in one distributed scan.
Checkpoint conclusions:
- Goal and tests: The patch addresses authoritative partial name mappings, file-wide ID projection, wrapper retention, statement-pinned metadata, and initial defaults; focused FE/BE tests cover the intended final semantics, but mixed-release plan/execution tests for the two blockers are missing.
- Scope: Large because it repairs all ordinary/equality/position-delete and Parquet/ORC parallel paths, but the changes remain within Iceberg schema evolution and its shared scanner metadata.
- Concurrency: Immutable snapshot mapping copies and statement MVCC binding introduce no new race after pinning; no shared mutable BE state was added.
- Lifecycle and static initialization: No new static-lifetime or shutdown behavior; decoded variable-width defaults are copied into owning columns/literals before temporary buffers expire.
- Configuration: No new configuration or validation path.
- Compatibility and rolling upgrade: Blocking. Field 9 safely versions authoritative mapping aliases, but it does not gate the any-ID mode change or decoded top-level and nested defaults already carried in fields 7/8.
- Parallel paths: V1/V2 ordinary reads, equality deletes, and position-delete metadata reads now agree at steady state, including Parquet-only ID-less wrappers and strict ORC behavior; the compatibility capability must cover all of them.
- Conditional/error handling: Absent versus present-empty mapping and wrapper ambiguity are explicit; invalid Base64 defaults return errors, while invalid mapping JSON retains its pre-existing warning-and-legacy-fallback behavior.
- Test coverage and results: The diff adds broad focused unit coverage. Per the review harness I did not run local builds/tests. At the final status refresh, CheckStyle, clang-format, license, secrets, dependency, and Cloud UT checks passed; macOS BE UT failed before tests because JDK 25 was selected instead of required JDK 17; BE UT and the performance check failed, but their TeamCity diagnostics require authentication so I could not attribute a cause; FE UT and compile remained pending.
- Observability: No new production state requiring metrics or logging was introduced; existing parse errors remain surfaced.
- Persistence, transactions, and writes: Not changed.
- FE-BE variables: Recursive IDs, aliases, mapping authority, initial defaults, and Base64 markers are transported consistently; a general compatible-FE scan-semantics version is still required for the two accepted behavior changes.
- Performance: New schema traversal and immutable mapping copies occur at metadata binding or reader setup, not in the per-row hot path; no material regression identified.
- Other issues: No additional actionable correctness issue found. No extra user-provided focus was supplied.
| ? format::TableColumnMappingMode::BY_FIELD_ID | ||
| : format::TableColumnMappingMode::BY_NAME; | ||
| if (!_data_reader.file_schema.empty() && | ||
| schema_has_any_field_id(_data_reader.file_schema)) { |
Contributor
There was a problem hiding this comment.
[P1] Version the new any-ID projection rule for rolling upgrades
This path is also reached by old-FE plans: TField IDs already existed, and no name-mapping property or field-9 marker is needed. During a BE-first rollout, for table a#1, b#2 over file a#1=10, b(no id)=20, old BEs choose name mode and return b=20, while upgraded BEs choose field-ID mode and synthesize b as missing. A single distributed scan can therefore combine different results. The new authority bit gates only name-mapping fallback and is absent here. Please add a scan-semantics capability emitted only by compatible FEs, retaining the legacy all-ID selection when it is absent, and apply it consistently to V1/V2 and position-delete paths.
| // Doris internal Iceberg row locator is never a physical Iceberg data column. It is built | ||
| // from file path, row position and partition metadata for delete/update/merge. | ||
| mapping->virtual_column_type = TableVirtualColumnType::ICEBERG_ROWID; | ||
| } else if (table_column.initial_default_value.has_value()) { |
Contributor
There was a problem hiding this comment.
[P1] Gate decoded initial defaults for mixed-version BEs
Thrift fields 7/8 and the FE's Base64 map already existed and were populated recursively before this PR. For an old-FE plan whose file lacks a UUID/BINARY/FIXED field, old BEs return the carrier/generic default at a projected root and NULL/type-default for a missing child; this branch plus the new V1 hook and recursive V1/V2 child materializers decode the logical bytes. During a BE-first rollout, fragments of one scan can therefore return different values. Field 9 does not gate this behavior. Please put all of these paths behind the same compatible-FE scan capability as the projection-mode change, retaining legacy behavior when it is absent, with mixed-BE coverage for root and nested fields in both readers.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: two P1 reader-correctness issues remain.
- V2 current-schema annotation gives an earlier historical alias the same priority as a later exact current name. A renamed field followed by a new field that reuses the old name can therefore attach the old field ID/defaults to the new projection, at both root and nested access paths.
- V1 ordinary mixed-ID mapping now follows Iceberg's any-ID rule, but hidden equality-delete expansion still uses an all-fields-have-IDs rule. After a key rename, that disagreement can synthesize an existing unprojected key as missing and allow equality-deleted rows to survive in both Parquet and ORC.
Critical checkpoint conclusions:
- Goal and proof: The PR substantially implements authoritative partial name mappings, schema-pinned metadata, mixed-ID selection, initial defaults, and V1/V2 delete-reader propagation. The changed tests cover many positive, negative, legacy-plan, nested, binary, and wrapper cases, but they do not prove the two rename/name-reuse cases above, so the goal is not yet fully achieved.
- Scope and focus: The 37-file FE/Thrift/BE/test change is broad because the contract crosses planner transport and V1/V2 readers, but the edits are generally focused on that contract. The two remaining mismatches are localized correctness gaps rather than unrelated scope growth.
- Concurrency: No new thread entry, lock, lock ordering, or mutable shared reader state is introduced. The existing metadata-cache concurrency boundary is handled through the statement-pinned
IcebergSnapshotCacheValue; all production constructors were traced to the same table generation. - Lifecycle and memory safety: No new cross-TU static-initialization dependency or special teardown path was found. Long VARBINARY/UUID default payloads are materialized into owning columns before temporary decode buffers die; root, nested, equality-delete, nullable-parent, and row-count lifetimes were checked without another defect.
- Configuration: No configuration item is added or changed.
- Compatibility: The optional Thrift additions are wire-compatible. The scan-semantics marker gates the new any-ID, wrapper, and initial-default result behavior so absent-version old-FE plans retain legacy behavior; the marker reaches every range through the shared mutable scan params object.
- Parallel paths: Parquet/ORC, V1/V2, ordinary scans,
position_deletes.row, and hidden equality-delete paths were compared. The second inline issue is the remaining V1 equality-path parity miss; outside the two inline issues, the position-delete and V2 equality paths are consistent. - Conditions and error handling: New version, authoritative-mapping, any-ID, and wrapper conditions are locally documented, and checked
Statuspropagation remains intact. The hidden-key all-ID condition is no longer consistent with the ordinary semantics-v1 condition and is called out inline. - Test coverage and expected results: Added unit/result coverage is broad and deterministic in the reviewed fixtures, including absent versus present-empty mappings, partial mappings, old plans, nested containers, binary defaults, mixed IDs, wrappers, branches/snapshots, and delete readers. Missing coverage is top-level/nested exact-name-versus-alias reuse in V2 and renamed hidden equality keys in mixed-ID Parquet/ORC. No local build or test was run because the review-runner contract prohibited it.
- Nullability and data correctness: Nullable struct-parent maps, const materialization, missing defaults, and equality-key row carriers were traced. The two inline findings are silent data-correctness failures; no additional nullable-shape or visible-version issue was substantiated.
- Observability: This is deterministic scan-planning/materialization behavior and does not require new metrics or logging; existing errors carry sufficient schema/default context for the failing decode paths.
- Persistence, transactions, and writes: No EditLog, transaction, storage-format write, or data-write path is modified.
- FE-BE variables: Name-mapping authority, aliases, schema IDs, Base64 markers, initial defaults, and scan-semantics version are transported through the relevant FE construction and Thrift consumption paths. All production snapshot-cache constructors and the sole production schema-info caller were checked.
- Performance: New work is schema-size initialization/projection traversal rather than row-by-row processing. No material CPU, allocation, or memory-accounting regression was found.
- Other issues and user focus: No additional distinct issue remains after deduplication against the existing review threads.
.code-review.lELdfJ/review_focus.txtcontains no extra user-provided focus point.
| } | ||
| 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] Prefer exact current names before mapping aliases
A legal evolution can rename b#1 to renamed_b and then add a new b#2; a partial mapping may retain alias b only for #1 while #2 has an authoritative empty alias list. Because find_external_root_field() accepts either a current name or alias in one ordered pass, projecting current b can select #1 before reaching #2, and this assignment copies #1's ID/default metadata onto the projected field. ID-bearing and ID-less files can then return old field data instead of #2's initial default or NULL. Please search all siblings for an exact current name before trying aliases; find_schema_child_by_path() has the same ordering issue, so cover both shapes with V2 result tests.
| RETURN_IF_ERROR(BuildTableInfoUtil::by_parquet_field_id_with_name_mapping( | ||
| get_scan_params().history_schema_info.front().root_field, *field_desc, | ||
| ctx->table_info_node)); | ||
| ctx->table_info_node, supports_iceberg_scan_semantics_v1(&get_scan_params()))); |
Contributor
There was a problem hiding this comment.
[P1] Use the same file-wide ID mode for hidden delete keys
This call now makes ordinary mapping use field IDs when any field in the data file has an Iceberg ID, but hidden equality-key expansion still requires every field to have an ID before looking up the key by ID. For a mixed-ID file containing old_key#1 plus an unrelated ID-less sibling, after #1 is renamed to new_key, an unprojected equality-delete file can still call the key old_key. The StructNode is keyed by current name new_key, so the hidden path misses the physical #1 column, synthesizes NULL/default, and fails to delete matching rows. Please use the same any-ID decision under semantics v1 for hidden Parquet and ORC keys, retain the legacy rule for absent-version plans, and add renamed hidden-key result coverage.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29588 ms |
Contributor
TPC-DS: Total hot run time: 177382 ms |
Contributor
ClickBench: Total hot run time: 25.64 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
There was a problem hiding this comment.
Requesting changes for one additional P1 correctness issue: exact-current-name precedence is not gated by the new scan-semantics capability, so an old-FE plan can bind different field IDs on old and new BEs during a BE-first upgrade. The existing unresolved hidden equality-key file-wide-ID thread also remains applicable; I did not duplicate it inline.
Checkpoint summary:
- Goal and scope: reviewed all 37 changed files and traced the name-mapping/default transport from FE through Thrift into V1/V2 Parquet and ORC ordinary, position-delete, and equality-delete paths. No additional user focus was provided.
- Compatibility: optional Thrift fields are additive and the new any-ID/default/wrapper behaviors are generally capability-gated, but the accepted P1 violates the absent-marker legacy contract. The existing hidden-key thread also remains unresolved.
- Concurrency, lifecycle, configuration, persistence, and data writes: this patch adds no new shared-thread, resource-lifecycle, configuration, transactional, or persistence mechanism; no separate issue was found in those checkpoints.
- Parallel paths and conditionals: V1/V2 and Parquet/ORC root, nested/container, position-delete, equality-delete, and missing-default branches were compared. Other residual hidden-key cases were deduplicated to the existing thread.
- Tests and validation: the changed tests cover compatible-plan mixed IDs, wrappers, recursive defaults, mappings, and delete readers, but do not cover the old-FE alias collision in the inline finding. This was a static review only; the review runner contract prohibits builds and test execution.
- Observability and performance: new failure paths retain contextual statuses/checks, and the added schema/default work is reader/planning initialization rather than a demonstrated scan hot-path regression. No additional issue was substantiated.
| if (field == nullptr) { | ||
| continue; | ||
| if (field != nullptr && field->__isset.name && | ||
| to_lower(field->name) == to_lower(column.name)) { |
Contributor
There was a problem hiding this comment.
[P1] Gate exact-name precedence for old-FE plans
This exact-first pass changes field selection before iceberg_scan_semantics_version is checked. An old FE can send renamed_b#1 with alias b before a newly added current b#2, while omitting fields 9 and 34. For an all-ID file containing distinct values for both fields, the old BE's ordered lookup binds projected b to #1, but this new BE binds it to #2; the later capability check only clears defaults and cannot restore the identifier, so a BE-first rolling upgrade can return different values for one distributed scan. Please gate both the root and nested exact-first lookup on compatible-FE semantics, retain the former ordered lookup when the marker is absent, and cover the old-FE descriptor with both IDs present.
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)