[fix](be) Fix Iceberg row-level deletes across schema evolution#65502
Merged
Gabriel39 merged 1 commit intoJul 14, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review completed after two rounds: main risk scan, normal coverage subagents, risk-focused subagents, and a convergence round all ended with the final M1-M5 set and NO_NEW_VALUABLE_FINDINGS after that set stabilized. I found five issues that should be fixed before merge.
Checkpoint conclusions:
- Scope/user focus: Reviewed the authoritative diff for the changed Iceberg V2 delete, reader, and test paths. No additional user-provided focus was supplied.
- Correctness/schema compatibility: Issues found in equality-delete row sizing and BY_NAME schema-evolution binding.
- Lifecycle/cache: Issue found in shared delete-file cache scoping across split filesystem identities.
- Tests: Added tests cover useful positive paths but miss the accepted edge cases, and one regression leaks session state on failure.
- Validation: Static review only; per the review prompt I did not build, run tests, or modify source.
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare absent in this checkout.
Contributor
There was a problem hiding this comment.
Automated review completed after two rounds: main risk scan, normal coverage subagents, risk-focused subagents, and a convergence round all ended with the final M1-M5 set and NO_NEW_VALUABLE_FINDINGS after that set stabilized. I found five issues that should be fixed before merge.
Checkpoint conclusions:
- Scope/user focus: Reviewed the authoritative diff for the changed Iceberg V2 delete, reader, and test paths. No additional user-provided focus was supplied.
- Correctness/schema compatibility: Issues found in equality-delete row sizing and BY_NAME schema-evolution binding.
- Lifecycle/cache: Issue found in shared delete-file cache scoping across split filesystem identities.
- Tests: Added tests cover useful positive paths but miss the accepted edge cases, and one regression leaks session state on failure.
- Validation: Static review only; per the review prompt I did not build, run tests, or modify source.
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare absent in this checkout.
| // Migrated Hive tables may have no field ids in their physical files. Once the mapper has | ||
| // selected BY_NAME mode, bind the delete key by its delete-file name or by the table column's | ||
| // historical name mapping, exactly as normal column materialization does for a renamed field. | ||
| const auto table_field_it = std::ranges::find_if( |
Contributor
There was a problem hiding this comment.
This lookup makes equality-delete binding depend on the query projection. _projected_columns comes from the scan's required slots, so a query that reads only data or count(*) does not necessarily include the equality key's table metadata. For an old field-id-less file with physical legacy_id, a current table field current_id with name_mapping = legacy_id, and an equality delete file for field id 0 named current_id, the test where current_id is projected works, but SELECT data ... reaches this code with no table field for id 0. The fallback then treats the key as a missing column and uses a NULL literal, so a non-null delete key like current_id = 2 will not delete the row. Equality-delete key binding needs to use table schema/history metadata independent of the selected columns, or add the delete key as a hidden mapping before resolving the data file field.
github-actions
Bot
dismissed
their stale review
Superseded by review 4680092049 after repairing inline anchors.
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 12, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65502 Problem Summary: Follow-up review found that equality-delete evaluation could derive a zero batch size from an unread first block column, BY_NAME equality-key binding depended on query projection and did not share the mapper's complete name semantics, and parsed delete-file caches were not isolated by filesystem identity. It also found that the Iceberg regression suite could leak its FileScannerV2 session override after a failure. Derive predicate row count from populated block columns, resolve hidden equality keys from the complete current table schema through the common BY_NAME matcher, include fs_name in both delete cache keys, and restore the session variable in a finally block. ### Release note Fix Iceberg V2 equality and position delete handling for lazy reads, schema evolution, and multi-filesystem scans. ### Check List (For Author) - Test: Unit Test / Regression test - `EqualityDeletePredicateTest.*:IcebergV2ReaderTest.Iceberg*Delete*` (29 tests) - Iceberg schema-evolution regression suite loaded successfully; external Iceberg data execution was disabled by the validation environment - Behavior changed: Yes, V2 delete filtering now uses reliable batch sizing, complete schema name mapping, and filesystem-scoped caches - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review completed after three rounds, including main-agent risk scan, full-review subagents, risk-focused subagents, candidate verification, duplicate suppression against existing threads, and final convergence.
I found three additional blocking issues in the current head:
- Missing equality-delete keys are always compared as NULL, which does not preserve Iceberg initial-default/default semantics for old files.
- Equality-delete cache keys are still structurally ambiguous when raw paths are concatenated with colon-prefixed field ids.
IcebergTableReader::prepare_split()now readsSplitReadOptions::cacheunconditionally even though the pointer has no default initializer.
Checkpoint conclusions: data correctness is still at risk for equality deletes in the missing/defaulted-column and cache-collision cases; lifecycle/test determinism is at risk for the uninitialized cache pointer; concurrency did not show a distinct new issue; position-delete grouping/bounds and delete-conjunct polarity were rechecked without additional non-duplicate findings; no configuration, persistence, FE-BE protocol, or observability change requires a separate comment here.
User focus: no additional user-provided focus was supplied.
Validation: static review only. I did not build or run tests, per the review instructions for this GitHub Actions review environment.
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 12, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65502 Problem Summary: Iceberg V2 equality delete handling could treat fields missing from old data files as NULL even when the field has a non-null initial default. Delete-file cache keys also allowed structural collisions between raw paths and appended equality field ids, and split cache pointers could be read before initialization. Carry Iceberg initial defaults through FE and BE schema metadata, use them for missing equality keys, length-encode cache key components, and initialize the split cache pointer. The existing lazy-batch row-count fix prevents the attached equality-delete filter-size coredump. ### Release note Fix Iceberg V2 equality and position delete correctness across schema evolution and cached splits. ### Check List (For Author) - Test: Regression test / Unit Test - ./build.sh --be --fe - EqualityDeletePredicateTest.*:IcebergV2ReaderTest.Iceberg*Delete* (30 passed) - ExternalUtilTest, IcebergUtilsTest (25 passed) - Behavior changed: Yes, missing Iceberg fields use their initial defaults and delete cache keys are structurally isolated - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 5:19 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29931 ms |
Contributor
TPC-DS: Total hot run time: 180574 ms |
Contributor
ClickBench: Total hot run time: 24.98 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 13, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65502 Problem Summary: The Iceberg equality-delete schema-change regression test indexed the SQL string literal before invoking the sql helper. Groovy therefore attempted to take index 1 from a one-character string and failed before the test queries ran. Store the SQL result rows first, validate that the variable exists, and then extract the session value for restoration. ### Release note None ### Check List (For Author) - Test: Regression test - Remote minimal regression test covering the same SQL-result extraction passed - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR 65502 at head daf0c5006176be580f7599e606fcc4eaaf1d6283. I found one remaining blocker, left inline.
Checkpoint conclusions: goal/scope is clear and the remaining issue is patch-scoped to the new Iceberg initial-default carrier. Concurrency/lifecycle/cache behavior was rechecked against the existing threads and current patch; I found no additional non-duplicated cache or split-lifetime issue. Compatibility and parallel paths were checked for scanner V2 versus fallback, Parquet/ORC delete handling, thrift optionality, and generated outputs; no additional issue found beyond the default-carrier problem. Condition checks and delete-predicate polarity/row sizing were rechecked across BE consumers; no additional issue found. Tests cover integer defaults and several BE delete paths, but miss the timestamp and VARBINARY-mapped default cases called out inline. Observability is not materially changed. Transaction/persistence/data-write boundaries are not directly changed; FE-BE schema/default transport is the relevant boundary and is where the blocker sits. Performance risk appears bounded to the new delete/default handling paths; no extra performance finding. Static review only per prompt; I did not build or run tests.
Contributor
TPC-H: Total hot run time: 29535 ms |
Contributor
TPC-DS: Total hot run time: 180849 ms |
Contributor
ClickBench: Total hot run time: 25.15 s |
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65502 Problem Summary: Iceberg equality-delete keys can be absent from older data files after schema evolution. Both scanner implementations treated such keys as NULL even when Iceberg assigned a non-null initial default, and the default carrier was not type-safe for timestamp and VARBINARY-mapped values. This change sends complete current schema metadata, normalizes initial defaults by type, materializes missing keys in FileScanner V1, and builds typed missing-key expressions in FileScanner V2. It preserves Iceberg type promotion when the table and delete-file key types differ. ### Release note Fix Iceberg equality deletes for evolved columns with initial defaults in FileScanner V1 and V2. ### Check List (For Author) - Test: Unit Test - Remote BE ASAN build - 3 focused Iceberg BE unit tests - IcebergUtilsTest#testParseSchemaPreservesInitialDefault - C++ format check - Behavior changed: Yes, both file scanners now apply equality deletes using the logical Iceberg initial value for columns absent from older files - Does this need documentation: No
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 13, 2026
### What problem does this PR solve? Issue Number: close #0 Related PR: apache#65502 Problem Summary: Format the missing Iceberg equality-delete key helper with the project clang-format 16 style. ### Release note None ### Check List (For Author) - Test: No need to test (format-only change); clang-format 16 verification passed - Behavior changed: No - Does this need documentation: No
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65502 Problem Summary: Iceberg equality-delete keys can be hidden from the query projection, absent from older data files, or mapped by historical names. V1 ORC attempted to read absent keys as physical columns, BY_NAME resolution could select stale field IDs, and binary or timestamptz initial defaults could lose their physical representation. Preserve complete schema metadata, resolve BY_NAME keys only by names and aliases, delay missing-key materialization until the batch row count is known, preserve binary defaults through an explicit Base64 marker, and inherit timestamp-with-zone mapping for delete readers. ### Release note Fix Iceberg equality-delete filtering across schema evolution, binary mappings, ORC files, and timestamp-with-zone values. ### Check List (For Author) - Test: - Unit Test: 65 focused BE Iceberg and equality-delete tests passed - Unit Test: 25 focused FE schema and Iceberg utility tests passed - Behavior changed: Yes, equality deletes now use correct schema mappings and initial-default representations - Does this need documentation: No
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#65502 Problem Summary: Iceberg equality-delete handling could bind stale field ids in BY_NAME mode, lose binary initial-default bytes, treat missing V1 keys as physical columns, select id-less fields by stale ids, derive binary defaults from the latest schema instead of the selected snapshot, and read delete-file TIMESTAMPTZ keys without the data reader mapping option. Complex schema rematerialization could also trust a nested descriptor that omitted nullability, producing Struct(String) under a Struct(Nullable(String)) output type. This change uses shared name mapping, carries lossless binary defaults from the selected snapshot schema, materializes absent V1 keys as full columns, propagates TIMESTAMPTZ mapping, and treats the parent complex DataType as the authoritative child-type contract. ### Release note Fix Iceberg equality deletes for evolved schemas, binary defaults, missing V1 keys, TIMESTAMPTZ keys, and nullable nested fields. ### Check List (For Author) - Test: Unit Test - 17 focused BE ASAN tests covering Iceberg readers, the nullable renamed struct reproduction, and rebase compatibility - 5 existing complex projection/materialization BE ASAN tests - 17 Iceberg DDL/DML planning FE tests - 29 FE tests in ExternalUtilTest, IcebergUtilsTest, and IcebergScanNodeTest - Targeted clang-format 16 check - Behavior changed: Yes, equality deletes and evolved complex columns now resolve and materialize using the correct mapping, snapshot schema, and authoritative table types - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29213 ms |
Contributor
TPC-DS: Total hot run time: 180325 ms |
Contributor
ClickBench: Total hot run time: 25.74 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#65502 Problem Summary: Iceberg equality-delete handling could bind stale field ids in BY_NAME mode, lose binary initial-default bytes, treat missing V1 keys as physical columns, select id-less fields by stale ids, derive binary defaults from the latest schema instead of the selected snapshot, and read delete-file TIMESTAMPTZ keys without the data reader mapping option. Complex schema rematerialization could also trust a nested descriptor that omitted nullability, producing Struct(String) under a Struct(Nullable(String)) output type. This change uses shared name mapping, carries lossless binary defaults from the selected snapshot schema, materializes absent V1 keys as full columns, propagates TIMESTAMPTZ mapping, and treats the parent Struct DataType as the authoritative child-type contract while preserving Array and Map wrapper semantics. ### Release note Fix Iceberg equality deletes for evolved schemas, binary defaults, missing V1 keys, TIMESTAMPTZ keys, and nullable nested fields. ### Check List (For Author) - Test: Unit Test - 17 focused BE ASAN tests covering Iceberg readers, the nullable renamed struct reproduction, and rebase compatibility - 5 existing complex projection/materialization BE ASAN tests - 175 ColumnMapper and TableReader BE ASAN tests - 17 Iceberg DDL/DML planning FE tests - 29 FE tests in ExternalUtilTest, IcebergUtilsTest, and IcebergScanNodeTest - Targeted clang-format 16 check - Behavior changed: Yes, equality deletes and evolved complex columns now resolve and materialize using the correct mapping, snapshot schema, and authoritative table types - Does this need documentation: No
Gabriel39
force-pushed
the
codex/fix-v2-iceberg-delete-handling
branch
from
189521b to
37cc0f8
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29600 ms |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 179855 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review completed for PR 65502.
I found one blocking V1 Iceberg reader issue and left it inline. The rest of the reviewed V2 equality-delete/default/cache/row-count paths either look addressed by this patch or are already covered by existing review threads, so I did not duplicate those comments.
Part 1.3 checkpoint conclusions:
- Goal and tests: the PR largely targets Iceberg equality-delete correctness across schema changes/defaults and adds relevant BE/FE/regression coverage, but the V1 metadata-column path is still missing coverage and has the inline failure below.
- Scope and focus: the change is focused on Iceberg reader/default/equality-delete plumbing; the remaining issue is in a parallel V1 init path touched by the patch.
- Concurrency and lifecycle: no new thread-safety, lock-order, static-init, or lifecycle defect found; the reviewed delete-file cache is scanner/operator local.
- Configuration: no new config item found.
- Compatibility and protocol: new thrift fields are optional append-only fields; rolling/pruned-descriptor default concerns were checked and are duplicate-covered by existing comments.
- Parallel paths: V1 Parquet/ORC and V2 paths were checked. The accepted issue affects V1 Parquet and the ORC twin; V2 does not use this
StructNodetuple-slot lookup path. - Conditions and error handling: most new checks follow existing patterns, but the inline issue treats every tuple slot as present in
StructNode, which is false for non-file-backed global row-id metadata slots. - Test coverage and results: tests were reviewed statically; I did not run builds or tests per the review prompt. This runner also lacks
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. - Observability, persistence, and writes: no new observability, persistence/EditLog, or data-write transaction issue found for this read-path change.
- Performance: no additional non-duplicate performance issue found after checking cache keying/lifetime and delete predicate row sizing.
- User focus: no additional user-provided focus was present.
yiguolei
approved these changes
Jul 14, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
Gabriel39
changed the title
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Jul 14, 2026
Issue Number: None Related PR: apache#65502 Problem Summary: Iceberg equality-delete handling could bind stale field ids in BY_NAME mode, lose binary initial-default bytes, treat missing V1 keys as physical columns, select id-less fields by stale ids, derive binary defaults from the latest schema instead of the selected snapshot, and read delete-file TIMESTAMPTZ keys without the data reader mapping option. Complex schema rematerialization could also trust a nested descriptor that omitted nullability, producing Struct(String) under a Struct(Nullable(String)) output type. This change uses shared name mapping, carries lossless binary defaults from the selected snapshot schema, materializes absent V1 keys as full columns, propagates TIMESTAMPTZ mapping, and treats the parent Struct DataType as the authoritative child-type contract while preserving Array and Map wrapper semantics. Fix Iceberg equality deletes for evolved schemas, binary defaults, missing V1 keys, TIMESTAMPTZ keys, and nullable nested fields. - Test: Unit Test - 17 focused BE ASAN tests covering Iceberg readers, the nullable renamed struct reproduction, and rebase compatibility - 5 existing complex projection/materialization BE ASAN tests - 175 ColumnMapper and TableReader BE ASAN tests - 17 Iceberg DDL/DML planning FE tests - 29 FE tests in ExternalUtilTest, IcebergUtilsTest, and IcebergScanNodeTest - Targeted clang-format 16 check - Behavior changed: Yes, equality deletes and evolved complex columns now resolve and materialize using the correct mapping, snapshot schema, and authoritative table types - Does this need documentation: No
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
3 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: None
Related PR: None
Problem Summary:
Iceberg row-level delete handling had correctness and efficiency gaps across the V1 and V2 file scanners:
This change:
SplitReadOptions.cache, with filesystem, path, and equality-field layout in the cache identity, and applies inclusive position bounds.Release note
Fix Iceberg equality and position deletes across schema evolution, including missing and renamed equality keys, initial defaults, historical files without field ids, and delete-file reuse across splits.
Check List (For Author)
ExternalUtilTest,IcebergUtilsTest, andIcebergScanNodeTest