Repository navigation
[fix](be) Keep projected structs readable with nested Iceberg equality keys in V1 Parquet - #69547
Conversation
…y keys in V1 Parquet ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: The V1 Parquet reader of an Iceberg table reads a nested equality-delete key, for example payload.existing, through its top-level struct: a hidden carrier column maps to the physical root payload, and the leaf is extracted after the read. ParquetReader::_init_read_columns keeps one table column per file column, so when the query also projects payload (any SELECT *), or when two keys sit under the same root, the later column replaces the earlier one and only one of them is read. With id and payload projected and an equality delete on payload.existing, payload came back empty when id was listed first (a DCHECK in vparquet_group_reader.cpp under ASAN), and the batch had no rows at all when payload was listed first. ORC reads are not affected. branch-4.1 (apache#66538) takes such a key from the projected root when the query projects it, lets keys under one physical root share one carrier, and lets ParquetReader read one file column under several table names. This change ports that to the V1 Parquet path on master: - IcebergParquetReader takes a nested key from the projected root column when the file has the key's full path and the projected struct holds the key with the same type. Otherwise the first key under a physical root reads the root, and later keys under that root are extracted from its column. A carrier that reads nothing gets a placeholder of the batch row count until its leaf is extracted. - The mixin extracts every nested key before it replaces any carrier column, since one carrier can be the source of other keys. - ParquetReader reads a file column once per table column that names it, and prefetches it once. ### Release note Fix empty or missing rows when the V1 Parquet reader reads an Iceberg table with an equality delete keyed on a struct field, and the query also projects that struct or the delete has several keys in the struct. ### Check List (For Author) - Test: Unit Test - IcebergReaderTest: four branch-4.1 cases for a projected root struct (Parquet and ORC, both column orders) and three Parquet cases: two keys under one unprojected root, two keys taken from the projected root, and a key that the projected struct lacks. All 50 cases pass; without the fix all five Parquet cases abort on a DCHECK - Behavior changed: Yes. Such reads return the correct rows. - Does this need documentation: No Co-authored-by: daidai <[email protected]> Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes for one verified P1: newly derived hidden equality keys violate the production Parquet synthesized-column slot contract, causing a debug assertion or an invalid block index during release lazy filtering. No existing review threads require deduplication.
Reviewed all four changed files at 6e2e371 and traced FE schema/slot construction, Parquet initialization and filtering, shared-carrier extraction, and parallel readers. This was a static-only review; no builds or tests were run.
Critical checkpoint conclusions:
- Goal: projected structs and multiple nested equality keys receive readers or derived values as intended, but the P1 prevents the new reuse paths from working safely in production.
- Scope and reuse: focused reader changes; existing synthesized filling and extraction are reused, with the slot-contract mismatch identified inline.
- Concurrency and locks: no new threads, shared concurrent state, atomics, or lock ordering.
- Lifecycle and initialization: two-phase extraction preserves carrier lifetimes; fresh expansion/shrink restores columns between batches; captured placeholders introduce no owning cycle or cross-TU initialization dependency.
- Configuration: no new settings or dynamic configuration behavior.
- Compatibility: no storage/wire-format change or externally exchanged interface change; no new libc API use identified. Compiled-artifact ABI validation is outside this static review.
- Thrift/protobuf: no definition changes.
- Parallel paths: ORC already handles the projected-root collision separately; shared extraction remains compatible. V2 uses its own mapping/expression path. Both V1 scan-semantics revisions were considered.
- Conditional checks: new branches explain projected-key reuse, physical fallback, and type promotion; field-ID, name normalization, pruning, and historical same-name cases were traced without another verified defect.
- Test coverage: seven new unit tests cover both projection orders in Parquet/ORC, shared unprojected roots, projected keys, and fallback rereads. They omit predicates and multiple batches, and BE_TEST excludes the production classifier that exposes the P1. Nulls/defaults and batch/filter lifecycle were inspected statically.
- Test results: new expected survivors and payload values match the fixtures; no regression output files changed. The author's test execution claims were not independently rerun.
- Observability: existing reader errors and IO/filter profiles cover the changed flow; no additional instrumentation requirement identified.
- Transactions and persistence: no transaction, failover, EditLog, or persistence changes; no visible-version or MoW bitmap changes.
- Writes and atomicity: no data-write path changes.
- FE/BE variables: no new variables or protocol fields to propagate.
- Performance and memory: shared carriers avoid duplicate decoding where shapes permit; physical duplicates remain adjacent and prefetch/compressed-byte accounting deduplicates them. Leaves own their copied data and statuses propagate; the inline P1 is the verified memory-safety/error-handling failure.
- Other concerns and user focus: no additional user focus was supplied and no further substantiated issue remains.
Coverage limit: runtime behavior, builds, and test execution were prohibited by the review contract. The finding is supported by the complete static call chain.
Review converged after 2 round(s).
| // the carrier at the batch row count, so the reader filters it with the other columns. | ||
| auto placeholder = delete_key_type->create_column(); | ||
| placeholder->insert_default(); | ||
| this->register_synthesized_column_handler( |
There was a problem hiding this comment.
[P1] Resolve hidden placeholders without requiring tuple slots
The new projected-root and shared-carrier paths register internal names such as __equality_delete_column__2_existing here, but those keys only exist in _expand_columns, not the query tuple. ParquetReader::_do_init_reader unconditionally calls _classify_columns_for_lazy_read, whose production-only synthesized-column loop requires a matching tuple slot. This aborts on DCHECK(slot_id != -1) in debug builds. In release, RowDescriptor::get_column_id(-1) returns -1, which is stored as UINT32_MAX in all_predicate_col_ids; with lazy materialization and a partially selective predicate (for example SELECT id, payload ... WHERE id > 1 with a nested equality delete), _do_lazy_read passes that index to Block::filter_block_internal and accesses outside the block. The added tests miss this because BE_TEST excludes the classifier loop. Please resolve these internal keys through their actual expanded block positions, or give them a separate fill/filter path, and cover production classification with a selective predicate.
There was a problem hiding this comment.
Fixed in 5acba3e. The keys that the reader does not read — a key extracted from the projected root struct, and the second key under a physical root that another key's carrier reads — are no longer synthesized columns. The reader block has no column for them during the read, so the lazy-read classifier never sees a name without a tuple slot, and on_after_read_block appends them once the batch has its final row count (_materialize_nested_equality_delete_columns appends the extracted leaf).
The missing-key carrier from #65502 had the same problem, and not only on Parquet: its synthesized handler appended the column after the reader had taken its column count, and the lazy reads of both Parquet and ORC erase columns beyond that count after filtering, so the key's block position went stale. It is now appended after the read as well, on both formats.
Tests: lazy-read variants with a filter on id for a key taken from the projected root (both column orders), for keys sharing an unprojected root and for keys taken from the projected root, plus a missing key with and without the filter on Parquet and ORC. Without the fix the four nested cases abort at vparquet_group_reader.cpp:829 (column_size == cz, 2 vs. 3) and both lazy missing-key cases fail with invalid input position, position=2; the production slot lookup itself is compiled out under BE_TEST. On reachability: master FE sets parquet_timestamp_semantics_version = 1 and Iceberg scan semantics v2, which routes Iceberg scans to FileScannerV2, so the V1 readers are reached when a BE that was upgraded first serves a plan of an older FE.
TPC-H: Total hot run time: 28660 ms |
TPC-DS: Total hot run time: 150622 ms |
…the read ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Review follow-up. The previous commit registered the equality-delete keys that the V1 Parquet reader does not read (a key extracted from a projected root struct, and the second key under a physical root that another key's carrier reads) as synthesized columns. The synthesized-column contract does not fit them: - ParquetReader::_classify_columns_for_lazy_read looks up a tuple slot for every synthesized column. These hidden keys have none, so a debug build fails DCHECK(slot_id != -1) when the reader is initialized, and a release build stores get_column_id(-1) as UINT32_MAX among the predicate columns, which Block::filter_block_internal then uses as a block position under lazy materialization. - The lazy read fills synthesized columns in its first phase at the pre-filter row count and filters only the predicate columns, so the placeholder kept rows that the filter removed (vparquet_group_reader.cpp: Check failed: column_size == cz). Keys that are missing from an old data file (apache#65502) have the same problem on both formats: their synthesized handler appends the column after the reader took its column count, and the lazy read of Parquet and of ORC erases columns beyond that count after filtering, so the key's block position goes stale and the equality-delete filter fails with "invalid input position". Master FE plans Iceberg scans for FileScannerV2, so the V1 readers are reached when a BE that has been upgraded first serves a plan of an older FE. None of these keys is a synthesized column any more. The reader block gets no column for them during the read; on_after_read_block appends them once the batch has its final row count: a missing key as its repeated initial default, an extracted key as the leaf of its source column. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergReaderTest: lazy-read variants with a filter on id for a key taken from the projected root (both column orders), for keys sharing an unprojected root and for keys taken from the projected root; a missing key with and without the filter on Parquet and on ORC. Without the fix the four lazy Parquet nested cases abort at vparquet_group_reader.cpp:829 and the two lazy missing-key cases fail with "invalid input position". - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
ClickBench: Total hot run time: 24.49 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: one new P1 finding identifies a reachable legacy-plan case where projected-root reuse reads a name-mapped NULL instead of the equality key found by field ID, allowing deleted rows to remain visible. All four changed files and their reader/schema/filter/IO dependencies were reviewed at head 5acba3e.
The earlier P1 comment 4359868360 is fixed at this head: hidden derived and missing keys no longer register synthesized handlers or enter production tuple-slot classification; they are appended after base-reader filtering. It is not reposted or retained in existing_blocking_comment_ids. There were no additional user-specified focus points.
The execution path checked was:
FE schema/slots -> IcebergParquetReader::on_before_init_reader -> ParquetReader/RowGroupReader
-> physical read and lazy/position filtering
-> materialize missing keys -> extract all nested leaves -> equality filter -> shrink hidden columns
Critical checkpoint conclusions:
- Goal and proof: the change addresses projected-struct/root-alias failures and adds 15 focused unit cases, but the new physical-binding assumption still permits the inline P1 wrong-row case.
- Scope and clarity: changes are focused on Parquet alias bookkeeping, Iceberg key materialization and test helpers; the shared extraction pass avoids replacing a carrier before other keys use it.
- Concurrency and locking: the changed reader state is per scanner and processed sequentially. No new threads, shared-state synchronization or lock order is introduced.
- Lifecycle and ownership: successful batches append keys only after the base read and erase all hidden columns/map entries before returning. Extraction creates fresh nullable leaves and preserves ancestor null maps; no new ownership cycle or cross-translation-unit static initialization dependency was found. Allocation behavior follows existing reader/column paths.
- Configuration: no configuration items are added or changed.
- Compatibility: no wire or storage layout changes. Older-FE plans can reach V1 during BE-first upgrades; this is the compatibility path affected by the P1. Current FE timestamp contracts route current versioned Iceberg scans through FileScannerV2.
- Thrift/protobuf: no definitions, field IDs or enum values change.
- Parallel paths: ORC retains independent physical alias readers and receives the shared missing-key timing fix. V2 uses separate readers. Parquet ordinary/lazy, predicate, page-index and prefetch paths were traced.
- Conditional checks: new branches document projected/shared/missing keys and consecutive physical aliases. The type check is useful but insufficient to establish the physical-binding invariant identified inline.
- Test coverage: new cases cover projected roots in both orders/formats, Parquet shared/projected/pruned roots, selective lazy filters and missing initial defaults in both formats. They omit the legacy no-marker nested mixed-ID case that triggers the P1.
- Expected results: the added expectations are internally consistent: equality deletion removes id 2; the optional id != 1 conjunct also removes id 1. No regression .out files change.
- Observability: existing error/status propagation, scanner file context and reader profiles remain in place; no additional logging or metric requirement was identified. The P1 is silent incorrect data, not a missing diagnostic for an explicit failure.
- Transactions and persistence: no transaction, edit-log, replay, visible-version or MoW delete-bitmap code changes. Iceberg equality-delete correctness is affected by the inline issue; no separate persistence concern was found.
- Writes and crash behavior: this is a read-path change; no data writes, commit atomicity or crash-recovery protocols are changed. Status failures abort the read rather than continue with partial extraction.
- FE-BE variables: no new transmitted variables or sending paths; existing semantics markers and recursive schema carriers were inspected, including a concrete pre-marker FE snapshot.
- Performance: root reuse removes redundant reads where its mapping is correct; the multimap preserves independent logical readers, and physical-order dedup avoids duplicate prefetch ranges and compressed-size accounting. Additional extraction storage is proportional to equality-key count/batch data. No separate performance defect was verified; no benchmark was run.
- Other issues: historical same-name fields, pruning/reordering, promotions, nullable ancestors, missing prefixes, shared-carrier replacement order, repeated/empty batches, count-only source-slot retention and the production classifier were checked. No other new substantiated finding remains.
Validation and coverage limits: this was static source review only; no builds, tests, runtime reproductions or benchmarks were run. New fixtures read one three-row batch; production-only classification, dual-alias lazy filtering, multiple pages/groups/batches, reuse-specific null/evolution combinations and page-index/bloom integration are not directly established by the added tests. Their implementation paths were inspected. The P1 trigger was derived from current reader code plus actual pre-marker FE schema construction, without claiming the old duplicate-root query already succeeded. All review candidates and risk focuses have explicit dispositions; one accepted new inline P1 remains, with no unresolved review candidate.
Review converged after 2 round(s).
| &leaf_type, &found)); | ||
| // The projected leaf has the table's current type. A key written before a type promotion | ||
| // (INT promoted to BIGINT) keeps its own type, so it still reads the physical root. | ||
| if (found && remove_nullable(leaf_type)->equals(*remove_nullable(key_type))) { |
There was a problem hiding this comment.
[P1] Verify physical binding before reusing a projected key
For a legacy FE plan without iceberg_scan_semantics_version, the projected struct and the hidden key can resolve different physical fields. For example, let the file contain ID-tagged roots id#1 and payload#2, with children old_key#3 INT and an unrelated ID-less sibling; the current projected type is payload<new_key#3 INT>, with no name-mapping aliases. Hidden-key lookup finds field 3 by ID, but BuildTableInfoUtil::by_parquet_field_id_with_name_mapping switches the projected struct's children to name matching when that sibling lacks an ID (table_schema_change_helper.cpp:895-921), so new_key is filled with NULL. This logical-path/type check still selects payload, and the reuse branch skips the physical carrier. An equality delete on field 3 = 20 then compares NULL and silently retains the deleted row. Older FE source actually emits nested schema metadata without the marker, so this is reachable during a BE-first rolling upgrade. Please verify that the projected schema-mapping path resolves the same physical leaf as file_path, or fall back to the carrier, and cover this legacy mixed-ID/renamed-key case. The new tests all set semantics v2 and miss this branch.
There was a problem hiding this comment.
Confirmed, and fixed in 9858972766f. With a plan from an FE that predates iceberg_scan_semantics_version, the projected struct is mapped level by level, and a level whose file children do not all carry field IDs is matched by name (kept for old FE plans by #65784), while the hidden key is located by field ID. find_projected_root checked only the key's logical path and type, so for a file payload{existing without an ID, k#3} with k renamed it took the key from the projected renamed_k, which that mapping reads as NULL. Before taking a key from the projected root, the reader now checks that the projected column's schema mapping resolves the root and every level of the key's path to the same file fields as the key's physical path, the way the struct reader resolves each child (children_file_column_name); when it does not, the key reads its own carrier, which the multimap change already supports. v1_parquet_legacy_plan_key_rereads_root_when_projected_child_matches_by_name covers this file under a legacy plan (without the check, id 2 survives the delete) and under a plan with the marker, where both sides bind k#3 by field ID and the key still comes from the projected payload.
On reachability: plans that carry the marker resolve both sides by field ID, and current FEs also send the Parquet timestamp marker, which routes Iceberg scans to FileScannerV2. So this case needs an FE at 4.1.3 or older during a BE-first upgrade, plus a data file whose nested struct mixes ID-tagged and ID-less children, which Iceberg writers, Doris's included, do not produce. branch-4.1 (since 4.1.4) and branch-4.2 resolve the projected root the same way; we will report it there.
TPC-H: Total hot run time: 28624 ms |
TPC-DS: Total hot run time: 151020 ms |
ClickBench: Total hot run time: 24.22 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…only when both bind the same file fields ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: The V1 Parquet Iceberg reader takes a nested equality-delete key from the projected root struct when the query projects that root, using the key's logical path in the current schema and the projected type. The projected column is read through its own schema mapping, while the hidden key is located in the file by field ID (or by its name path). For a plan without iceberg_scan_semantics_version, which an FE at 4.1.3 or older sends during a BE-first upgrade, the mapping matches a struct level by name when not all of its file children carry field IDs, so the two can bind different file fields. With a data file payload{existing without an ID, k#3} and the key renamed to renamed_k, the projected payload read renamed_k as NULL while the key's physical path was payload.k, and the reader took the NULL: an equality delete on renamed_k = 200 kept the row it deletes (or, with a NULL tombstone, would have deleted every row whose key reads NULL). The reader now takes the key from the projected root only when the projected column's mapping resolves the root and every level of the key's path to the same file fields as the key's physical path, as the struct reader resolves each child; otherwise the key reads its own carrier, the path this PR already uses for a key outside the projected struct. ### Release note None ### Check List (For Author) - Test: Unit Test - IcebergReaderTest.v1_parquet_legacy_plan_key_rereads_root_when_projected_child_matches_by_name: a legacy plan over payload{existing without an ID, k#3} with k renamed deletes id 2 again (it kept it without the check); with the scan semantics marker the key still comes from the projected payload - the existing V1 Parquet equality-delete tests of this PR - Behavior changed: No (a legacy plan over such a file reads the key from its carrier instead of the projected root) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
Addressed the second review round in
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed PR #69547 at 9858972766f2fcb75452e2e887507ae9e2c631b9. No new verified findings. Review converged after one round and the final sweep, with two complete-scope reviewers and a separate schema/batch-lifecycle risk reviewer.
Both prior P1 findings were independently revalidated as fixed: 4359868360 no longer applies because hidden missing/derived keys never enter synthesized-column classification and are appended after base filtering; 4365360556 no longer applies because reuse now verifies the projected mapping against every physical path component and otherwise reads a carrier. Neither is retained as an existing blocker or reposted.
The inspected flow is FE schema/delete metadata -> reader initialization and physical-column selection -> base read and filtering -> missing/nested key materialization -> equality-delete filtering -> internal-column cleanup.
Critical checkpoint conclusions:
- Goal and tests: projected structs and nested equality keys receive their intended data. Added tests check projected column order, shared roots, separate-carrier fallback, lazy predicates, missing-key defaults, and legacy mixed-ID binding. Static evidence supports the intended fix.
- Scope and reuse: changes stay within the three reader implementation files and their unit tests; existing carrier extraction handles incompatible projections.
- Concurrency and locking: new state belongs to an individual reader's synchronous initialization/read flow. No new threads, shared mutable state, locks, heavy locked operations, or lock-order changes were introduced.
- Lifecycle and static initialization: all leaves are extracted before any shared carrier is replaced; projected owners are read without mutation. Every successful batch removes internal columns and their position entries. No new ownership cycle or cross-translation-unit static dependency was found.
- Configuration: none added or changed.
- Compatibility: no wire, stored-format, or serialization contract changes. Legacy schema mapping is preserved, with physical-binding checks guarding projected reuse. Type mismatches retain the existing carrier path and its pre-existing casting limitations.
- Thrift/protobuf: no definitions or field IDs changed.
- Parallel paths: ORC retains its existing projected-root behavior and shares the corrected post-read missing-key materialization. Parquet requires the multimap to retain distinct logical readers of one physical column.
- Conditional checks: reuse, fallback, and shared-root branches have concrete comments explaining the failing layouts and required invariants.
- Coverage: all four changed paths, upstream schema/slot construction, reader consumers, and eager/lazy/empty/repeated-batch cleanup were inspected. The new tests are focused unit integrations rather than full scanner/cluster tests.
- Expected results: fixture rows and equality keys agree with the expected surviving IDs and structs; the legacy case correctly retains NULL in the projected renamed child while its separate key carrier still applies the delete. No result files changed.
- Observability: existing reader/delete timing, row, and I/O counters and field-ID/path errors cover these local operations; no additional distributed tracing requirement was identified.
- Transactions and persistence: this is read-side filtering; transaction visibility/version selection, delete bitmaps, EditLog, persistence, and failover state are unchanged.
- Data writes and error handling: no write or atomicity path changed. New Status-returning operations propagate failures; no silent continuation was found.
- FE/BE variables: none added; current and legacy slot enumeration supply the projected roots used here.
- Performance and memory: compatible projected roots and shared carriers avoid redundant decoding. Fallback readers own independent decoder state, while physical prefetch ranges and compressed-size accounting are deduplicated. Column allocation and nullable-ancestor copying follow existing machinery; no material new performance or ownership defect was established.
- Other risks: physical binding/pruning, post-filter key lifetime, and duplicate-reader I/O were explicitly checked and dismissed with source evidence. No unresolved code candidate remains. No additional user focus was supplied.
Validation limits: static review only; no builds, tests, ASAN run, or final-ELF audit was performed. Added tests read small single batches and do not directly execute the production-only classifier excluded by BE_TEST, multiple batches/row groups, zero-survivor batches, or every nullable-parent/type-promotion combination. Those relevant source paths were traced, but runtime results are unverified.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28247 ms |
TPC-DS: Total hot run time: 150363 ms |
ClickBench: Total hot run time: 24.22 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: None
Related PR: #66538
Problem Summary:
In short. This ports a fix from branch-4.1 #66538 (Iceberg V3 default values) to the V1 Parquet reader on master. When an Iceberg equality delete is keyed on a struct field such as
payload.existing, and the query also projectspayload(anySELECT *), or the delete has several keys in the same struct, master reads only one of the columns that map to the struct, and returns an empty struct or no rows.Background. The V1 Parquet reader reads a nested equality key through its top-level struct. A hidden carrier column named
__equality_delete_column__<id>_<leaf>maps to the physical root (payload), the whole struct is read into it, and the key's leaf is extracted after the read.ParquetReader::_init_read_columnsbuilds a map from each file column to one table column.The problem, and what it cost. When two table columns name the same file column, the later one replaces the earlier one in that map, and only one of them gets a column reader. Two cases do this: a projected
payloadnext to the carrier ofpayload.existing, and the carriers of two keys under one root, such aspayload.aandpayload.b. Reproduced with BE unit tests on a file holding (1,{10}), (2,{20}), (3,{30}) and an equality delete onpayload.existing = 20:id, payloadpayloadis empty, and the block has fewerpayloadrows thanidrows (a DCHECK invparquet_group_reader.cppunder ASAN)payload, idid, with keyspayload.existingandpayload.kpayload.existingis never readORC reads of a projected root struct are not affected: the ORC cases below pass without this change.
How this PR fixes it. It follows branch-4.1 / branch-4.2 (the Parquet carrier setup in
be/src/format/table/iceberg_reader.cppandParquetReader::set_duplicate_file_column_aliases):ParquetReaderreads a file column once for each table column that names it, and prefetches it once. This covers a key that the projected struct lacks, for example a key dropped from the table, whose carrier reads the same root as the projectedpayload.Two differences from branch-4.x, both on the safe side: a key whose path is incomplete in the file (an older file written before the key existed) keeps reading the physical root instead of relying on the projected struct for the missing child, and a key whose type differs from the projected type (a key written before a type promotion) also keeps reading the physical root. Branch-4.x casts that case; master does not cast equality keys yet, which is a separate fix.
Equality-delete keys that the V1 reader does not read are not synthesized columns: a key extracted from a projected root or from another key's carrier, and a key missing from an old data file, are appended in on_after_read_block once the batch has its final row count. This also fixes the missing-key path from #65502 on Parquet and ORC, whose synthesized column was dropped by the lazy read's column trimming.
Results. All 58 tests of
IcebergReaderTestpass. Without this change, each of the five Parquet cases below aborts on the DCHECKblock->rows() == col.column->size()invparquet_group_reader.cpp, because one of the columns that share the root is never read:payloadlisted before and afterid. Without this change the Parquet cases leavepayloadunread in both orders; the ORC cases already pass.v1_parquet_equality_keys_share_unprojected_root: two keys under an unprojected root share one carrier. Without this change the carrier ofpayload.existingis never read.v1_parquet_equality_keys_come_from_projected_root: two keys are taken from the projected root. Without this changepayloadis never read.v1_parquet_equality_key_outside_projected_struct_rereads_root: the projected struct lacks the key, so the carrier reads the same file column again. Without this changepayloadis never read.v1_parquet_legacy_plan_key_rereads_root_when_projected_child_matches_by_name(follow-up): a plan without the Iceberg scan semantics marker mapspayload{existing without an ID, k#3}by name, so the projectedpayloadreads the renamed key as NULL while the key is found by field ID; the key must read its own carrier. Without the binding check id 2 survives the delete. Under a plan with the marker the key still comes from the projectedpayload.Eight more IcebergReaderTest cases cover lazy materialization under a filter on id (nested keys on Parquet, missing keys on Parquet and ORC); without the fix they abort at vparquet_group_reader.cpp:829 or fail with "invalid input position". IcebergReaderTest has 59 cases, all passing after the follow-up commit; the Iceberg reader suites (366 cases) passed on 5acba3e, before it.
Original author: @hubgeter
Release note
Fix empty or missing rows when the V1 Parquet reader reads an Iceberg table with an equality delete keyed on a struct field, and the query also projects that struct or the delete has several keys in the struct.
Check List (For Author)
Test
Behavior changed:
Such reads return the correct rows.
Does this need documentation?
Check List (For Reviewer who merge this PR)